[Qemu-devel] [PATCH V9 2/2] Add a new qmp command to do checkpoint, query xen replication status

[Zhang Chen]

We can call this qmp command to do checkpoint outside of qemu.
Xen colo will need this function.

Signed-off-by: Zhang Chen 
Signed-off-by: Wen Congyang 
---
 migration/colo.c | 23 +++
 qapi-schema.json | 49 +
 2 files changed, 72 insertions(+)

diff --git a/migration/colo.c b/migration/colo.c
index 0114899..2b241b4 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -130,6 +130,29 @@ void qmp_xen_set_replication(bool enable, bool primary,
 }
 }
 
+ReplicationStatus *qmp_query_xen_replication_status(Error **errp)
+{
+Error *err = NULL;
+ReplicationStatus *s = g_new0(ReplicationStatus, 1);
+
+replication_get_error_all();
+if (err) {
+s->error = true;
+s->has_desc = true;
+s->desc = g_strdup(error_get_pretty(err));
+} else {
+s->error = false;
+}
+
+error_free(err);
+return s;
+}
+
+void qmp_xen_colo_do_checkpoint(Error **errp)
+{
+replication_do_checkpoint_all(errp);
+}
+
 static void colo_send_message(QEMUFile *f, COLOMessage msg,
   Error **errp)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index 9445b93..1310f61 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5931,6 +5931,55 @@
   'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
 
 ##
+# @ReplicationStatus:
+#
+# The result format for 'query-xen-replication-status'.
+#
+# @error: true to error, false to normal.
+#
+# @desc: #optional the human readable error description string, when
+#@error is 'true'.
+#
+# Since: 2.9
+##
+{ 'struct': 'ReplicationStatus',
+  'data': { 'error': 'bool', '*desc': 'str' } }
+
+##
+# @query-xen-replication-status:
+#
+# Query replication status while the vm is running.
+#
+# Returns: A @ReplicationResult object showing the status.
+#
+# Example:
+#
+# -> { "execute": "query-xen-replication-status" }
+# <- { "return": [ { "error": false },
+#  { "error": true } ] }
+#
+# Since: 2.9
+##
+{ 'command': 'query-xen-replication-status',
+  'returns': 'ReplicationStatus' }
+
+##
+# @xen-colo-do-checkpoint:
+#
+# Xen uses this command to notify replication to trigger a checkpoint.
+#
+# Returns: nothing.
+#
+# Example:
+#
+# -> { "execute": "xen-colo-do-checkpoint" }
+# <- { "return": {} }
+#
+# Since: 2.9
+##
+{ 'command': 'xen-colo-do-checkpoint' }
+
+##
 # @GICCapability:
 #
 # The struct describes capability for a specific GIC (Generic
-- 
2.7.4

[Qemu-devel] [PATCH V9 1/2] Add a new qmp command to start/stop replication

[Zhang Chen]

We can call this qmp command to start/stop replication outside of qemu.
Like Xen colo need this function.

Signed-off-by: Zhang Chen 
Signed-off-by: Wen Congyang 
Reviewed-by: Eric Blake 
Reviewed-by: Stefano Stabellini 
Reviewed-by: zhanghailiang 

---
 migration/colo.c | 26 ++
 qapi-schema.json | 25 +
 2 files changed, 51 insertions(+)

diff --git a/migration/colo.c b/migration/colo.c
index 93c85c5..0114899 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -19,6 +19,8 @@
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "migration/failover.h"
+#include "replication.h"
+#include "qmp-commands.h"
 
 #define COLO_BUFFER_BASE_SIZE (4 * 1024 * 1024)
 
@@ -104,6 +106,30 @@ void colo_do_failover(MigrationState *s)
 }
 }
 
+void qmp_xen_set_replication(bool enable, bool primary,
+ bool has_failover, bool failover,
+ Error **errp)
+{
+ReplicationMode mode = primary ?
+   REPLICATION_MODE_PRIMARY :
+   REPLICATION_MODE_SECONDARY;
+
+if (has_failover && enable) {
+error_setg(errp, "Parameter 'failover' is only for"
+   " stopping replication");
+return;
+}
+
+if (enable) {
+replication_start_all(mode, errp);
+} else {
+if (!has_failover) {
+failover = NULL;
+}
+replication_stop_all(failover, failover ? NULL : errp);
+}
+}
+
 static void colo_send_message(QEMUFile *f, COLOMessage msg,
   Error **errp)
 {
diff --git a/qapi-schema.json b/qapi-schema.json
index cbdffdd..9445b93 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -5906,6 +5906,31 @@
 { 'command': 'xen-load-devices-state', 'data': {'filename': 'str'} }
 
 ##
+# @xen-set-replication:
+#
+# Enable or disable replication.
+#
+# @enable: true to enable, false to disable.
+#
+# @primary: true for primary or false for secondary.
+#
+# @failover: #optional true to do failover, false to stop. but cannot be
+#specified if 'enable' is true. default value is false.
+#
+# Returns: nothing.
+#
+# Example:
+#
+# -> { "execute": "xen-set-replication",
+#  "arguments": {"enable": true, "primary": false} }
+# <- { "return": {} }
+#
+# Since: 2.9
+##
+{ 'command': 'xen-set-replication',
+  'data': { 'enable': 'bool', 'primary': 'bool', '*failover' : 'bool' } }
+
+##
 # @GICCapability:
 #
 # The struct describes capability for a specific GIC (Generic
-- 
2.7.4

[Qemu-devel] [PATCH V9 0/2] Add new qmp commands to suppurt Xen COLO

[Zhang Chen]

Xen COLO depend on qemu COLO replication function.
So, We need new qmp commands for Xen to use qemu replication.

Corresponding libxl patches already in xen.git.
Commit ID:

ed37ef1f91c20f0ab162ce60f8c38400b917fa64
COLO: introduce new API to prepare/start/do/get_error/stop replication

a0ddc0b359375b2418213966dfbdbfab593ecc6f
tools/libxl: introduction of libxl__qmp_restore to load qemu state


Zhang Chen (2):
  Add a new qmp command to start/stop replication
  Add a new qmp command to do checkpoint, query xen replication status

 migration/colo.c | 49 +
 qapi-schema.json | 74 
 2 files changed, 123 insertions(+)

-- 
2.7.4

Re: [Qemu-devel] [PATCH v14 00/24] MTTCG Base enabling patches with ARM enablement

[Alex Bennée]

Richard Henderson  writes:

> On 02/24/2017 05:29 AM, Alex Bennée wrote:
>> Hi Richard/Peter,
>>
>> Well obviously it was expecting a bit much for v13 to pass with flying
>> colours. A merge failure which didn't affect ARM caused the
>> regressions on other platforms resulting in the iothread not being
>> released during cpu_handle_interrupt. This fix has been merged into
>> Jan's drop global lock patch. I also fixed a whitespace issue on the
>> error_report/printf for the memory order check.
>
> LGTM.  I've re-tested with Alpha and everything seems to be ok.
> You've got all of my R-B already.

Cool.

>> So if you are happy are you going to be able to submit the pull
>> request before the soft-freeze kicks in?
>
> Are you looking for someone else to sent the pull?  I suspect that
> Peter and I are happy for you to do so.

I was following convention:

  "Generally only existing submaintainers of a tree will need to submit
  pull requests, although occasionally for a large patch series we might
  ask a submitter to send a pull request. "

So if Peter is happy I'll send the pullreq today. Peter?

--
Alex Bennée

Re: [Qemu-devel] [Qemu-ppc] [PATCH v4 06/15] target/ppc: remove xer split-out flags(so, ov, ca)

[Nikunj A Dadhania]

Nikunj A Dadhania  writes:

> Richard Henderson  writes:
>
>> On 02/24/2017 01:58 PM, David Gibson wrote:
>>> On Fri, Feb 24, 2017 at 06:18:22AM +0530, Nikunj A Dadhania wrote:
 Richard Henderson  writes:

> On 02/24/2017 06:56 AM, Nikunj A Dadhania wrote:
>> Now get rid all the split out variables so, ca, ov. After this patch,
>> all the bits are stored in CPUPPCState::xer at appropriate places.
>>
>> Signed-off-by: Nikunj A Dadhania 
>> ---
>>  target/ppc/cpu.c|   8 +---
>>  target/ppc/cpu.h|  26 ++--
>>  target/ppc/int_helper.c |  12 +++---
>>  target/ppc/translate.c  | 106 
>> +---
>>  4 files changed, 78 insertions(+), 74 deletions(-)
>
> I do not think this is a good direction to take this.

 Hmm, any particular reason?
>>>
>>> Right, I suggested this, but based only a suspicion that the split
>>> variables weren't worth the complexity.  I'm happy to be corrected by
>>> someone with better knowledge of TCG, but it'd be nice to know why.
>>
>> Normally we're interested in minimizing the size of the generated code, 
>> delaying computation until we can show it being used.
>>
>> Now, ppc is a bit different from other targets (which might compute overflow 
>> for any addition insn) in that it only computes overflow when someone asks 
>> for 
>> it.  Moreover, it's fairly rare for the addo/subo/nego instructions to
>> be used.
>
>> Therefore, I'm not 100% sure what the "best" solution is.
>
> Agreed, with that logic, wont it be more efficient to move the OV/CA
> updationg to respective callers, and when xer_read/write happens, its
> just one tcg_ops.

BTW, i haven't seen remarkable difference in the boot time after this
change.

Regards
Nikunj

Re: [Qemu-devel] [PATCH v2] intel_iommu: check misordered init when realize

[Peter Xu]

On Fri, Feb 24, 2017 at 01:35:10AM -0500, Pankaj Gupta wrote:
> 
> > 
> > On Fri, Feb 24, 2017 at 12:07:33AM -0500, Pankaj Gupta wrote:
> > > Hello Peter,
> > 
> > Hi, Pankaj!
> > 
> > > 
> > > This solution looks to check dependency of 'vfio-pci' over 'intel-iommu'
> > > before 'intel-iommu' is not initialized.
> > > 
> > > Overall it looks good to me, just a small nit below.
> > >  
> > > > 
> > > > Intel vIOMMU devices are created with "-device" parameter, while here
> > > > actually we need to make sure the dmar device be created before other
> > > > PCI devices (like vfio-pci) so that we know iommu_fn will be setup
> > > > correctly before realizations of those PCI devices (it is sensible that
> > > > PCI device fetch these info during its realization). Now this ordering
> > > > yet cannot be achieved elsewhere, and devices will be created in the
> > > > order that user specified. That might be dangerous.
> > > > 
> > > > Here we add one more function to detect this kind of misordering issue,
> > > > then report to guest. Currently, the only known device that is affected
> > > > by this VT-d defect is the vfio-pci typed devices. So for now we just
> > > > check against it to make sure we are safe.
> > > > 
> > > > Signed-off-by: Peter Xu 
> > > > ---
> > > >  hw/i386/intel_iommu.c | 22 ++
> > > >  1 file changed, 22 insertions(+)
> > > > 
> > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > index 22d8226..b723ece 100644
> > > > --- a/hw/i386/intel_iommu.c
> > > > +++ b/hw/i386/intel_iommu.c
> > > > @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> > > > Error **errp)
> > > >  return true;
> > > >  }
> > > >  
> > > > +/*
> > > > + * TODO: we should have a better way to achieve the ordering rather
> > > > + * than this misorder check explicitly against vfio-pci. After all, no
> > > > + * one should be blamed for this, and vfio-pci did nothing wrong.
> > > > + */
> > > > +static bool vtd_detected_misorder_init(Error **errp)
> > > > +{
> > > > +Object *dev = object_resolve_path_type("", "vfio-pci", NULL);
> > > > +
> > > > +if (dev) {
> > > > +error_setg(errp, "Please specify \"intel-iommu\" before all the
> > > > rest
> > > 
> > > "before all the rest" does not give much clue to user. Do you
> > > think better
> > > error message would help? just a thought.
> > 
> > Yes this is my intention to emphasize that we should possibly put
> > intel-iommu even before all the PCI devices. As mentioned above (and
> > also in the commit message), although vfio-pci is the only one that is
> > affected by this, we should probably put intel-iommu even before all
> > the rest of PCI devices. E.g., in the future we can have new devices
> > that need this dependency as well (that vt-d being inited before that
> > device), which is still legal imho.
> 
> yes, something like this can help "intel-iommu should be specified as first 
> device"?

I can switch this description if you prefer this one. :) Not sure
whether this can be touched up during merge if this patch is good for
2.9, or I'll just repost with yours if there is further comments.

> Or we can just check for "intel-iommu" as first device at the start in place 
> of
> checking for "vfio-pci". This will also help us to check for all other 
> devices. 

Actually your suggestion is just what I did in version 1. The problem
is that, it's not easy to let vt-d really be the first device to be
inited... Please check pc_q35_init(), within that we have:

pc_vga_init(isa_bus, host_bus);
pc_nic_init(isa_bus, host_bus);

These are integrated devices along with ICH9. Such devices are
possibly pci devices as well, but they are created much earlier than
vt-d device, since that's still during machine init.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v4 06/15] target/ppc: remove xer split-out flags(so, ov, ca)

[Nikunj A Dadhania]

Richard Henderson  writes:

> On 02/24/2017 01:58 PM, David Gibson wrote:
>> On Fri, Feb 24, 2017 at 06:18:22AM +0530, Nikunj A Dadhania wrote:
>>> Richard Henderson  writes:
>>>
 On 02/24/2017 06:56 AM, Nikunj A Dadhania wrote:
> Now get rid all the split out variables so, ca, ov. After this patch,
> all the bits are stored in CPUPPCState::xer at appropriate places.
>
> Signed-off-by: Nikunj A Dadhania 
> ---
>  target/ppc/cpu.c|   8 +---
>  target/ppc/cpu.h|  26 ++--
>  target/ppc/int_helper.c |  12 +++---
>  target/ppc/translate.c  | 106 
> +---
>  4 files changed, 78 insertions(+), 74 deletions(-)

 I do not think this is a good direction to take this.
>>>
>>> Hmm, any particular reason?
>>
>> Right, I suggested this, but based only a suspicion that the split
>> variables weren't worth the complexity.  I'm happy to be corrected by
>> someone with better knowledge of TCG, but it'd be nice to know why.
>
> Normally we're interested in minimizing the size of the generated code, 
> delaying computation until we can show it being used.
>
> Now, ppc is a bit different from other targets (which might compute overflow 
> for any addition insn) in that it only computes overflow when someone asks 
> for 
> it.  Moreover, it's fairly rare for the addo/subo/nego instructions to
> be used.

> Therefore, I'm not 100% sure what the "best" solution is.

Agreed, with that logic, wont it be more efficient to move the OV/CA
updationg to respective callers, and when xer_read/write happens, its
just one tcg_ops.


> However, I'd be surprised if the least amount of code places all of
> the bits into their canonical location within XER.
>
> Do note that when looking at this, the various methods by which the OV/SO 
> bits 
> are copied to CR flags ought to be taken into account.

I lost you in the last two para, can you explain in detail?

Regards
Nikunj

Re: [Qemu-devel] [PATCH v2] mttcg/i386: Patch instruction using async_safe_* framework

[Richard Henderson]

On 02/24/2017 04:42 PM, Pranith Kumar wrote:

In mttcg, calling pause_all_vcpus() during execution from the
generated TBs causes a deadlock if some vCPU is waiting for exclusive
execution in start_exclusive(). Fix this by using the aync_safe_*
framework instead of pausing vcpus for patching instructions.

CC: Richard Henderson 
CC: Peter Maydell 
CC: Alex Bennée 
Signed-off-by: Pranith Kumar 
---
 hw/i386/kvmvapic.c | 82 ++
 1 file changed, 52 insertions(+), 30 deletions(-)


Reviewed-by: Richard Henderson 


r~

Re: [Qemu-devel] [PATCH v4 06/15] target/ppc: remove xer split-out flags(so, ov, ca)

[Richard Henderson]

On 02/24/2017 01:58 PM, David Gibson wrote:

On Fri, Feb 24, 2017 at 06:18:22AM +0530, Nikunj A Dadhania wrote:

Richard Henderson  writes:


On 02/24/2017 06:56 AM, Nikunj A Dadhania wrote:

Now get rid all the split out variables so, ca, ov. After this patch,
all the bits are stored in CPUPPCState::xer at appropriate places.

Signed-off-by: Nikunj A Dadhania 
---
 target/ppc/cpu.c|   8 +---
 target/ppc/cpu.h|  26 ++--
 target/ppc/int_helper.c |  12 +++---
 target/ppc/translate.c  | 106 +---
 4 files changed, 78 insertions(+), 74 deletions(-)


I do not think this is a good direction to take this.


Hmm, any particular reason?


Right, I suggested this, but based only a suspicion that the split
variables weren't worth the complexity.  I'm happy to be corrected by
someone with better knowledge of TCG, but it'd be nice to know why.


Normally we're interested in minimizing the size of the generated code, 
delaying computation until we can show it being used.


Now, ppc is a bit different from other targets (which might compute overflow 
for any addition insn) in that it only computes overflow when someone asks for 
it.  Moreover, it's fairly rare for the addo/subo/nego instructions to be used.


Therefore, I'm not 100% sure what the "best" solution is.  However, I'd be 
surprised if the least amount of code places all of the bits into their 
canonical location within XER.


Do note that when looking at this, the various methods by which the OV/SO bits 
are copied to CR flags ought to be taken into account.



r~

Re: [Qemu-devel] [PATCH v2] intel_iommu: check misordered init when realize

[Pankaj Gupta]

> 
> On Fri, Feb 24, 2017 at 12:07:33AM -0500, Pankaj Gupta wrote:
> > Hello Peter,
> 
> Hi, Pankaj!
> 
> > 
> > This solution looks to check dependency of 'vfio-pci' over 'intel-iommu'
> > before 'intel-iommu' is not initialized.
> > 
> > Overall it looks good to me, just a small nit below.
> >  
> > > 
> > > Intel vIOMMU devices are created with "-device" parameter, while here
> > > actually we need to make sure the dmar device be created before other
> > > PCI devices (like vfio-pci) so that we know iommu_fn will be setup
> > > correctly before realizations of those PCI devices (it is sensible that
> > > PCI device fetch these info during its realization). Now this ordering
> > > yet cannot be achieved elsewhere, and devices will be created in the
> > > order that user specified. That might be dangerous.
> > > 
> > > Here we add one more function to detect this kind of misordering issue,
> > > then report to guest. Currently, the only known device that is affected
> > > by this VT-d defect is the vfio-pci typed devices. So for now we just
> > > check against it to make sure we are safe.
> > > 
> > > Signed-off-by: Peter Xu 
> > > ---
> > >  hw/i386/intel_iommu.c | 22 ++
> > >  1 file changed, 22 insertions(+)
> > > 
> > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > index 22d8226..b723ece 100644
> > > --- a/hw/i386/intel_iommu.c
> > > +++ b/hw/i386/intel_iommu.c
> > > @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> > > Error **errp)
> > >  return true;
> > >  }
> > >  
> > > +/*
> > > + * TODO: we should have a better way to achieve the ordering rather
> > > + * than this misorder check explicitly against vfio-pci. After all, no
> > > + * one should be blamed for this, and vfio-pci did nothing wrong.
> > > + */
> > > +static bool vtd_detected_misorder_init(Error **errp)
> > > +{
> > > +Object *dev = object_resolve_path_type("", "vfio-pci", NULL);
> > > +
> > > +if (dev) {
> > > +error_setg(errp, "Please specify \"intel-iommu\" before all the
> > > rest
> > 
> > "before all the rest" does not give much clue to user. Do you
> > think better
> > error message would help? just a thought.
> 
> Yes this is my intention to emphasize that we should possibly put
> intel-iommu even before all the PCI devices. As mentioned above (and
> also in the commit message), although vfio-pci is the only one that is
> affected by this, we should probably put intel-iommu even before all
> the rest of PCI devices. E.g., in the future we can have new devices
> that need this dependency as well (that vt-d being inited before that
> device), which is still legal imho.

yes, something like this can help "intel-iommu should be specified as first 
device"?
Or we can just check for "intel-iommu" as first device at the start in place of
checking for "vfio-pci". This will also help us to check for all other devices. 

Thanks. 

> 
> Meanwhile, I intended to avoid naming "vfio-pci" here since it'll let
> user think of "something bad with vfio-pci" but actually vfio-pci did
> nothing wrong.

o.k 
> 
> Thanks,
> 
> -- peterx
> 

[Qemu-devel] [PULL 4/5] MAINTAINERS: merge Build and test automation with Docker tests

[Fam Zheng]

From: Alex Bennée 

The docker framework is really just another piece in the build
automation puzzle so lets merge it together. For added bonus I've also
included the Travis and Patchew status links. The Shippable links will
be added later once mainline tests have been configured and setup.

Signed-off-by: Alex Bennée 
Reviewed-by: Fam Zheng 
Message-Id: <20170220105139.21581-5-alex.ben...@linaro.org>
Signed-off-by: Fam Zheng 
---
 MAINTAINERS | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6740467..be79f68 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1800,10 +1800,14 @@ F: docs/block-replication.txt
 Build and test automation
 -
 M: Alex Bennée 
+M: Fam Zheng 
 L: qemu-devel@nongnu.org
-S: Supported
+S: Maintained
 F: .travis.yml
 F: .shippable.yml
+F: tests/docker/
+W: https://travis-ci.org/qemu/qemu
+W: http://patchew.org/QEMU/
 
 Documentation
 -
@@ -1812,9 +1816,3 @@ M: Daniel P. Berrange 
 S: Odd Fixes
 F: docs/build-system.txt
 
-Docker testing
---
-Docker based testing framework and cases
-M: Fam Zheng 
-S: Maintained
-F: tests/docker/
-- 
2.9.3

[Qemu-devel] [PULL 5/5] docker: Install python2 explicitly in docker image

[Fam Zheng]

Python is no longer installed implicitly, but the QEMU build system
requires it. List it in PACKAGES.

Reported-by: Auger Eric 
Signed-off-by: Fam Zheng 
Message-Id: <20170222021801.28658-1-f...@redhat.com>
Tested-by: Eric Auger 
Signed-off-by: Fam Zheng 
---
 tests/docker/dockerfiles/fedora.docker | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/docker/dockerfiles/fedora.docker 
b/tests/docker/dockerfiles/fedora.docker
index 478163b..c4f80ad 100644
--- a/tests/docker/dockerfiles/fedora.docker
+++ b/tests/docker/dockerfiles/fedora.docker
@@ -1,6 +1,6 @@
 FROM fedora:latest
 ENV PACKAGES \
-ccache git tar PyYAML sparse flex bison \
+ccache git tar PyYAML sparse flex bison python2 \
 glib2-devel pixman-devel zlib-devel SDL-devel libfdt-devel \
 gcc gcc-c++ clang make perl which bc findutils \
 mingw32-pixman mingw32-glib2 mingw32-gmp mingw32-SDL mingw32-pkg-config \
-- 
2.9.3

[Qemu-devel] [PULL 3/5] .shippable.yml: new CI provider

[Fam Zheng]

From: Alex Bennée 

Ostensibly Shippable offers a similar set of services as Travis.
However they are focused on Docker container based work-flows so we
can use our existing containers to run a few extra builds - in this
case a bunch of cross-compiled targets on a Debian multiarch system.

Signed-off-by: Alex Bennée 
Reviewed-by: Fam Zheng 
Message-Id: <20170220105139.21581-4-alex.ben...@linaro.org>
Signed-off-by: Fam Zheng 
---
 .shippable.yml | 19 +++
 MAINTAINERS|  1 +
 2 files changed, 20 insertions(+)
 create mode 100644 .shippable.yml

diff --git a/.shippable.yml b/.shippable.yml
new file mode 100644
index 000..1a1fd7a
--- /dev/null
+++ b/.shippable.yml
@@ -0,0 +1,19 @@
+language: c
+env:
+  matrix:
+- IMAGE=debian-armhf-cross
+  TARGET_LIST=arm-softmmu,arm-linux-user
+- IMAGE=debian-arm64-cross
+  TARGET_LIST=aarch64-softmmu,aarch64-linux-user
+build:
+  pre_ci:
+- make docker-image-${IMAGE}
+  pre_ci_boot:
+image_name: qemu
+image_tag: ${IMAGE}
+pull: false
+options: "-e HOME=/root"
+  ci:
+- unset CC
+- ./configure ${QEMU_CONFIGURE_OPTS} --target-list=${TARGET_LIST}
+- make -j2
diff --git a/MAINTAINERS b/MAINTAINERS
index 4714df8..6740467 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1803,6 +1803,7 @@ M: Alex Bennée 
 L: qemu-devel@nongnu.org
 S: Supported
 F: .travis.yml
+F: .shippable.yml
 
 Documentation
 -
-- 
2.9.3

[Qemu-devel] [PULL 0/5] Docker testing and shippable patches

[Fam Zheng]

The following changes since commit 10f25e4844cb9b3f02fb032f88051dd5b65b4206:

  Merge remote-tracking branch 'remotes/yongbok/tags/mips-20170222' into 
staging (2017-02-23 09:59:40 +)

are available in the git repository at:

  git://github.com/famz/qemu.git tags/for-upstream

for you to fetch changes up to a8f159d45bcb78c00ea160bfc2b94512e4ed9910:

  docker: Install python2 explicitly in docker image (2017-02-24 14:18:11 +0800)



Hi Peter,

These are testing and build automation patches:

- Shippable.com powered CI config
- Docker cross build
- Fixes and MAINTAINERS tweaks.



Alex Bennée (4):
  tests/docker: add basic user mapping support
  new: debian docker targets for cross-compiling
  .shippable.yml: new CI provider
  MAINTAINERS: merge Build and test automation with Docker tests

Fam Zheng (1):
  docker: Install python2 explicitly in docker image

 .shippable.yml | 19 
 MAINTAINERS| 13 ++-
 tests/docker/Makefile.include  |  6 ++
 tests/docker/common.rc |  2 +-
 tests/docker/docker.py | 16 --
 tests/docker/dockerfiles/debian-arm64-cross.docker | 15 +
 tests/docker/dockerfiles/debian-armhf-cross.docker | 15 +
 tests/docker/dockerfiles/debian.docker | 25 ++
 tests/docker/dockerfiles/fedora.docker |  2 +-
 9 files changed, 102 insertions(+), 11 deletions(-)
 create mode 100644 .shippable.yml
 create mode 100644 tests/docker/dockerfiles/debian-arm64-cross.docker
 create mode 100644 tests/docker/dockerfiles/debian-armhf-cross.docker
 create mode 100644 tests/docker/dockerfiles/debian.docker

-- 
2.9.3

[Qemu-devel] [PULL 2/5] new: debian docker targets for cross-compiling

[Fam Zheng]

From: Alex Bennée 

This provides a basic Debian install with access to the emdebian cross
compilers. The debian-armhf-cross and debian-arm64-cross targets build
on the basic Debian image to allow cross compiling to those targets.

A new environment variable (QEMU_CONFIGURE_OPTS) is set as part of the
docker container and passed to the build to specify the
--cross-prefix. The user still calls the build in the usual way, for
example:

  make docker-test-build@debian-arm64-cross \
TARGET_LIST="aarch64-softmmu,aarch64-linux-user"

Signed-off-by: Alex Bennée 
Reviewed-by: Fam Zheng 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20170220105139.21581-3-alex.ben...@linaro.org>
Signed-off-by: Fam Zheng 
---
 tests/docker/Makefile.include  |  4 
 tests/docker/common.rc |  2 +-
 tests/docker/dockerfiles/debian-arm64-cross.docker | 15 +
 tests/docker/dockerfiles/debian-armhf-cross.docker | 15 +
 tests/docker/dockerfiles/debian.docker | 25 ++
 5 files changed, 60 insertions(+), 1 deletion(-)
 create mode 100644 tests/docker/dockerfiles/debian-arm64-cross.docker
 create mode 100644 tests/docker/dockerfiles/debian-armhf-cross.docker
 create mode 100644 tests/docker/dockerfiles/debian.docker

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 3b5ffec..03eda37 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -54,6 +54,10 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
$(if $(EXECUTABLE),--include-executable=$(EXECUTABLE)),\
"BUILD","$*")
 
+# Enforce dependancies for composite images
+docker-image-debian-armhf-cross: docker-image-debian
+docker-image-debian-arm64-cross: docker-image-debian
+
 # Expand all the pre-requistes for each docker image and test combination
 $(foreach i,$(DOCKER_IMAGES), \
$(foreach t,$(DOCKER_TESTS) $(DOCKER_TOOLS), \
diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index 21657e8..6865689 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -29,7 +29,7 @@ build_qemu()
 config_opts="--enable-werror \
  ${TARGET_LIST:+--target-list=${TARGET_LIST}} \
  --prefix=$PWD/install \
- $EXTRA_CONFIGURE_OPTS \
+ $QEMU_CONFIGURE_OPTS $EXTRA_CONFIGURE_OPTS \
  $@"
 echo "Configure options:"
 echo $config_opts
diff --git a/tests/docker/dockerfiles/debian-arm64-cross.docker 
b/tests/docker/dockerfiles/debian-arm64-cross.docker
new file mode 100644
index 000..592b5d7
--- /dev/null
+++ b/tests/docker/dockerfiles/debian-arm64-cross.docker
@@ -0,0 +1,15 @@
+#
+# Docker arm64 cross-compiler target
+#
+# This docker target builds on the base debian image.
+#
+FROM qemu:debian
+
+# Add the foreign architecture we want and install dependencies
+RUN dpkg --add-architecture arm64
+RUN apt update
+RUN apt install -yy crossbuild-essential-arm64
+RUN apt-get build-dep -yy -a arm64 qemu
+
+# Specify the cross prefix for this image (see tests/docker/common.rc)
+ENV QEMU_CONFIGURE_OPTS --cross-prefix=aarch64-linux-gnu-
diff --git a/tests/docker/dockerfiles/debian-armhf-cross.docker 
b/tests/docker/dockerfiles/debian-armhf-cross.docker
new file mode 100644
index 000..668d60a
--- /dev/null
+++ b/tests/docker/dockerfiles/debian-armhf-cross.docker
@@ -0,0 +1,15 @@
+#
+# Docker armhf cross-compiler target
+#
+# This docker target builds on the base debian image.
+#
+FROM qemu:debian
+
+# Add the foreign architecture we want and install dependencies
+RUN dpkg --add-architecture armhf
+RUN apt update
+RUN apt install -yy crossbuild-essential-armhf
+RUN apt-get build-dep -yy -a armhf qemu
+
+# Specify the cross prefix for this image (see tests/docker/common.rc)
+ENV QEMU_CONFIGURE_OPTS --cross-prefix=arm-linux-gnueabihf-
diff --git a/tests/docker/dockerfiles/debian.docker 
b/tests/docker/dockerfiles/debian.docker
new file mode 100644
index 000..52bd799
--- /dev/null
+++ b/tests/docker/dockerfiles/debian.docker
@@ -0,0 +1,25 @@
+#
+# Docker multiarch cross-compiler target
+#
+# This docker target is builds on Debian and Emdebian's cross compiler targets
+# to build distro with a selection of cross compilers for building test 
binaries.
+#
+# On its own you can't build much but the docker-foo-cross targets
+# build on top of the base debian image.
+#
+FROM debian:stable-slim
+
+# Setup some basic tools we need
+RUN apt update
+RUN apt install -yy curl aptitude
+
+# Setup Emdebian
+RUN echo "deb http://emdebian.org/tools/debian/ jessie main" >> 
/etc/apt/sources.list
+RUN curl http://emdebian.org/tools/debian/emdebian-toolchain-archive.key | 
apt-key add -
+
+# Duplicate deb line as deb-src
+RUN cat /etc/apt/sources.list | sed "s/deb/deb-src/" >> /etc/apt/sources.list
+
+# Install common build 

[Qemu-devel] [PULL 1/5] tests/docker: add basic user mapping support

[Fam Zheng]

From: Alex Bennée 

Currently all docker builds are done by exporting a tarball to the
docker container and running the build as the containers root user.
Other use cases are possible however and it is possible to map a part
of users file-system to the container. This is useful for example for
doing cross-builds of arbitrary source trees. For this to work
smoothly the container needs to have a user created that maps cleanly
to the host system.

This adds a -u option to the docker script so that:

  DEB_ARCH=armhf DEB_TYPE=stable ./tests/docker/docker.py build \
-u --include-executable=arm-linux-user/qemu-arm \
debian:armhf ./tests/docker/dockerfiles/debian-bootstrap.docker

Will build a container that can then be run like:

  docker run --rm -it -v /home/alex/lsrc/qemu/risu.git/:/src \
--user=alex:alex -w /src/ debian:armhf \
sh -c "make clean && ./configure -s && make"

All docker containers built will add the current user unless
explicitly disabled by specifying NOUSER when invoking the Makefile:

  make docker-image-debian-armhf-cross NOUSER=1

Signed-off-by: Alex Bennée 
Reviewed-by: Fam Zheng 
Tested-by: Philippe Mathieu-Daudé 
Reviewed-by: Philippe Mathieu-Daudé 
Message-Id: <20170220105139.21581-2-alex.ben...@linaro.org>
Signed-off-by: Fam Zheng 
---
 tests/docker/Makefile.include |  2 ++
 tests/docker/docker.py| 16 ++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 3f15d5a..3b5ffec 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -50,6 +50,7 @@ docker-image-%: $(DOCKER_FILES_DIR)/%.docker
$(call quiet-command,\
$(SRC_PATH)/tests/docker/docker.py build qemu:$* $< \
$(if $V,,--quiet) $(if $(NOCACHE),--no-cache) \
+   $(if $(NOUSER),,--add-current-user) \
$(if $(EXECUTABLE),--include-executable=$(EXECUTABLE)),\
"BUILD","$*")
 
@@ -99,6 +100,7 @@ docker:
@echo ' (default is 1)'
@echo 'DEBUG=1  Stop and drop to shell in the created 
container'
@echo ' before running the command.'
+   @echo 'NOUSER   Define to disable adding current user 
to containers passwd.'
@echo 'NOCACHE=1Ignore cache when build images.'
@echo 'EXECUTABLE=Include executable in image.'
 
diff --git a/tests/docker/docker.py b/tests/docker/docker.py
index 37d8319..9fd32ab 100755
--- a/tests/docker/docker.py
+++ b/tests/docker/docker.py
@@ -25,6 +25,7 @@ import signal
 from tarfile import TarFile, TarInfo
 from StringIO import StringIO
 from shutil import copy, rmtree
+from pwd import getpwuid
 
 
 DEVNULL = open(os.devnull, 'wb')
@@ -149,13 +150,21 @@ class Docker(object):
 labels = json.loads(resp)[0]["Config"].get("Labels", {})
 return labels.get("com.qemu.dockerfile-checksum", "")
 
-def build_image(self, tag, docker_dir, dockerfile, quiet=True, argv=None):
+def build_image(self, tag, docker_dir, dockerfile,
+quiet=True, user=False, argv=None):
 if argv == None:
 argv = []
 
 tmp_df = tempfile.NamedTemporaryFile(dir=docker_dir, suffix=".docker")
 tmp_df.write(dockerfile)
 
+if user:
+uid = os.getuid()
+uname = getpwuid(uid).pw_name
+tmp_df.write("\n")
+tmp_df.write("RUN id %s 2>/dev/null || useradd -u %d -U %s" %
+ (uname, uid, uname))
+
 tmp_df.write("\n")
 tmp_df.write("LABEL com.qemu.dockerfile-checksum=%s" %
  _text_checksum(dockerfile))
@@ -225,6 +234,9 @@ class BuildCommand(SubCommand):
 help="""Specify a binary that will be copied to the
 container together with all its dependent
 libraries""")
+parser.add_argument("--add-current-user", "-u", dest="user",
+action="store_true",
+help="Add the current user to image's passwd")
 parser.add_argument("tag",
 help="Image Tag")
 parser.add_argument("dockerfile",
@@ -261,7 +273,7 @@ class BuildCommand(SubCommand):
docker_dir)
 
 dkr.build_image(tag, docker_dir, dockerfile,
-quiet=args.quiet, argv=argv)
+quiet=args.quiet, user=args.user, argv=argv)
 
 rmtree(docker_dir)
 
-- 
2.9.3

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[Richard Henderson]

On 02/24/2017 03:50 PM, David Gibson wrote:

Although I guess they'd shrink right down again if we put an
env->xer_mask in.  Thoughts on that option Richard?


Why would xer_mask shrink the code?  I can't see that we'd be able to eliminate 
any code using the mask.



r~

Re: [Qemu-devel] [PATCH] docker: Install python2 explicitly in docker image

[Fam Zheng]

On Wed, 02/22 10:18, Fam Zheng wrote:
> Python is no longer installed implicitly, but the QEMU build system
> requires it. List it in PACKAGES.
> 
> Reported-by: Auger Eric 
> Signed-off-by: Fam Zheng 
> ---
>  tests/docker/dockerfiles/fedora.docker | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/docker/dockerfiles/fedora.docker 
> b/tests/docker/dockerfiles/fedora.docker
> index 478163b..c4f80ad 100644
> --- a/tests/docker/dockerfiles/fedora.docker
> +++ b/tests/docker/dockerfiles/fedora.docker
> @@ -1,6 +1,6 @@
>  FROM fedora:latest
>  ENV PACKAGES \
> -ccache git tar PyYAML sparse flex bison \
> +ccache git tar PyYAML sparse flex bison python2 \
>  glib2-devel pixman-devel zlib-devel SDL-devel libfdt-devel \
>  gcc gcc-c++ clang make perl which bc findutils \
>  mingw32-pixman mingw32-glib2 mingw32-gmp mingw32-SDL mingw32-pkg-config \
> -- 
> 2.9.3
> 

Applied to my staging tree.

Fam

Re: [Qemu-devel] [PATCH 03/21] qmp-test: New, covering basic QMP protocol

[Markus Armbruster]

Eric Blake  writes:

> On 02/23/2017 03:44 PM, Markus Armbruster wrote:
>> Signed-off-by: Markus Armbruster 
>> ---
>>  tests/Makefile.include |   5 +-
>>  tests/libqtest.c   |  17 --
>>  tests/libqtest.h   |   8 +++
>>  tests/qmp-test.c   | 139 
>> +
>>  4 files changed, 163 insertions(+), 6 deletions(-)
>>  create mode 100644 tests/qmp-test.c
>> 
>
>> +++ b/tests/libqtest.h
>> @@ -32,6 +32,14 @@ extern QTestState *global_qtest;
>>  QTestState *qtest_init(const char *extra_args);
>>  
>>  /**
>> + * qtest_init:
>
> Wrong name (too much copy-and-paste)

One of the several reasons why I detest this comment style.  I'll fix
it, of course.

>> + * @extra_args: other arguments to pass to QEMU.
>> + *
>> + * Returns: #QTestState instance.
>> + */
>> +QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
>> +
>> +/**
>>   * qtest_quit:
>>   * @s: #QTestState instance to operate on.
>>   *
>> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
>> new file mode 100644
>> index 000..405e49e
>> --- /dev/null
>> +++ b/tests/qmp-test.c
>
>> +
>> +static void test_malformed(void)
>> +{
>> +QDict *resp;
>> +
>> +/* Not even a dictionary */
>> +resp = qmp("null");
>> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +QDECREF(resp);
>> +
>
> Shall we test an array, integer, and/or boolean literal as well?

Not necessary for code coverage.  Wouldn't hurt, of course.

>> +/* No "execute" key */
>> +resp = qmp("{}");
>> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +QDECREF(resp);
>> +
>> +/* "execute" isn't a string */
>> +resp = qmp("{ 'execute': true }");
>> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +QDECREF(resp);
>> +
>> +/* "arguments" isn't a dictionary */
>> +resp = qmp("{ 'execute': 'no-such-cmd', 'arguments': [] }");
>> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +QDECREF(resp);
>> +
>> +/* extra key */
>> +resp = qmp("{ 'execute': 'no-such-cmd', 'extra': true }");
>> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +QDECREF(resp);
>
> Worth testing "{ 'arguments': {} }"?

Likewise.

>> +}
>> +
>> +static void test_qmp_protocol(void)
>> +{
>> +QDict *resp, *q, *ret;
>> +QList *capabilities;
>> +
>> +global_qtest = qtest_init_without_qmp_handshake(common_args);
>> +
>> +/* Test greeting */
>> +resp = qmp_receive();
>> +q = qdict_get_qdict(resp, "QMP");
>> +g_assert(q);
>> +test_version(qdict_get(q, "version"));
>> +capabilities = qdict_get_qlist(q, "capabilities");
>> +g_assert(capabilities && qlist_empty(capabilities));
>> +QDECREF(resp);
>> +
>> +/* Test valid command before handshake */
>> +resp = qmp("{ 'execute': 'query-version' }");
>> +g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
>> +QDECREF(resp);
>> +
>> +/* Test malformed commands before handshake */
>> +test_malformed();
>> +
>> +/* Test handshake */
>> +resp = qmp("{ 'execute': 'qmp_capabilities' }");
>> +ret = qdict_get_qdict(resp, "return");
>> +g_assert(ret && !qdict_size(ret));
>> +QDECREF(resp);
>> +
>> +/* Test repeated handshake */
>> +resp = qmp("{ 'execute': 'qmp_capabilities' }");
>> +g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
>> +QDECREF(resp);
>> +
>> +/* Test valid command */
>> +resp = qmp("{ 'execute': 'query-version' }");
>> +test_version(qdict_get(resp, "return"));
>> +QDECREF(resp);
>> +
>> +/* Test malformed commands */
>> +test_malformed();
>> +
>> +/* Test 'id' */
>> +resp = qmp("{ 'execute': 'query-name', 'id': 'cookie#1' }");
>> +ret = qdict_get_qdict(resp, "return");
>> +g_assert(ret);
>> +g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, "cookie#1");
>> +QDECREF(resp);
>> +
>> +/* Test command failure with 'id' */
>> +resp = qmp("{ 'execute': 'human-monitor-command', 'id': 2 }");
>> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
>> +g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
>> +QDECREF(resp);
>
> id can be _any JSON_ (at one point, QMP crashed if id included a null
> literal); so maybe we should have more id tests such as "'id': null,",
> "'id': [ { }, true ]"
>
> But any tests at all is better than our previous state of none, so even
> if you don't add tests, but just fix the typo:
>
> Reviewed-by: Eric Blake 

I'm in a bit of a time squeeze, so I'll take your offer to just fix the
typo for now.

Thanks!

Re: [Qemu-devel] [RFC v6] RBD: Add support readv,writev for rbd

[Jaze Lee]

2017-02-24 11:52 GMT+08:00 Jeff Cody :
> On Tue, Feb 21, 2017 at 02:50:03PM +0800, jaze...@gmail.com wrote:
>> From: tianqing 
>>
>> Rbd can do readv and writev directly, so wo do not need to transform
>> iov to buf or vice versa any more.
>>
>> Signed-off-by: tianqing 
>> ---
>
>
> This is marked as an RFC still - is this a series you would like to see in
> 2.9?

Yes.   What should i do if i like it in 2.9

-- 
谦谦君子

Re: [Qemu-devel] [PATCH 01/21] qga: Fix crash on non-dictionary QMP argument

[Markus Armbruster]

Eric Blake  writes:

> On 02/23/2017 04:46 PM, Eric Blake wrote:
>> On 02/23/2017 03:44 PM, Markus Armbruster wrote:
>>> The value of key 'arguments' must be a JSON object.  qemu-ga neglects
>>> to check, and crashes.  To reproduce, send
>>>
>>> { 'execute': 'guest-sync', 'arguments': [] }
>>>
>>> to qemu-ga.
>>>
>>> do_qmp_dispatch() uses qdict_get_qdict() to get the arguments.  When
>>> not a JSON object, this gets a null pointer, which flows through the
>>> generated marshalling function to qobject_input_visitor_new(), where
>>> it fails the assertion.  qmp_dispatch_check_obj() needs to catch this
>>> error.
>>>
>>> QEMU isn't affected, because it runs qmp_check_input_obj() first,
>>> which basically duplicates qmp_check_input_obj()'s checks, plus the
>
> This sentence is weird (func A can't duplicate func A's checks; you're
> missing a func B, but I'm not sure which instance is wrong, nor what B
> should be).

B is qmp_dispatch_check_obj().  I'll fix it.

>>> missing one.
>>>
>>> Fix by copying the missing one from qmp_check_input_obj() to
>>> qmp_dispatch_check_obj().
>>>
>>> Signed-off-by: Markus Armbruster 
>>> Cc: Michael Roth 
>>> ---
>>>  qapi/qmp-dispatch.c | 8 +++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> Reviewed-by: Eric Blake 

Thanks!

[Qemu-devel] [PATCH v5 5/8] target/ppc: use tcg ops for neg instruction

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania 
Reviewed-by: Richard Henderson 
---
 target/ppc/translate.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 16f422f..d4d9941 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1483,7 +1483,10 @@ static inline void gen_op_arith_neg(DisasContext *ctx, 
bool compute_ov)
 
 static void gen_neg(DisasContext *ctx)
 {
-gen_op_arith_neg(ctx, 0);
+tcg_gen_neg_tl(cpu_gpr[rD(ctx->opcode)], cpu_gpr[rA(ctx->opcode)]);
+if (unlikely(Rc(ctx->opcode))) {
+gen_set_Rc0(ctx, cpu_gpr[rD(ctx->opcode)]);
+}
 }
 
 static void gen_nego(DisasContext *ctx)
-- 
2.7.4

[Qemu-devel] [PATCH v5 3/8] target/ppc: update ca32 in arithmetic substract

[Nikunj A Dadhania]

Signed-off-by: Nikunj A Dadhania 
Reviewed-by: Richard Henderson 
---
 target/ppc/translate.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index eba83ef..e083082 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -827,7 +827,11 @@ static inline void gen_op_arith_compute_ca32(DisasContext 
*ctx,
 }
 
 t0 = tcg_temp_new();
-tcg_gen_xor_tl(t0, arg0, arg1);
+if (sub) {
+tcg_gen_eqv_tl(t0, arg0, arg1);
+} else {
+tcg_gen_xor_tl(t0, arg0, arg1);
+}
 tcg_gen_xor_tl(t0, t0, res);
 tcg_gen_extract_tl(cpu_ca32, t0, 32, 1);
 tcg_temp_free(t0);
@@ -1378,17 +1382,22 @@ static inline void gen_op_arith_subf(DisasContext *ctx, 
TCGv ret, TCGv arg1,
 tcg_temp_free(t1);
 tcg_gen_shri_tl(cpu_ca, cpu_ca, 32);/* extract bit 32 */
 tcg_gen_andi_tl(cpu_ca, cpu_ca, 1);
+if (is_isa300(ctx)) {
+tcg_gen_mov_tl(cpu_ca32, cpu_ca);
+}
 } else if (add_ca) {
 TCGv zero, inv1 = tcg_temp_new();
 tcg_gen_not_tl(inv1, arg1);
 zero = tcg_const_tl(0);
 tcg_gen_add2_tl(t0, cpu_ca, arg2, zero, cpu_ca, zero);
 tcg_gen_add2_tl(t0, cpu_ca, t0, cpu_ca, inv1, zero);
+gen_op_arith_compute_ca32(ctx, t0, inv1, arg2, 0);
 tcg_temp_free(zero);
 tcg_temp_free(inv1);
 } else {
 tcg_gen_setcond_tl(TCG_COND_GEU, cpu_ca, arg2, arg1);
 tcg_gen_sub_tl(t0, arg2, arg1);
+gen_op_arith_compute_ca32(ctx, t0, arg1, arg2, 1);
 }
 } else if (add_ca) {
 /* Since we're ignoring carry-out, we can simplify the
-- 
2.7.4

Re: [Qemu-devel] [PATCH v4 00/15] POWER9 TCG enablements - part15

[Nikunj A Dadhania]

David Gibson  writes:

> [ Unknown signature status ]
> On Fri, Feb 24, 2017 at 01:26:25AM +0530, Nikunj A Dadhania wrote:
>> Patches:
>> 01-06  Cleans up the XER split out variables and now the 
>>flag bits are stored in XER at their respective places 
>> 
>> 07-14  Contains implentation of CA32 and OV32 bits added to the 
>>ISA 3.0. Various fixed-point arithmetic instructions are 
>>updated to take care of the newer flags.
>>  
>> 15 Finally the last patch adds new instruction mcrxrx, that helps
>>reading the carry (CA and CA32) and the overflow (OV and OV32) flags
>
> I've applied 1/15, I've rest the left pending correction of 2/15 and
> discussions on the rest.

I thought of changing back to previous implementation, and posted v5 :-)

/me needs to slow down !

Regards,
Nikunj

Re: [Qemu-devel] [PATCH v2] intel_iommu: check misordered init when realize

[Peter Xu]

On Fri, Feb 24, 2017 at 12:07:33AM -0500, Pankaj Gupta wrote:
> Hello Peter,

Hi, Pankaj!

> 
> This solution looks to check dependency of 'vfio-pci' over 'intel-iommu'
> before 'intel-iommu' is not initialized.
> 
> Overall it looks good to me, just a small nit below.
>  
> > 
> > Intel vIOMMU devices are created with "-device" parameter, while here
> > actually we need to make sure the dmar device be created before other
> > PCI devices (like vfio-pci) so that we know iommu_fn will be setup
> > correctly before realizations of those PCI devices (it is sensible that
> > PCI device fetch these info during its realization). Now this ordering
> > yet cannot be achieved elsewhere, and devices will be created in the
> > order that user specified. That might be dangerous.
> > 
> > Here we add one more function to detect this kind of misordering issue,
> > then report to guest. Currently, the only known device that is affected
> > by this VT-d defect is the vfio-pci typed devices. So for now we just
> > check against it to make sure we are safe.
> > 
> > Signed-off-by: Peter Xu 
> > ---
> >  hw/i386/intel_iommu.c | 22 ++
> >  1 file changed, 22 insertions(+)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 22d8226..b723ece 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> > Error **errp)
> >  return true;
> >  }
> >  
> > +/*
> > + * TODO: we should have a better way to achieve the ordering rather
> > + * than this misorder check explicitly against vfio-pci. After all, no
> > + * one should be blamed for this, and vfio-pci did nothing wrong.
> > + */
> > +static bool vtd_detected_misorder_init(Error **errp)
> > +{
> > +Object *dev = object_resolve_path_type("", "vfio-pci", NULL);
> > +
> > +if (dev) {
> > +error_setg(errp, "Please specify \"intel-iommu\" before all the 
> > rest
> 
> "before all the rest" does not give much clue to user. Do you 
> think better
> error message would help? just a thought.

Yes this is my intention to emphasize that we should possibly put
intel-iommu even before all the PCI devices. As mentioned above (and
also in the commit message), although vfio-pci is the only one that is
affected by this, we should probably put intel-iommu even before all
the rest of PCI devices. E.g., in the future we can have new devices
that need this dependency as well (that vt-d being inited before that
device), which is still legal imho.

Meanwhile, I intended to avoid naming "vfio-pci" here since it'll let
user think of "something bad with vfio-pci" but actually vfio-pci did
nothing wrong.

Thanks,

-- peterx

[Qemu-devel] [PATCH v5 0/8] POWER9 TCG enablements - part15

[Nikunj A Dadhania]

This series contains implentation of CA32 and OV32 bits added to the 
ISA 3.0. Various fixed-point arithmetic instructions are updated to take
care of the newer flags. 

Finally the last patch adds new instruction mcrxrx, that helps reading 
the carry (CA and CA32) and the overflow (OV and OV32) flags

Changelog:
v4: 
* Get back to the v3 implementation. Dropped removal of split 
  out variables.
* Remove checking of isa300 on the write side. Read side will do 
  the checking.

v3:
* Get rid of cpu_ca, cpu_ov, cpu_so split out variables
* As most of the patches under went changes, dropped the 
  reviewed-bys(except neg[.] patch)

v2: 
* Add missing condition in narrow mode(add/subf), multiply and divide
* Drop nego patch, subf implementation is sufficient for setting OV and OV32
* Retaining neg[.], as the code is simplified.
* Fix OV resetting in compute_ov()

v1: 
* Use these ISA 3.0 flag to enable CA32 and OV32
* Re-write ca32 compute routine
* Add setting of flags for "neg." and "nego."

Nikunj A Dadhania (8):
  target/ppc: support for 32-bit carry and overflow
  target/ppc: update ca32 in arithmetic add
  target/ppc: update ca32 in arithmetic substract
  target/ppc: update overflow flags for add/sub
  target/ppc: use tcg ops for neg instruction
  target/ppc: add ov32 flag for multiply low insns
  target/ppc: add ov32 flag in divide operations
  target/ppc: add mcrxrx instruction

 target/ppc/cpu.c|  13 +-
 target/ppc/cpu.h|   7 +++
 target/ppc/int_helper.c |  53 +-
 target/ppc/translate.c  | 106 
 target/ppc/translate_init.c |   2 +-
 5 files changed, 138 insertions(+), 43 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v5 8/8] target/ppc: add mcrxrx instruction

[Nikunj A Dadhania]

mcrxrx: Move to CR from XER Extended

Signed-off-by: Nikunj A Dadhania 
Reviewed-by: Richard Henderson 
---
 target/ppc/translate.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 982e66f..6e6868b 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -3819,6 +3819,28 @@ static void gen_mcrxr(DisasContext *ctx)
 tcg_gen_movi_tl(cpu_ca, 0);
 }
 
+#ifdef TARGET_PPC64
+/* mcrxrx */
+static void gen_mcrxrx(DisasContext *ctx)
+{
+TCGv t0 = tcg_temp_new();
+TCGv t1 = tcg_temp_new();
+TCGv_i32 dst = cpu_crf[crfD(ctx->opcode)];
+
+/* copy OV and OV32 */
+tcg_gen_shli_tl(t0, cpu_ov, 1);
+tcg_gen_or_tl(t0, t0, cpu_ov32);
+tcg_gen_shli_tl(t0, t0, 2);
+/* copy CA and CA32 */
+tcg_gen_shli_tl(t1, cpu_ca, 1);
+tcg_gen_or_tl(t1, t1, cpu_ca32);
+tcg_gen_or_tl(t0, t0, t1);
+tcg_gen_trunc_tl_i32(dst, t0);
+tcg_temp_free(t0);
+tcg_temp_free(t1);
+}
+#endif
+
 /* mfcr mfocrf */
 static void gen_mfcr(DisasContext *ctx)
 {
@@ -6488,6 +6510,7 @@ GEN_HANDLER(mtcrf, 0x1F, 0x10, 0x04, 0x0801, 
PPC_MISC),
 #if defined(TARGET_PPC64)
 GEN_HANDLER(mtmsrd, 0x1F, 0x12, 0x05, 0x001EF801, PPC_64B),
 GEN_HANDLER_E(setb, 0x1F, 0x00, 0x04, 0x0003F801, PPC_NONE, PPC2_ISA300),
+GEN_HANDLER_E(mcrxrx, 0x1F, 0x00, 0x12, 0x007FF801, PPC_NONE, PPC2_ISA300),
 #endif
 GEN_HANDLER(mtmsr, 0x1F, 0x12, 0x04, 0x001EF801, PPC_MISC),
 GEN_HANDLER(mtspr, 0x1F, 0x13, 0x0E, 0x, PPC_MISC),
-- 
2.7.4

[Qemu-devel] [PATCH v5 6/8] target/ppc: add ov32 flag for multiply low insns

[Nikunj A Dadhania]

For Multiply Word:
SO, OV, and OV32 bits reflects overflow of the 32-bit result

For Multiply DoubleWord:
SO, OV, and OV32 bits reflects overflow of the 64-bit result

Signed-off-by: Nikunj A Dadhania 
Reviewed-by: Richard Henderson 
---
 target/ppc/translate.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index d4d9941..ccf3bff 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1285,6 +1285,9 @@ static void gen_mullwo(DisasContext *ctx)
 tcg_gen_sari_i32(t0, t0, 31);
 tcg_gen_setcond_i32(TCG_COND_NE, t0, t0, t1);
 tcg_gen_extu_i32_tl(cpu_ov, t0);
+if (is_isa300(ctx)) {
+tcg_gen_mov_tl(cpu_ov32, cpu_ov);
+}
 tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
 
 tcg_temp_free_i32(t0);
@@ -1346,6 +1349,9 @@ static void gen_mulldo(DisasContext *ctx)
 
 tcg_gen_sari_i64(t0, t0, 63);
 tcg_gen_setcond_i64(TCG_COND_NE, cpu_ov, t0, t1);
+if (is_isa300(ctx)) {
+tcg_gen_mov_tl(cpu_ov32, cpu_ov);
+}
 tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
 
 tcg_temp_free_i64(t0);
-- 
2.7.4

[Qemu-devel] [PATCH v5 7/8] target/ppc: add ov32 flag in divide operations

[Nikunj A Dadhania]

Add helper_div_compute_ov() in the int_helper for updating the overflow
flags.

For Divide Word:
SO, OV, and OV32 bits reflects overflow of the 32-bit result

For Divide DoubleWord:
SO, OV, and OV32 bits reflects overflow of the 64-bit result

Signed-off-by: Nikunj A Dadhania 
Reviewed-by: Richard Henderson 
---
 target/ppc/int_helper.c | 53 +++--
 target/ppc/translate.c  | 10 --
 2 files changed, 28 insertions(+), 35 deletions(-)

diff --git a/target/ppc/int_helper.c b/target/ppc/int_helper.c
index dd0a892..1cad62f 100644
--- a/target/ppc/int_helper.c
+++ b/target/ppc/int_helper.c
@@ -28,6 +28,22 @@
 /*/
 /* Fixed point operations helpers */
 
+static inline void helper_div_compute_ov(CPUPPCState *env, uint32_t oe,
+ int overflow)
+{
+if (oe) {
+if (unlikely(overflow)) {
+env->so = env->ov = 1;
+} else {
+env->ov = 0;
+}
+
+if (is_isa300(env)) {
+env->ov32 = env->ov;
+}
+}
+}
+
 target_ulong helper_divweu(CPUPPCState *env, target_ulong ra, target_ulong rb,
uint32_t oe)
 {
@@ -48,14 +64,7 @@ target_ulong helper_divweu(CPUPPCState *env, target_ulong 
ra, target_ulong rb,
 rt = 0; /* Undefined */
 }
 
-if (oe) {
-if (unlikely(overflow)) {
-env->so = env->ov = 1;
-} else {
-env->ov = 0;
-}
-}
-
+helper_div_compute_ov(env, oe, overflow);
 return (target_ulong)rt;
 }
 
@@ -80,14 +89,7 @@ target_ulong helper_divwe(CPUPPCState *env, target_ulong ra, 
target_ulong rb,
 rt = 0; /* Undefined */
 }
 
-if (oe) {
-if (unlikely(overflow)) {
-env->so = env->ov = 1;
-} else {
-env->ov = 0;
-}
-}
-
+helper_div_compute_ov(env, oe, overflow);
 return (target_ulong)rt;
 }
 
@@ -104,14 +106,7 @@ uint64_t helper_divdeu(CPUPPCState *env, uint64_t ra, 
uint64_t rb, uint32_t oe)
 rt = 0; /* Undefined */
 }
 
-if (oe) {
-if (unlikely(overflow)) {
-env->so = env->ov = 1;
-} else {
-env->ov = 0;
-}
-}
-
+helper_div_compute_ov(env, oe, overflow);
 return rt;
 }
 
@@ -126,15 +121,7 @@ uint64_t helper_divde(CPUPPCState *env, uint64_t rau, 
uint64_t rbu, uint32_t oe)
 rt = 0; /* Undefined */
 }
 
-if (oe) {
-
-if (unlikely(overflow)) {
-env->so = env->ov = 1;
-} else {
-env->ov = 0;
-}
-}
-
+helper_div_compute_ov(env, oe, overflow);
 return rt;
 }
 
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index ccf3bff..982e66f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -1021,6 +1021,9 @@ static inline void gen_op_arith_divw(DisasContext *ctx, 
TCGv ret, TCGv arg1,
 }
 if (compute_ov) {
 tcg_gen_extu_i32_tl(cpu_ov, t2);
+if (is_isa300(ctx)) {
+tcg_gen_extu_i32_tl(cpu_ov32, t2);
+}
 tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
 }
 tcg_temp_free_i32(t0);
@@ -1092,6 +1095,9 @@ static inline void gen_op_arith_divd(DisasContext *ctx, 
TCGv ret, TCGv arg1,
 }
 if (compute_ov) {
 tcg_gen_mov_tl(cpu_ov, t2);
+if (is_isa300(ctx)) {
+tcg_gen_mov_tl(cpu_ov32, t2);
+}
 tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
 }
 tcg_temp_free_i64(t0);
@@ -1110,10 +1116,10 @@ static void glue(gen_, name)(DisasContext *ctx)
   cpu_gpr[rA(ctx->opcode)], cpu_gpr[rB(ctx->opcode)], \
   sign, compute_ov);  \
 }
-/* divwu  divwu.  divwuo  divwuo.   */
+/* divdu  divdu.  divduo  divduo.   */
 GEN_INT_ARITH_DIVD(divdu, 0x0E, 0, 0);
 GEN_INT_ARITH_DIVD(divduo, 0x1E, 0, 1);
-/* divw  divw.  divwo  divwo.   */
+/* divd  divd.  divdo  divdo.   */
 GEN_INT_ARITH_DIVD(divd, 0x0F, 1, 0);
 GEN_INT_ARITH_DIVD(divdo, 0x1F, 1, 1);
 
-- 
2.7.4

[Qemu-devel] [PATCH v5 1/8] target/ppc: support for 32-bit carry and overflow

[Nikunj A Dadhania]

POWER ISA 3.0 adds CA32 and OV32 status in 64-bit mode. Add the flags
and corresponding defines.

Moreover, CA32 is updated when CA is updated and OV32 is updated when OV
is updated.

Arithmetic instructions:
* Addition and Substractions:

addic, addic., subfic, addc, subfc, adde, subfe, addme, subfme,
addze, and subfze always updates CA and CA32.

=> CA reflects the carry out of bit 0 in 64-bit mode and out of
   bit 32 in 32-bit mode.
=> CA32 reflects the carry out of bit 32 independent of the
   mode.

=> SO and OV reflects overflow of the 64-bit result in 64-bit
   mode and overflow of the low-order 32-bit result in 32-bit
   mode
=> OV32 reflects overflow of the low-order 32-bit independent of
   the mode

* Multiply Low and Divide:

For mulld, divd, divde, divdu and divdeu: SO, OV, and OV32 bits
reflects overflow of the 64-bit result

For mullw, divw, divwe, divwu and divweu: SO, OV, and OV32 bits
reflects overflow of the 32-bit result

 * Negate with OE=1 (nego)

   For 64-bit mode if the register RA contains
   0x8000___, OV and OV32 are set to 1.

   For 32-bit mode if the register RA contains 0x8000_, OV and
   OV32 are set to 1.

Signed-off-by: Nikunj A Dadhania 
---
 target/ppc/cpu.c| 13 -
 target/ppc/cpu.h|  7 +++
 target/ppc/translate.c  | 21 ++---
 target/ppc/translate_init.c |  2 +-
 4 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index de3004b..2801166 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -23,6 +23,12 @@
 
 target_ulong cpu_read_xer(CPUPPCState *env)
 {
+if (is_isa300(env)) {
+return env->xer | (env->so << XER_SO) |
+(env->ov << XER_OV) | (env->ca << XER_CA) |
+(env->ov32 << XER_OV32) | (env->ca32 << XER_CA32);
+}
+
 return env->xer | (env->so << XER_SO) | (env->ov << XER_OV) |
 (env->ca << XER_CA);
 }
@@ -32,5 +38,10 @@ void cpu_write_xer(CPUPPCState *env, target_ulong xer)
 env->so = (xer >> XER_SO) & 1;
 env->ov = (xer >> XER_OV) & 1;
 env->ca = (xer >> XER_CA) & 1;
-env->xer = xer & ~((1u << XER_SO) | (1u << XER_OV) | (1u << XER_CA));
+/* write all the flags, while reading back check of isa300 */
+env->ov32 = (xer >> XER_OV32) & 1;
+env->ca32 = (xer >> XER_CA32) & 1;
+env->xer = xer & ~((1ul << XER_SO) |
+   (1ul << XER_OV) | (1ul << XER_CA) |
+   (1ul << XER_OV32) | (1ul << XER_CA32));
 }
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index b559b67..ee2eb45 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -965,6 +965,8 @@ struct CPUPPCState {
 target_ulong so;
 target_ulong ov;
 target_ulong ca;
+target_ulong ov32;
+target_ulong ca32;
 /* Reservation address */
 target_ulong reserve_addr;
 /* Reservation value */
@@ -1372,11 +1374,15 @@ int ppc_compat_max_threads(PowerPCCPU *cpu);
 #define XER_SO  31
 #define XER_OV  30
 #define XER_CA  29
+#define XER_OV32  19
+#define XER_CA32  18
 #define XER_CMP  8
 #define XER_BC   0
 #define xer_so  (env->so)
 #define xer_ov  (env->ov)
 #define xer_ca  (env->ca)
+#define xer_ov32  (env->ov)
+#define xer_ca32  (env->ca)
 #define xer_cmp ((env->xer >> XER_CMP) & 0xFF)
 #define xer_bc  ((env->xer >> XER_BC)  & 0x7F)
 
@@ -2343,6 +2349,7 @@ enum {
 
 /*/
 
+#define is_isa300(ctx) (!!(ctx->insns_flags2 & PPC2_ISA300))
 target_ulong cpu_read_xer(CPUPPCState *env);
 void cpu_write_xer(CPUPPCState *env, target_ulong xer);
 
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index b09e16f..be7378b 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -71,7 +71,7 @@ static TCGv cpu_lr;
 #if defined(TARGET_PPC64)
 static TCGv cpu_cfar;
 #endif
-static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca;
+static TCGv cpu_xer, cpu_so, cpu_ov, cpu_ca, cpu_ov32, cpu_ca32;
 static TCGv cpu_reserve;
 static TCGv cpu_fpscr;
 static TCGv_i32 cpu_access_type;
@@ -173,6 +173,10 @@ void ppc_translate_init(void)
 offsetof(CPUPPCState, ov), "OV");
 cpu_ca = tcg_global_mem_new(cpu_env,
 offsetof(CPUPPCState, ca), "CA");
+cpu_ov32 = tcg_global_mem_new(cpu_env,
+  offsetof(CPUPPCState, ov32), "OV32");
+cpu_ca32 = tcg_global_mem_new(cpu_env,
+  offsetof(CPUPPCState, ca32), "CA32");
 
 cpu_reserve = tcg_global_mem_new(cpu_env,
  offsetof(CPUPPCState, reserve_addr),
@@ -3703,7 +3707,7 @@ static void gen_tdi(DisasContext *ctx)
 
 /***  Processor control***/
 
-static void 

[Qemu-devel] [PATCH v5 4/8] target/ppc: update overflow flags for add/sub

[Nikunj A Dadhania]

* SO and OV reflects overflow of the 64-bit result in 64-bit mode and
  overflow of the low-order 32-bit result in 32-bit mode

* OV32 reflects overflow of the low-order 32-bit independent of the mode

Signed-off-by: Nikunj A Dadhania 
---
 target/ppc/translate.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index e083082..16f422f 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -810,9 +810,16 @@ static inline void gen_op_arith_compute_ov(DisasContext 
*ctx, TCGv arg0,
 }
 tcg_temp_free(t0);
 if (NARROW_MODE(ctx)) {
-tcg_gen_ext32s_tl(cpu_ov, cpu_ov);
+tcg_gen_extract_tl(cpu_ov, cpu_ov, 31, 1);
+if (is_isa300(ctx)) {
+tcg_gen_mov_tl(cpu_ov32, cpu_ov);
+}
+} else {
+if (is_isa300(ctx)) {
+tcg_gen_extract_tl(cpu_ov32, cpu_ov, 31, 1);
+}
+tcg_gen_extract_tl(cpu_ov, cpu_ov, 63, 1);
 }
-tcg_gen_shri_tl(cpu_ov, cpu_ov, TARGET_LONG_BITS - 1);
 tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v5 2/8] target/ppc: update ca32 in arithmetic add

[Nikunj A Dadhania]

Adds routine to compute ca32 - gen_op_arith_compute_ca32

For 64-bit mode use the compute ca32 routine. While for 32-bit mode, CA
and CA32 will have same value.

Signed-off-by: Nikunj A Dadhania 
Reviewed-by: Richard Henderson 
---
 target/ppc/translate.c | 21 +
 1 file changed, 21 insertions(+)

diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index be7378b..eba83ef 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -816,6 +816,23 @@ static inline void gen_op_arith_compute_ov(DisasContext 
*ctx, TCGv arg0,
 tcg_gen_or_tl(cpu_so, cpu_so, cpu_ov);
 }
 
+static inline void gen_op_arith_compute_ca32(DisasContext *ctx,
+ TCGv res, TCGv arg0, TCGv arg1,
+ int sub)
+{
+TCGv t0;
+
+if (!is_isa300(ctx)) {
+return;
+}
+
+t0 = tcg_temp_new();
+tcg_gen_xor_tl(t0, arg0, arg1);
+tcg_gen_xor_tl(t0, t0, res);
+tcg_gen_extract_tl(cpu_ca32, t0, 32, 1);
+tcg_temp_free(t0);
+}
+
 /* Common add function */
 static inline void gen_op_arith_add(DisasContext *ctx, TCGv ret, TCGv arg1,
 TCGv arg2, bool add_ca, bool compute_ca,
@@ -842,6 +859,9 @@ static inline void gen_op_arith_add(DisasContext *ctx, TCGv 
ret, TCGv arg1,
 tcg_temp_free(t1);
 tcg_gen_shri_tl(cpu_ca, cpu_ca, 32);   /* extract bit 32 */
 tcg_gen_andi_tl(cpu_ca, cpu_ca, 1);
+if (is_isa300(ctx)) {
+tcg_gen_mov_tl(cpu_ca32, cpu_ca);
+}
 } else {
 TCGv zero = tcg_const_tl(0);
 if (add_ca) {
@@ -850,6 +870,7 @@ static inline void gen_op_arith_add(DisasContext *ctx, TCGv 
ret, TCGv arg1,
 } else {
 tcg_gen_add2_tl(t0, cpu_ca, arg1, zero, arg2, zero);
 }
+gen_op_arith_compute_ca32(ctx, t0, arg1, arg2, 0);
 tcg_temp_free(zero);
 }
 } else {
-- 
2.7.4

[Qemu-devel] [PATCH v2] mttcg/i386: Patch instruction using async_safe_* framework

[Pranith Kumar]

In mttcg, calling pause_all_vcpus() during execution from the
generated TBs causes a deadlock if some vCPU is waiting for exclusive
execution in start_exclusive(). Fix this by using the aync_safe_*
framework instead of pausing vcpus for patching instructions.

CC: Richard Henderson 
CC: Peter Maydell 
CC: Alex Bennée 
Signed-off-by: Pranith Kumar 
---
 hw/i386/kvmvapic.c | 82 ++
 1 file changed, 52 insertions(+), 30 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 82a4955..11b0d49 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -383,8 +383,7 @@ static void patch_byte(X86CPU *cpu, target_ulong addr, 
uint8_t byte)
 cpu_memory_rw_debug(CPU(cpu), addr, , 1, 1);
 }
 
-static void patch_call(VAPICROMState *s, X86CPU *cpu, target_ulong ip,
-   uint32_t target)
+static void patch_call(X86CPU *cpu, target_ulong ip, uint32_t target)
 {
 uint32_t offset;
 
@@ -393,23 +392,24 @@ static void patch_call(VAPICROMState *s, X86CPU *cpu, 
target_ulong ip,
 cpu_memory_rw_debug(CPU(cpu), ip + 1, (void *), sizeof(offset), 1);
 }
 
-static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
+struct PatchInfo {
+VAPICHandlers *handler;
+target_ulong ip;
+};
+
+static void do_patch_instruction(CPUState *cs, run_on_cpu_data data)
 {
-CPUState *cs = CPU(cpu);
-CPUX86State *env = >env;
-VAPICHandlers *handlers;
+X86CPU *x86_cpu = X86_CPU(cs);
+CPUX86State *env = _cpu->env;
+struct PatchInfo *info = (struct PatchInfo *) data.host_ptr;
+VAPICHandlers *handlers = info->handler;
+target_ulong ip = info->ip;
 uint8_t opcode[2];
 uint32_t imm32 = 0;
 target_ulong current_pc = 0;
 target_ulong current_cs_base = 0;
 uint32_t current_flags = 0;
 
-if (smp_cpus == 1) {
-handlers = >rom_state.up;
-} else {
-handlers = >rom_state.mp;
-}
-
 if (!kvm_enabled()) {
 cpu_get_tb_cpu_state(env, _pc, _cs_base,
  _flags);
@@ -421,48 +421,70 @@ static void patch_instruction(VAPICROMState *s, X86CPU 
*cpu, target_ulong ip)
 }
 }
 
-pause_all_vcpus();
-
 cpu_memory_rw_debug(cs, ip, opcode, sizeof(opcode), 0);
 
 switch (opcode[0]) {
 case 0x89: /* mov r32 to r/m32 */
-patch_byte(cpu, ip, 0x50 + modrm_reg(opcode[1]));  /* push reg */
-patch_call(s, cpu, ip + 1, handlers->set_tpr);
+patch_byte(x86_cpu, ip, 0x50 + modrm_reg(opcode[1]));  /* push reg */
+patch_call(x86_cpu, ip + 1, handlers->set_tpr);
 break;
 case 0x8b: /* mov r/m32 to r32 */
-patch_byte(cpu, ip, 0x90);
-patch_call(s, cpu, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
+patch_byte(x86_cpu, ip, 0x90);
+patch_call(x86_cpu, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
 break;
 case 0xa1: /* mov abs to eax */
-patch_call(s, cpu, ip, handlers->get_tpr[0]);
+patch_call(x86_cpu, ip, handlers->get_tpr[0]);
 break;
 case 0xa3: /* mov eax to abs */
-patch_call(s, cpu, ip, handlers->set_tpr_eax);
+patch_call(x86_cpu, ip, handlers->set_tpr_eax);
 break;
 case 0xc7: /* mov imm32, r/m32 (c7/0) */
-patch_byte(cpu, ip, 0x68);  /* push imm32 */
+patch_byte(x86_cpu, ip, 0x68);  /* push imm32 */
 cpu_memory_rw_debug(cs, ip + 6, (void *), sizeof(imm32), 0);
 cpu_memory_rw_debug(cs, ip + 1, (void *), sizeof(imm32), 1);
-patch_call(s, cpu, ip + 5, handlers->set_tpr);
+patch_call(x86_cpu, ip + 5, handlers->set_tpr);
 break;
 case 0xff: /* push r/m32 */
-patch_byte(cpu, ip, 0x50); /* push eax */
-patch_call(s, cpu, ip + 1, handlers->get_tpr_stack);
+patch_byte(x86_cpu, ip, 0x50); /* push eax */
+patch_call(x86_cpu, ip + 1, handlers->get_tpr_stack);
 break;
 default:
 abort();
 }
 
-resume_all_vcpus();
+g_free(info);
+}
+
+static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
+{
+CPUState *cs = CPU(cpu);
+VAPICHandlers *handlers;
+struct PatchInfo *info;
+
+if (smp_cpus == 1) {
+handlers = >rom_state.up;
+} else {
+handlers = >rom_state.mp;
+}
+
+info  = g_new(struct PatchInfo, 1);
+info->handler = handlers;
+info->ip = ip;
 
 if (!kvm_enabled()) {
-/* Both tb_lock and iothread_mutex will be reset when
- *  longjmps back into the cpu_exec loop. */
-tb_lock();
-tb_gen_code(cs, current_pc, current_cs_base, current_flags, 1);
-cpu_loop_exit_noexc(cs);
+const run_on_cpu_func fn = do_patch_instruction;
+
+async_safe_run_on_cpu(cs, fn, RUN_ON_CPU_HOST_PTR(info));
+cs->exception_index = EXCP_INTERRUPT;
+cpu_loop_exit(cs);
  

Re: [Qemu-devel] [PATCH V8 2/2] Add a new qmp command to do checkpoint, query xen replication status

[Zhang Chen]

On 02/23/2017 11:34 PM, Eric Blake wrote:

On 02/23/2017 01:14 AM, Zhang Chen wrote:

We can call this qmp command to do checkpoint outside of qemu.
Xen colo will need this function.

Signed-off-by: Zhang Chen 
Signed-off-by: Wen Congyang 
Reviewed-by: Eric Blake 

You made a substantial change to this patch since v7 in response to my
comments; when you do that, it's best to remove the R-b to make sure I
re-review the changes and am still happy with them.


OK, I will remove the R-b in next version.




---
  migration/colo.c | 23 +++
  qapi-schema.json | 48 
  2 files changed, 71 insertions(+)

  ##
+# @ReplicationStatus:
+#
+# The result format for 'query-xen-replication-status'.
+#
+# @status: true to error, false to normal.

This is now a poor name for the parameter.  Please rename it; probably
to @error (if you want to keep true meaning a problem has been
detected), or to @okay (if you want to invert the sense, and @desc is
only present when @okay is false).


I will rename it to "@error" in next version.




+#
+# @desc: #optional the human readable error description string, when
+#@status is 'true'.
+#
+# Since: 2.9
+##
+{ 'struct': 'ReplicationStatus',
+  'data': { 'status': 'bool', '*desc': 'str' } }
+
+##
+# @query-xen-replication-status:
+#
+# Query replication status while the vm is running.
+#
+# Returns: A @ReplicationResult objects showing the status.

s/objects/object/


I got it.




+#
+# Example:
+#
+# -> { "execute": "query-xen-replication-status" }
+# <- { "return": { "status": "normal" } }

This example is now wrong.

You'll need a v9.


Yes, I will send the V9 later.

Thanks
Zhang Chen





--
Thanks
Zhang Chen

Re: [Qemu-devel] [PATCH v4 00/15] POWER9 TCG enablements - part15

[David Gibson]

On Fri, Feb 24, 2017 at 01:26:25AM +0530, Nikunj A Dadhania wrote:
> Patches:
> 01-06  Cleans up the XER split out variables and now the 
>flag bits are stored in XER at their respective places 
> 
> 07-14  Contains implentation of CA32 and OV32 bits added to the 
>ISA 3.0. Various fixed-point arithmetic instructions are 
>updated to take care of the newer flags.
>  
> 15 Finally the last patch adds new instruction mcrxrx, that helps
>reading the carry (CA and CA32) and the overflow (OV and OV32) flags

I've applied 1/15, I've rest the left pending correction of 2/15 and
discussions on the rest.

> 
> 
> Booted the POWER8 guest fine, needs more testing as changes are 
> intrusive in nature.
> 
> Changelog:
> v3:
> * Get rid of cpu_ca, cpu_ov, cpu_so split out variables
> * As most of the patches under went changes, dropped the 
>   reviewed-bys(except neg[.] patch)
> 
> v2: 
> * Add missing condition in narrow mode(add/subf), multiply and divide
> * Drop nego patch, subf implementation is sufficient for setting OV and OV32
> * Retaining neg[.], as the code is simplified.
> * Fix OV resetting in compute_ov()
> 
> v1: 
> * Use these ISA 3.0 flag to enable CA32 and OV32
> * Re-write ca32 compute routine
> * Add setting of flags for "neg." and "nego."
> 
> Nikunj A Dadhania (15):
>   target/ppc: introduce helper_update_ov_legacy
>   target/ppc: update ov flag from remaining paths
>   target/ppc: introduce helper_update_ca_legacy
>   target/ppc: add gen_op_update_ca_legacy() helper
>   target/ppc: add gen_op_update_ov_legacy() helper
>   target/ppc: remove xer split-out flags(so, ov, ca)
>   target/ppc: support for 32-bit carry and overflow
>   target/ppc: update ca32 in arithmetic add
>   target/ppc: update ca32 in arithmetic substract
>   target/ppc: add gen_op_update_ov_isa300()
>   target/ppc: update OV/OV32 for mull[d,w] insns
>   target/ppc: update OV/OV32 for divide operations
>   target/ppc: update OV/OV32 flags for add/sub
>   target/ppc: use tcg ops for neg instruction
>   target/ppc: add mcrxrx instruction
> 
>  target/ppc/cpu.c|   8 +-
>  target/ppc/cpu.h|  33 ++--
>  target/ppc/int_helper.c |  90 ++-
>  target/ppc/translate.c  | 396 
> +++-
>  4 files changed, 371 insertions(+), 156 deletions(-)
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH 5/6] target/ppc: Eliminate htab_base and htab_mask variables

[David Gibson]

On Thu, Feb 23, 2017 at 04:52:59PM +1100, Suraj Jitindar Singh wrote:
> On Thu, 2017-02-23 at 13:09 +1100, David Gibson wrote:
> > CPUPPCState includes fields htab_base and htab_mask which store the
> > base
> > address (GPA) and size (as a mask) of the guest's hashed page table
> > (HPT).
> > These are set when the SDR1 register is updated.
> > 
> > Keeping these in sync with the SDR1 is actually a little bit fiddly,
> > and
> > probably not useful for performance, since keeping them expands the
> > size of
> > CPUPPCState.  It also makes some upcoming changes harder to
> > implement.
> > 
> > This patch removes these fields, in favour of calculating them
> > directly
> > from the SDR1 contents when necessary.
> > 
> > This does make a change to the behaviour of attempting to write a bad
> > value
> > (invalid HPT size) to the SDR1 with an mtspr
> > instruction.  Previously, the
> > bad value would be stored in SDR1 and could be retrieved with a later
> > mfspr, but the HPT size as used by the softmmu would be, clamped to
> > the
> > allowed values.  Now, writing a bad value is treated as a no-op.  An
> > error
> > message is printed in both new and old versions.
> > 
> > I'm not sure which behaviour, if either, matches real hardware.  I
> > don't
> > think it matters that much, since it's pretty clear that if an OS
> > writes
> > a bad value to SDR1, it's not going to boot.
> > 
> > Signed-off-by: David Gibson 
> 
> Comments Below.
> 
> > ---
> >  hw/ppc/spapr_hcall.c|  4 ++--
> >  target/ppc/cpu.h|  8 
> >  target/ppc/machine.c|  1 -
> >  target/ppc/mmu-hash32.c | 14 +++---
> >  target/ppc/mmu-hash32.h | 24 ++--
> >  target/ppc/mmu-hash64.c | 37 -
> >  target/ppc/mmu-hash64.h | 13 +
> >  target/ppc/mmu_helper.c | 31 ---
> >  8 files changed, 72 insertions(+), 60 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index fd961b5..85d96f6 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -50,9 +50,9 @@ static bool has_spr(PowerPCCPU *cpu, int spr)
> >  static inline bool valid_ptex(PowerPCCPU *cpu, target_ulong ptex)
> >  {
> >  /*
> > - * hash value/pteg group index is normalized by htab_mask
> > + * hash value/pteg group index is normalized by HPT mask
> >   */
> > -if (((ptex & ~7ULL) / HPTES_PER_GROUP) & ~cpu->env.htab_mask) {
> > +if (((ptex & ~7ULL) / HPTES_PER_GROUP) &
> > ~ppc_hash64_hpt_mask(cpu)) {
> >  return false;
> >  }
> >  return true;
> > diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> > index c89973e..c6cd9ab 100644
> > --- a/target/ppc/cpu.h
> > +++ b/target/ppc/cpu.h
> > @@ -309,11 +309,6 @@ union ppc_tlb_t {
> >  #define SDR_32_HTABORG 0xUL
> >  #define SDR_32_HTABMASK0x01FFUL
> >  
> > -#if defined(TARGET_PPC64)
> > -#define SDR_64_HTABORG 0xFFFCULL
> > -#define SDR_64_HTABSIZE0x001FULL
> > -#endif /* defined(TARGET_PPC64 */
> > -
> >  typedef struct ppc_slb_t ppc_slb_t;
> >  struct ppc_slb_t {
> >  uint64_t esid;
> > @@ -1006,9 +1001,6 @@ struct CPUPPCState {
> >  /* tcg TLB needs flush (deferred slb inval instruction
> > typically) */
> >  #endif
> >  /* segment registers */
> > -hwaddr htab_base;
> > -/* mask used to normalize hash value to PTEG index */
> > -hwaddr htab_mask;
> >  target_ulong sr[32];
> >  /* externally stored hash table */
> >  uint8_t *external_htab;
> > diff --git a/target/ppc/machine.c b/target/ppc/machine.c
> > index df9f7a4..1ccbc8a 100644
> > --- a/target/ppc/machine.c
> > +++ b/target/ppc/machine.c
> > @@ -229,7 +229,6 @@ static int cpu_post_load(void *opaque, int
> > version_id)
> >  }
> >  
> >  if (!env->external_htab) {
> > -/* Restore htab_base and htab_mask variables */
> >  ppc_store_sdr1(env, env->spr[SPR_SDR1]);
> 
> Do we still need to do this? As far as I can tell it pretty much does:
> env->spr[SPR_SDR1] = env->spr[SPR_SDR1]
> and checks the htab_size, which would have been done when we set SDR1
> in the first place anyway?

I'd prefer to recheck it here: if we had a bug in the verification,
then fixed it, I wouldn't want the bugfix to be bypassed by migrating
in from an older version.

> >  }
> >  
> > diff --git a/target/ppc/mmu-hash32.c b/target/ppc/mmu-hash32.c
> > index 29bace6..03ae3c1 100644
> > --- a/target/ppc/mmu-hash32.c
> > +++ b/target/ppc/mmu-hash32.c
> > @@ -304,9 +304,9 @@ static int ppc_hash32_direct_store(PowerPCCPU
> > *cpu, target_ulong sr,
> >  
> >  hwaddr get_pteg_offset32(PowerPCCPU *cpu, hwaddr hash)
> >  {
> > -CPUPPCState *env = >env;
> > +target_ulong mask = ppc_hash32_hpt_mask(cpu);
> >  
> > -return (hash * HASH_PTEG_SIZE_32) & env->htab_mask;
> > +return (hash * HASH_PTEG_SIZE_32) & mask;
> >  }
> >  
> >  static 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[David Gibson]

On Fri, Feb 24, 2017 at 06:11:30AM +0530, Nikunj Dadhania wrote:
> On 24 February 2017 at 04:23, David Gibson  
> wrote:
> > On Fri, Feb 24, 2017 at 09:34:32AM +1100, Richard Henderson wrote:
> >> On 02/23/2017 05:40 PM, Nikunj A Dadhania wrote:
> >> > Richard Henderson  writes:
> >> > > These functions are becoming quite large.  Are they performance 
> >> > > critical enough
> >> > > that they need to stay as inline code, or should they be moved to 
> >> > > helpers and
> >> > > share code with cpu_read/write_xer?
> >> >
> >> > Just to boot to login prompt, these are the numbers for 
> >> > gen_read/write_xer:
> >> >
> >> > helper_myprint - rd_count 231103, wr_count 68897
> >> >
> >> > And it keeps on incrementing, maybe scope of optimization here.
> >>
> >> That's not very large considering the total number of instructions executed
> >> during a boot to prompt.
> >>
> >> Thoughts, David?
> >
> > Hm, I'm not clear if that's the number of executions, or the number of
> > translations.
> 
> That is number of executions.

Ok, I guess that's not that big, then.  I guess moving them into
helpers would make sense.

Although I guess they'd shrink right down again if we put an
env->xer_mask in.  Thoughts on that option Richard?

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v6 0/2] spapr: generate DT node names

[David Gibson]

On Fri, Feb 17, 2017 at 02:31:32PM +0100, Laurent Vivier wrote:
1;4601;0c> This series is a rebased series from September 2015, it has been
> reviewed but has never reached the master branch.
> 
> When DT node names for PCI devices are generated by SLOF,
> they are generated according to the type of the device
> (for instance, ethernet for virtio-net-pci device).
> 
> Node name for hotplugged devices is generated by QEMU.
> This series adds the mechanic to QEMU to create the node
> name according to the device type too.

Applied to ppc-for-2.9, thanks.

> 
> v6: rebase on master
> 
> [Wed, 30 Sep 2015]
> v5: store subclass and iface ids as-is (int) and mask them when
> we compare them.
> 
> v4: move pci_ids.h to a separate patch, fix PCI_CLASS_NETWORK_WORDFIP
> remove  duplicate NL, remove 386, 486 and alpha subclasses
> rename "unknown-legacy-device", correctly check array size
> add Thomas and Michael "Reviewed-by".
> 
> v3: use values from pci_ids.h, update pci_ids.h values
> keep only details for USB (xhci, ohci, ...) and PIC (IO-APIC, IO-XAPIC)
> 
> v2: Use CamelCase name, remove misc-* name,
> remove _OTHER entries to fallback to class name (as SLOF does).
> Fix typo (IPMI-bltr).
> 
> Laurent Vivier (2):
>   PCI: add missing classes in pci_ids.h to build device tree
>   spapr: generate DT node names
> 
>  hw/ppc/spapr_pci.c   | 290 
> ---
>  include/hw/pci/pci_ids.h | 112 --
>  2 files changed, 379 insertions(+), 23 deletions(-)
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v2] intel_iommu: check misordered init when realize

[Pankaj Gupta]

Hello Peter,

This solution looks to check dependency of 'vfio-pci' over 'intel-iommu'
before 'intel-iommu' is not initialized.

Overall it looks good to me, just a small nit below.
 
> 
> Intel vIOMMU devices are created with "-device" parameter, while here
> actually we need to make sure the dmar device be created before other
> PCI devices (like vfio-pci) so that we know iommu_fn will be setup
> correctly before realizations of those PCI devices (it is sensible that
> PCI device fetch these info during its realization). Now this ordering
> yet cannot be achieved elsewhere, and devices will be created in the
> order that user specified. That might be dangerous.
> 
> Here we add one more function to detect this kind of misordering issue,
> then report to guest. Currently, the only known device that is affected
> by this VT-d defect is the vfio-pci typed devices. So for now we just
> check against it to make sure we are safe.
> 
> Signed-off-by: Peter Xu 
> ---
>  hw/i386/intel_iommu.c | 22 ++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 22d8226..b723ece 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s,
> Error **errp)
>  return true;
>  }
>  
> +/*
> + * TODO: we should have a better way to achieve the ordering rather
> + * than this misorder check explicitly against vfio-pci. After all, no
> + * one should be blamed for this, and vfio-pci did nothing wrong.
> + */
> +static bool vtd_detected_misorder_init(Error **errp)
> +{
> +Object *dev = object_resolve_path_type("", "vfio-pci", NULL);
> +
> +if (dev) {
> +error_setg(errp, "Please specify \"intel-iommu\" before all the rest

"before all the rest" does not give much clue to user. Do you think 
better
error message would help? just a thought.
> "
> +   "of the devices.");
> +return true;
> +}
> +
> +return false;
> +}
> +
>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
>  PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> @@ -2567,6 +2585,10 @@ static void vtd_realize(DeviceState *dev, Error
> **errp)
>  IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>  X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>  
> +if (vtd_detected_misorder_init(errp)) {
> +return;
> +}
> +
>  VTD_DPRINTF(GENERAL, "");
>  x86_iommu->type = TYPE_INTEL;
>  
> --
> 2.7.4
> 
> 
> 

[Qemu-devel] [PATCH qemu] sysemu: support up to 1024 vCPUs

[Alexey Kardashevskiy]

From: Greg Kurz 

Some systems can already provide more than 255 hardware threads.

Bumping the QEMU limit to 1024 seems reasonable:
- it has no visible overhead in top;
- the limit itself has no effect on hot paths.

Cc: Greg Kurz 
Signed-off-by: Alexey Kardashevskiy 
---

With ulimit -u/-n bumped (nproc and nofile), I was able to boot a guest
with 1024 CPUs, both with threads=1 and threads=8.

It takes time though - 3:15 to get to the guest shell but it is probably
expected on 160-threads machine.

---
 hw/ppc/spapr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e465d7ac98..46b81a625d 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2712,7 +2712,7 @@ static void spapr_machine_class_init(ObjectClass *oc, 
void *data)
 mc->init = ppc_spapr_init;
 mc->reset = ppc_spapr_reset;
 mc->block_default_type = IF_SCSI;
-mc->max_cpus = 255;
+mc->max_cpus = 1024;
 mc->no_parallel = 1;
 mc->default_boot_order = "";
 mc->default_ram_size = 512 * M_BYTE;
-- 
2.11.0

[Qemu-devel] [PATCH v2] intel_iommu: check misordered init when realize

[Peter Xu]

Intel vIOMMU devices are created with "-device" parameter, while here
actually we need to make sure the dmar device be created before other
PCI devices (like vfio-pci) so that we know iommu_fn will be setup
correctly before realizations of those PCI devices (it is sensible that
PCI device fetch these info during its realization). Now this ordering
yet cannot be achieved elsewhere, and devices will be created in the
order that user specified. That might be dangerous.

Here we add one more function to detect this kind of misordering issue,
then report to guest. Currently, the only known device that is affected
by this VT-d defect is the vfio-pci typed devices. So for now we just
check against it to make sure we are safe.

Signed-off-by: Peter Xu 
---
 hw/i386/intel_iommu.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 22d8226..b723ece 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2560,6 +2560,24 @@ static bool vtd_decide_config(IntelIOMMUState *s, Error 
**errp)
 return true;
 }
 
+/*
+ * TODO: we should have a better way to achieve the ordering rather
+ * than this misorder check explicitly against vfio-pci. After all, no
+ * one should be blamed for this, and vfio-pci did nothing wrong.
+ */
+static bool vtd_detected_misorder_init(Error **errp)
+{
+Object *dev = object_resolve_path_type("", "vfio-pci", NULL);
+
+if (dev) {
+error_setg(errp, "Please specify \"intel-iommu\" before all the rest "
+   "of the devices.");
+return true;
+}
+
+return false;
+}
+
 static void vtd_realize(DeviceState *dev, Error **errp)
 {
 PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
@@ -2567,6 +2585,10 @@ static void vtd_realize(DeviceState *dev, Error **errp)
 IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
 X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
 
+if (vtd_detected_misorder_init(errp)) {
+return;
+}
+
 VTD_DPRINTF(GENERAL, "");
 x86_iommu->type = TYPE_INTEL;
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v8 1/2] block/vxhs.c: Add support for a new block device type called "vxhs"

[ashish mittal]

Hi,

Just want to check if the following mechanism for accepting the secret
password looks OK?

We have yet to internally discuss the semantics of how we plan to use
the user-ID/password for authentication. This diff is just to
understand how I am expected to accept the secret from the command
line.

Example specifying the username and password:

$ ./qemu-io --trace enable=vxhs* --object
secret,id=ashish,data=asd888d989s  -c 'read 66000 128k'
'json:{"server.host": "127.0.0.1", "server.port": "", "vdisk-id":
"/test.raw", "driver": "vxhs", "user": "ashish"}'
17396@1487908905.546890:vxhs_open_vdiskid Opening vdisk-id /test.raw
17396@1487908905.546969:vxhs_get_creds SecretID ashish, Password
asd888d989s   < NOTE!
17396@1487908905.546994:vxhs_open_hostinfo Adding host 127.0.0.1:
to BDRVVXHSState
17396@1487908905.568370:vxhs_get_vdisk_stat vDisk /test.raw stat ioctl
returned size 196616
read 131072/131072 bytes at offset 66000
128 KiB, 1 ops; 0.0017 sec (72.844 MiB/sec and 582.7506 ops/sec)
17396@1487908905.570917:vxhs_close Closing vdisk /test.raw
[root@rhel72ga-build-upstream qemu] 2017-02-23 20:01:45$

Example where username and password are missing:

[root@rhel72ga-build-upstream qemu] 2017-02-23 19:30:16$ ./qemu-io
--trace enable=vxhs* -c 'read 66000 128k' 'json:{"server.host":
"127.0.0.1", "server.port": "", "vdisk-id": "/test.raw", "driver":
"vxhs"}'
16120@1487907025.251167:vxhs_open_vdiskid Opening vdisk-id /test.raw
16120@1487907025.251245:vxhs_get_creds SecretID user, Password (null)
can't open device json:{"server.host": "127.0.0.1", "server.port":
"", "vdisk-id": "/test.raw", "driver": "vxhs"}: No secret with id
'user'  < NOTE!
[root@rhel72ga-build-upstream qemu] 2017-02-23 19:30:25$


diff --git a/block/vxhs.c b/block/vxhs.c
index 4f0633e..f3e70ce 100644
--- a/block/vxhs.c
+++ b/block/vxhs.c
@@ -17,6 +17,7 @@
 #include "qemu/uri.h"
 #include "qapi/error.h"
 #include "qemu/uuid.h"
+#include "crypto/secret.h"

 #define VXHS_OPT_FILENAME   "filename"
 #define VXHS_OPT_VDISK_ID   "vdisk-id"
@@ -136,6 +137,14 @@ static QemuOptsList runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "UUID of the VxHS vdisk",
 },
+{
+.name = "user",
+.type = QEMU_OPT_STRING,
+},
+{
+.name = "password",
+.type = QEMU_OPT_STRING,
+},
 { /* end of list */ }
 },
 };
@@ -257,6 +266,8 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
 const char *server_host_opt;
 char *str = NULL;
 int ret = 0;
+const char *user = NULL;
+const char *password = NULL;

 ret = vxhs_init_and_ref();
 if (ret < 0) {
@@ -320,6 +331,23 @@ static int vxhs_open(BlockDriverState *bs, QDict *options,
 goto out;
 }

+/* check if we got username via the options */
+user = qemu_opt_get(opts, "user");
+if (!user) {
+//trace_vxhs_get_creds(user, password);
+user = "user";
+//return;
+}
+
+password = qcrypto_secret_lookup_as_utf8(user, _err);
+if (local_err != NULL) {
+trace_vxhs_get_creds(user, password);
+qdict_del(backing_options, str);
+ret = -EINVAL;
+goto out;
+}
+trace_vxhs_get_creds(user, password);
+
 s->vdisk_hostinfo.host = g_strdup(server_host_opt);

 s->vdisk_hostinfo.port = g_ascii_strtoll(qemu_opt_get(tcp_opts,

Thanks,
Ashish

On Wed, Feb 22, 2017 at 6:44 AM, Jeff Cody  wrote:
> On Wed, Feb 22, 2017 at 02:22:30PM +, Daniel P. Berrange wrote:
>> On Wed, Feb 22, 2017 at 02:09:20PM +, Stefan Hajnoczi wrote:
>> > On Tue, Feb 21, 2017 at 11:33:53AM +, Daniel P. Berrange wrote:
>> > > On Tue, Feb 21, 2017 at 10:59:18AM +, Stefan Hajnoczi wrote:
>> > > > On Mon, Feb 20, 2017 at 03:34:57AM -0800, ashish mittal wrote:
>> > > > > On Mon, Feb 20, 2017 at 3:02 AM, Stefan Hajnoczi 
>> > > > >  wrote:
>> > > > > > On Sat, Feb 18, 2017 at 12:30:31AM +, Ketan Nilangekar wrote:
>> > > > > >> On 2/17/17, 1:42 PM, "Jeff Cody"  wrote:
>> > > > > >>
>> > > > > >> On Thu, Feb 16, 2017 at 02:24:19PM -0800, ashish mittal wrote:
>> > > > > >> > Hi,
>> > > > > >> >
>> > > > > >> > I am getting the following error with checkpatch.pl
>> > > > > >> >
>> > > > > >> > ERROR: externs should be avoided in .c files
>> > > > > >> > #78: FILE: block/vxhs.c:28:
>> > > > > >> > +QemuUUID qemu_uuid __attribute__ ((weak));
>> > > > > >> >
>> > > > > >> > Is there any way to get around this, or does it mean that I 
>> > > > > >> would have
>> > > > > >> > to add a vxhs.h just for this one entry?
>> > > > > >> >
>> > > > > >>
>> > > > > >> I remain skeptical on the use of the qemu_uuid as a way to 
>> > > > > >> select the TLS
>> > > > > >> cert.
>> > > > > >>
>> > > > > >> [ketan]
>> > > > > >> Is there another identity that can be used 

Re: [Qemu-devel] [PATCH V8 1/2] Add a new qmp command to start/stop replication

[Zhang Chen]

On 02/23/2017 11:31 PM, Eric Blake wrote:

On 02/23/2017 01:14 AM, Zhang Chen wrote:

We can call this qmp command to start/stop replication outside of qemu.
Like Xen colo need this function.

Signed-off-by: Zhang Chen 
Signed-off-by: Wen Congyang 
Reviewed-by: Eric Blake 
Reviewed-by: Stefano Stabellini 
Reviewed-by: zhanghailiang 

---
@@ -104,6 +106,27 @@ void colo_do_failover(MigrationState *s)
  }
  }
  
+void qmp_xen_set_replication(bool enable, bool primary,

+ bool has_failover, bool failover,
+ Error **errp)
+{
+ReplicationMode mode = primary ?
+   REPLICATION_MODE_PRIMARY :
+   REPLICATION_MODE_SECONDARY;
+
+if (has_failover && enable) {
+error_setg(errp, "Parameter 'failover' is only for"
+   " stopping replication");
+return;
+}
+
+if (enable) {
+replication_start_all(mode, errp);
+} else {
+replication_stop_all(failover, failover ? NULL : errp);

Technically, this is use of failover without first checking
has_failover; it would be safer if you inserted:

if (!has_failover) {
 failover = NULL;
}


OK, I will insert this code in next version.

Thanks
Zhang Chen


earlier in the function to be safe against any callers that don't go
through the QAPI-generated code.  Fortunately, our QAPI generated code
guarantees that failover is NULL when has_failover is false, so my R-b
can stand as-is if there is no other reason for a respin.


--
Thanks
Zhang Chen

Re: [Qemu-devel] [RFC v6] RBD: Add support readv,writev for rbd

[Jeff Cody]

On Tue, Feb 21, 2017 at 02:50:03PM +0800, jaze...@gmail.com wrote:
> From: tianqing 
> 
> Rbd can do readv and writev directly, so wo do not need to transform
> iov to buf or vice versa any more.
> 
> Signed-off-by: tianqing 
> ---


This is marked as an RFC still - is this a series you would like to see in
2.9?

Re: [Qemu-devel] [PATCH 0/2] block/nfs optimizations

[Jeff Cody]

On Fri, Feb 17, 2017 at 05:38:59PM +0100, Peter Lieven wrote:
> Peter Lieven (2):
>   block/nfs: convert to preadv / pwritev
>   block/nfs: try to avoid the bounce buffer in pwritev
> 
>  block/nfs.c | 50 --
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> -- 
> 1.9.1
> 

Thanks,

Applied to my block branch:

git://github.com/codyprime/qemu-kvm-jtc.git block

-Jeff

[Qemu-devel] [PATCH v4] mem-prealloc: reduce large guest start-up and migration time.

[Jitendra Kolhe]

Using "-mem-prealloc" option for a large guest leads to higher guest
start-up and migration time. This is because with "-mem-prealloc" option
qemu tries to map every guest page (create address translations), and
make sure the pages are available during runtime. virsh/libvirt by
default, seems to use "-mem-prealloc" option in case the guest is
configured to use huge pages. The patch tries to map all guest pages
simultaneously by spawning multiple threads. Currently limiting the
change to QEMU library functions on POSIX compliant host only, as we are
not sure if the problem exists on win32. Below are some stats with
"-mem-prealloc" option for guest configured to use huge pages.


Idle Guest  | Start-up time | Migration time

Guest stats with 2M HugePage usage - single threaded (existing code)

64 Core - 4TB   | 54m11.796s| 75m43.843s
64 Core - 1TB   | 8m56.576s | 14m29.049s
64 Core - 256GB | 2m11.245s | 3m26.598s

Guest stats with 2M HugePage usage - map guest pages using 8 threads

64 Core - 4TB   | 5m1.027s  | 34m10.565s
64 Core - 1TB   | 1m10.366s | 8m28.188s
64 Core - 256GB | 0m19.040s | 2m10.148s
---
Guest stats with 2M HugePage usage - map guest pages using 16 threads
---
64 Core - 4TB   | 1m58.970s | 31m43.400s
64 Core - 1TB   | 0m39.885s | 7m55.289s
64 Core - 256GB | 0m11.960s | 2m0.135s
---

Changed in v2:
 - modify number of memset threads spawned to min(smp_cpus, 16).
 - removed 64GB memory restriction for spawning memset threads.

Changed in v3:
 - limit number of threads spawned based on
   min(sysconf(_SC_NPROCESSORS_ONLN), 16, smp_cpus)
 - implement memset thread specific siglongjmp in SIGBUS signal_handler.

Changed in v4
 - remove sigsetjmp/siglongjmp and SIGBUS unblock/block for main thread
   as main thread no longer touches any pages.
 - simplify code my returning memset_thread_failed status from
   touch_all_pages.

Signed-off-by: Jitendra Kolhe 
---
 backends/hostmem.c   |   4 +-
 exec.c   |   2 +-
 include/qemu/osdep.h |   3 +-
 util/oslib-posix.c   | 108 +--
 util/oslib-win32.c   |   3 +-
 5 files changed, 94 insertions(+), 26 deletions(-)

diff --git a/backends/hostmem.c b/backends/hostmem.c
index 7f5de70..162c218 100644
--- a/backends/hostmem.c
+++ b/backends/hostmem.c
@@ -224,7 +224,7 @@ static void host_memory_backend_set_prealloc(Object *obj, 
bool value,
 void *ptr = memory_region_get_ram_ptr(>mr);
 uint64_t sz = memory_region_size(>mr);
 
-os_mem_prealloc(fd, ptr, sz, _err);
+os_mem_prealloc(fd, ptr, sz, smp_cpus, _err);
 if (local_err) {
 error_propagate(errp, local_err);
 return;
@@ -328,7 +328,7 @@ host_memory_backend_memory_complete(UserCreatable *uc, 
Error **errp)
  */
 if (backend->prealloc) {
 os_mem_prealloc(memory_region_get_fd(>mr), ptr, sz,
-_err);
+smp_cpus, _err);
 if (local_err) {
 goto out;
 }
diff --git a/exec.c b/exec.c
index 8b9ed73..53afcd2 100644
--- a/exec.c
+++ b/exec.c
@@ -1379,7 +1379,7 @@ static void *file_ram_alloc(RAMBlock *block,
 }
 
 if (mem_prealloc) {
-os_mem_prealloc(fd, area, memory, errp);
+os_mem_prealloc(fd, area, memory, smp_cpus, errp);
 if (errp && *errp) {
 goto error;
 }
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 56c9e22..fb1d22b 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -401,7 +401,8 @@ unsigned long qemu_getauxval(unsigned long type);
 
 void qemu_set_tty_echo(int fd, bool echo);
 
-void os_mem_prealloc(int fd, char *area, size_t sz, Error **errp);
+void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
+ Error **errp);
 
 int qemu_read_password(char *buf, int buf_size);
 
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index f631464..7e87c87 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -55,6 +55,21 @@
 #include "qemu/error-report.h"
 #endif
 
+#define MAX_MEM_PREALLOC_THREAD_COUNT (MIN(sysconf(_SC_NPROCESSORS_ONLN), 16))
+
+struct MemsetThread {
+char *addr;
+uint64_t numpages;
+uint64_t hpagesize;
+QemuThread pgthread;
+sigjmp_buf env;
+};
+typedef struct MemsetThread MemsetThread;
+
+static MemsetThread 

Re: [Qemu-devel] [PATCH] memory: make ram device read/write endian sensitive

[David Gibson]

On Thu, Feb 23, 2017 at 12:43:37PM +0100, Paolo Bonzini wrote:
> 
> 
> On 23/02/2017 12:34, Peter Maydell wrote:
> > On 23 February 2017 at 10:33, Paolo Bonzini  wrote:
> >>
> >>
> >> On 23/02/2017 11:23, Peter Maydell wrote:
> >>> On 23 February 2017 at 10:10, Paolo Bonzini  wrote:
>  On 23/02/2017 11:02, Peter Maydell wrote:
> > I'm really not convinced we need DEVICE_HOST_ENDIAN. RAM
> > areas should be target-endian (you can probably define
> > "target endianness" as "the endianness that RAM areas have".)
> 
>  This is not RAM.  This is MMIO, backed by a MMIO area in the host.
> >>>
> >>> Hmm, I see...the naming is a bit unfortunate if it's not RAM.
> >>
> >> Yeah, it's called like that because it is backed by a RAMBlock but it
> >> returns false for memory_access_is_direct.
> > 
> > We should probably update the doc comment to note that the
> > pointer is to host-endianness memory (and that this is not
> > like normal RAM which is target-endian)...
> 
> I wouldn't call it host-endianness memory, and I disagree that normal
> RAM is target-endian---in both cases it's just a bunch of bytes.

Right.  Really, endianness is not a property of the device - even less
so of RAM.  It's a property of an individual multibyte value and how
it is interpreted relative to individual byte addresses.

Really the declarations in the MRs are saying: assuming the guest
(software + hardware) does (big|little|target native) accesses on this
device, do the right swaps so we get host native multi-byte values
which match the original multibyte values the guest had in mind.

What we want for memory (both RAM and VFIO MMIO) is: don't even try to
preserve multi-byte values between host and guest views, just preserve
byte address order.  Tastes may vary as to whether you call that "host
endian" or "no endian" or "bag-o'-bytesian" or whatever.

> However, the access done by the MemoryRegionOps callbacks needs to match
> the endianness declared by the MemoryRegionOps themselves.
> 
> Paolo
> 
>  The
>  MemoryRegionOps read from the MMIO area (so the data has host
>  endianness) and do not do any further swap:
> 
>  data = *(uint16_t *)(mr->ram_block->host + addr);
> 
>  Here, the dereference is basically the same as ldl_he_p.
> 
>  If you wanted to make the MemoryRegion use DEVICE_NATIVE_ENDIAN, you'd
>  need to tswap around the access.  Or you can use ldl_le_p and
>  DEVICE_LITTLE_ENDIAN (this is what Yongji's patch open codes), or
>  ldl_be_p and DEVICE_BIG_ENDIAN.  They are all the same in the end.
> >>>
> >>> Using stl_p  in a DEVICE_NATIVE_ENDIAN MR would work too, right?
> >>> (This is how all the NATIVE_ENDIAN MRs in exec.c work.)
> >>
> >> Yes, it should, as long as the memcpy(...) of {ld,st}*_he_p is compiled
> >> to a single access, which should be the case.
> > 
> > ...and whichever of these approaches we take, we should have
> > a comment which notes that we are converting from the host
> > endianness memory to the endianness specified by the MemoryRegion
> > endianness attribute.
> > 
> > thanks
> > -- PMM
> > 
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] memory: make ram device read/write endian sensitive

[David Gibson]

On Fri, Feb 24, 2017 at 01:14:09AM +0800, Yongji Xie wrote:
> on 2017/2/24 0:15, Paolo Bonzini wrote:
> 
> > 
> > On 23/02/2017 17:08, Peter Maydell wrote:
> > > On 23 February 2017 at 15:58, Paolo Bonzini  wrote:
> > > > However, DEVICE_NATIVE_ENDIAN would have to be paired with tswap, which
> > > > the current code does not do, hence the bug.  To have no swap at all,
> > > > you'd need DEVICE_HOST_ENDIAN.
> > > Yes, I agree that the current ramdevice code has this bug (and
> > > that we can fix it by any of the various options).
> > Good. :)
> > 
> > > > > AIUI what we want for this VFIO case is "when the guest does
> > > > > a 32-bit write of 0x12345678 then the bytes are 0x12 0x34 0x56 0x78
> > > > > regardless of whether TARGET_BIG_ENDIAN or not".
> > > > No, I don't think so.  This is not specific to VFIO.  You can do it with
> > > > any device, albeit VFIO is currently the only one using ramd regions.
> > > The commit message in the patch that started this thread off
> > > says specifically that "VFIO PCI device is little endian".
> > > Is that wrong?
> > Yes, I think it's a red herring.  Hence my initial confusion, when I
> > asked "would Yongji's patch just work if it used DEVICE_BIG_ENDIAN and
> > beNN_to_cpu/cpu_to_beNN".
> > 
> 
> Thank you for the great discussion. I have a better understanding of the
> endianness now.:-)
> 
> And for the commit message, I was wrong to assume the same endianness
> as vfio. That's my fault. This bug should happen when target and host
> endianness are different regardless of the device endianness.
> 
> To fix it, introducing DEVICE_HOST_ENDIAN for the ram device seems to be
> more reasonable than other ways. I think I'll update the patch with this
> way.

I think this is basically the right approach, with the only caveat
being whether we want to call it "host endian" or something else.

> diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h
> index bd15853..eef74df 100644
> --- a/include/exec/cpu-common.h
> +++ b/include/exec/cpu-common.h
> @@ -36,6 +36,12 @@ enum device_endian {
>  DEVICE_LITTLE_ENDIAN,
>  };
> 
> +#if defined(HOST_WORDS_BIGENDIAN)
> +#define DEVICE_HOST_ENDIAN DEVICE_BIG_ENDIAN
> +#else
> +#define DEVICE_HOST_ENDIAN DEVICE_LITTLE_ENDIAN
> +#endif
> +
>  /* address in the RAM (different from a physical address) */
>  #if defined(CONFIG_XEN_BACKEND)
>  typedef uint64_t ram_addr_t;
> diff --git a/memory.c b/memory.c
> index ed8b5aa..17cfada 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1180,7 +1180,7 @@ static void memory_region_ram_device_write(void
> *opaque, hwaddr addr,
>  static const MemoryRegionOps ram_device_mem_ops = {
>  .read = memory_region_ram_device_read,
>  .write = memory_region_ram_device_write,
> -.endianness = DEVICE_NATIVE_ENDIAN,
> +.endianness = DEVICE_HOST_ENDIAN,
>  .valid = {
>  .min_access_size = 1,
>  .max_access_size = 8,
> 
> Thanks,
> Yongji
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v4 06/15] target/ppc: remove xer split-out flags(so, ov, ca)

[David Gibson]

On Fri, Feb 24, 2017 at 06:18:22AM +0530, Nikunj A Dadhania wrote:
> Richard Henderson  writes:
> 
> > On 02/24/2017 06:56 AM, Nikunj A Dadhania wrote:
> >> Now get rid all the split out variables so, ca, ov. After this patch,
> >> all the bits are stored in CPUPPCState::xer at appropriate places.
> >>
> >> Signed-off-by: Nikunj A Dadhania 
> >> ---
> >>  target/ppc/cpu.c|   8 +---
> >>  target/ppc/cpu.h|  26 ++--
> >>  target/ppc/int_helper.c |  12 +++---
> >>  target/ppc/translate.c  | 106 
> >> +---
> >>  4 files changed, 78 insertions(+), 74 deletions(-)
> >
> > I do not think this is a good direction to take this.
> 
> Hmm, any particular reason?

Right, I suggested this, but based only a suspicion that the split
variables weren't worth the complexity.  I'm happy to be corrected by
someone with better knowledge of TCG, but it'd be nice to know why.

> I can send back the v3 with suggested changes dropping the xer split out
> changes.
> 
> Regards
> Nikunj
> 
> 

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] kvm bug in __rmap_clear_dirty during live migration

[Herongguang (Stephen)]

On 2017/2/22 22:43, Paolo Bonzini wrote:



On 22/02/2017 14:31, Chris Friesen wrote:




Can you reproduce it with kernel 4.8+?  I'm suspecting commmit
4e59516a12a6 ("kvm: vmx: ensure VMCS is current while enabling PML",
2016-07-14) to be the fix.


I can't easily try with a newer kernel, the software package we're using
has kernel patches that would have to be ported.

I'm at a conference, don't really have time to set up a pair of test
machines from scratch with a custom kernel.


Hopefully Gaohuai and Rongguang can help with this too.

Paolo

.


Yes, we are looking into and testing this.

I think this can result in any memory corruption, if VM1 writes its
PML buffer into VM2’s VMCS (since sched_in/sched_out notifier of VM1
is not registered yet), then VM1 is destroyed (hence its PML buffer
is freed back to kernel), after that, VM2 starts migration, so CPU
logs VM2’s dirty GFNS into a freed memory, results in any memory corruption.

As its severity, this commit 
(http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=4e59516a12a6ef6dcb660cb3a3f70c64bd60cfec)
is eligible to back port to kernel stable.

Re: [Qemu-devel] [PATCH] intel_iommu: make sure its init before PCI dev

[Peter Xu]

On Fri, Feb 24, 2017 at 01:21:52AM +0200, Michael S. Tsirkin wrote:
> On Wed, Feb 22, 2017 at 01:49:25PM +0800, Peter Xu wrote:
> > Intel vIOMMU devices are created with "-device" parameter, while here
> > actually we need to make sure this device will be created before some
> > other PCI devices (like vfio-pci devices) so that we know iommu_fn will
> > be setup correctly before realizations of those PCI devices.
> > Here we do explicit check to make sure intel-iommu device will be inited
> > before all the rest of the PCI devices. This is done by checking against
> > the devices dangled under current root PCIe bus and we should see
> > nothing there besides integrated ICH9 ones.
> > 
> > If the user violated this rule, we abort the program.
> > 
> > Maybe one day we will be able to manage the ordering of device
> > initialization, and then we can grant VT-d devices a higher init
> > priority. But before that, let's have this explicit check to make sure
> > of it.
> > 
> > Signed-off-by: Peter Xu 
> > ---
> >  hw/i386/intel_iommu.c | 40 
> >  1 file changed, 40 insertions(+)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 22d8226..db74124 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -31,6 +31,7 @@
> >  #include "hw/i386/apic-msidef.h"
> >  #include "hw/boards.h"
> >  #include "hw/i386/x86-iommu.h"
> > +#include "hw/i386/ich9.h"
> >  #include "hw/pci-host/q35.h"
> >  #include "sysemu/kvm.h"
> >  #include "hw/i386/apic_internal.h"
> > @@ -2560,6 +2561,41 @@ static bool vtd_decide_config(IntelIOMMUState *s, 
> > Error **errp)
> >  return true;
> >  }
> >  
> > +static bool vtd_has_inited_pci_devices(PCIBus *bus, Error **errp)
> > +{
> > +int i;
> > +uint8_t func;
> > +
> > +/* We check against root bus */
> > +assert(bus && pci_bus_is_root(bus));
> > +
> > +/*
> > + * We need to make sure vIOMMU device is created before other PCI
> > + * devices other than the integrated ICH9 ones,
> 
> 
> Why wouldn't this apply to integrated ICH9 ones?

Because these integrated devices are created along with q35 in
pc_q35_init() with:

pc_vga_init(isa_bus, host_bus);
pc_nic_init(isa_bus, host_bus);

which is definitely ahead of the general device init routines,
including VT-d device.

> 
> 
> > so that they can
> > + * get correct iommu_fn setup even during its realize(). Some
> > + * devices (e.g., vfio-pci) will need a correct iommu_fn to work.
> 
> If there's something special about vfio devices, then just check
> the device type.

Yeah. I think that's the most feasible way to do this for now. If Alex
won't disagree, I can prepare a new version.

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH 2/2] vmxnet3: VMStatify rx/tx q_descr and int_state

[Jason Wang]

On 2017年02月01日 03:44, Dr. David Alan Gilbert wrote:

* Dmitry Fleytman (dmi...@daynix.com) wrote:

On 3 Jan 2017, at 21:40 PM, Dr. David Alan Gilbert  wrote:

* Dmitry Fleytman (dmi...@daynix.com  ) wrote:

On 16 Dec 2016, at 14:19 PM, Dr. David Alan Gilbert  wrote:

* Dr. David Alan Gilbert (git) (dgilb...@redhat.com) wrote:

From: "Dr. David Alan Gilbert"

Fairly simple mechanical conversion of all fields.

TODO
The problem is vmxnet3-ring size/cell_size/next are declared as size_t
but written as 32bit.

Oops, I should have removed that warning in the commit message when
I added the 1st patch in.


Acked-by: Dmitry Fleytman

Any reason for Acked-by rather than Reviewed-by ?
Are you going to pull those?

Hi,

There is no specific reason, the patch is OK as for me.
Reviewed-by: Dmitry Fleytman

Jason, would you pick up this patch?

Hi Jason,
   Did you see 

Dave



Applied to -net, thanks.

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

On Fri, Feb 24, 2017 at 09:49:11AM +1100, David Gibson wrote:
> On Thu, Feb 23, 2017 at 08:52:01AM -0600, Eric Blake wrote:
> > On 02/23/2017 06:33 AM, Peter Maydell wrote:
> > > What's the right way to use libfdt's fdt_setprop to set a property
> > > to have an empty value? At the moment in QEMU we tend to use
> > >   fdt_setprop(fdt, nodeoffset, "propertyname", NULL, 0);
> 
> That's what I've always used..
> 
> > > and git grep 'fdt_setprop.*NULL' produces examples of this usage in
> > > PPC and ARM fdt creation code.
> > > 
> > > However the fdt_setprop() documentation doesn't document that a NULL
> > > value pointer is OK if the length is 0, and indeed the implementation
> > > unconditionally calls memcpy(prop->data, val, len), which is
> > > undefined behaviour, and warned about by clang sanitizers if you
> > > build libfdt with them:
> > >  dtc/libfdt/fdt_rw.c:288:21: runtime error: null pointer passed
> > >as argument 2, which is declared to never be null
> 
> ..but indeed that isn't really technically right.
> 
> > > So what's the best thing to do here? I can't offhand think of a
> > > non-ugly/non-confusing way to pass a valid pointer here...
> > 
> > Does fdt_setprop(fdt, nodeoffset, "propertyname", "", 0) do the right thing?
> 
> That should work, but it's a bit clunky.
> 
> I guess I should add an fdt_setprop_empty() to libfdt.

Ok, I've pushed libfdt upstream patches to (a) make passing NULL to
setprop() with zero length explicitly safe and (b) add an
fdt_setprop_empty() helper macro.  Do you want me to make a pullreq to
update the qemu submodule?

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v4 06/15] target/ppc: remove xer split-out flags(so, ov, ca)

[Nikunj A Dadhania]

Richard Henderson  writes:

> On 02/24/2017 06:56 AM, Nikunj A Dadhania wrote:
>> Now get rid all the split out variables so, ca, ov. After this patch,
>> all the bits are stored in CPUPPCState::xer at appropriate places.
>>
>> Signed-off-by: Nikunj A Dadhania 
>> ---
>>  target/ppc/cpu.c|   8 +---
>>  target/ppc/cpu.h|  26 ++--
>>  target/ppc/int_helper.c |  12 +++---
>>  target/ppc/translate.c  | 106 
>> +---
>>  4 files changed, 78 insertions(+), 74 deletions(-)
>
> I do not think this is a good direction to take this.

Hmm, any particular reason?

I can send back the v3 with suggested changes dropping the xer split out
changes.

Regards
Nikunj

Re: [Qemu-devel] [PATCH v4 02/15] target/ppc: update ov flag from remaining paths

[Nikunj A Dadhania]

Richard Henderson  writes:

> On 02/24/2017 06:56 AM, Nikunj A Dadhania wrote:
>> @@ -320,22 +320,24 @@ target_ulong helper_divo(CPUPPCState *env, 
>> target_ulong arg1,
>>   target_ulong arg2)
>>  {
>>  uint64_t tmp = (uint64_t)arg1 << 32 | env->spr[SPR_MQ];
>> +int ov;
>>
>>  if (((int32_t)tmp == INT32_MIN && (int32_t)arg2 == (int32_t)-1) ||
>>  (int32_t)arg2 == 0) {
>> -env->so = env->ov = 1;
>> +ov = 1;
>>  env->spr[SPR_MQ] = 0;
>>  return INT32_MIN;
>>  } else {
>>  env->spr[SPR_MQ] = tmp % arg2;
>>  tmp /= (int32_t)arg2;
>>  if ((int32_t)tmp != tmp) {
>> -env->so = env->ov = 1;
>> +ov = 1;
>>  } else {
>> -env->ov = 0;
>> +ov = 0;
>>  }
>>  return tmp;
>>  }
>> +helper_update_ov_legacy(env, ov);
>>  }
>>
>
> You're attempting to run the helper after "return".

Right, will correct it.

Regards
Nikunj

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[Nikunj Dadhania]

On 24 February 2017 at 04:23, David Gibson  wrote:
> On Fri, Feb 24, 2017 at 09:34:32AM +1100, Richard Henderson wrote:
>> On 02/23/2017 05:40 PM, Nikunj A Dadhania wrote:
>> > Richard Henderson  writes:
>> > > These functions are becoming quite large.  Are they performance critical 
>> > > enough
>> > > that they need to stay as inline code, or should they be moved to 
>> > > helpers and
>> > > share code with cpu_read/write_xer?
>> >
>> > Just to boot to login prompt, these are the numbers for gen_read/write_xer:
>> >
>> > helper_myprint - rd_count 231103, wr_count 68897
>> >
>> > And it keeps on incrementing, maybe scope of optimization here.
>>
>> That's not very large considering the total number of instructions executed
>> during a boot to prompt.
>>
>> Thoughts, David?
>
> Hm, I'm not clear if that's the number of executions, or the number of
> translations.

That is number of executions.

Regards
Nikunj

Re: [Qemu-devel] [PULL v2 8/8] hw/mips: MIPS Boston board support

[Yongbok Kim]

On 23/02/2017 18:09, Peter Maydell wrote:
> On 22 February 2017 at 00:21, Yongbok Kim  wrote:
>> From: Paul Burton 
>>
>> Introduce support for emulating the MIPS Boston development board. The
>> Boston board is built around an FPGA & 3 PCIe controllers, one of which
>> is connected to an Intel EG20T Platform Controller Hub. It is used
>> during the development & debug of new CPUs and the software intended to
>> run on them, and is essentially the successor to the older MIPS Malta
>> board.
>>
>> This patch does not implement the EG20T, instead connecting an already
>> supported ICH-9 AHCI controller. Whilst this isn't accurate it's enough
>> for typical stock Boston software (eg. Linux kernels) to work with hard
>> disks given that both the ICH-9 & EG20T implement the AHCI
>> specification.
>>
>> Boston boards typically boot kernels in the FIT image format, and this
>> patch will treat kernels provided to QEMU as such. When loading a kernel
>> directly, the board code will generate minimal firmware much as the
>> Malta board code does. This firmware will set up the CM, CPC & GIC
>> register base addresses then set argument registers & jump to the kernel
>> entry point. Alternatively, bootloader code may be loaded using the bios
>> argument in which case no firmware will be generated & execution will
>> proceed from the start of the boot code at the default MIPS boot
>> exception vector (offset 0x1fc0 into (c)kseg1).
>>
>> Currently real Boston boards are always used with FPGA bitfiles that
>> include a Global Interrupt Controller (GIC), so the interrupt
>> configuration is only defined for such cases. Therefore the board will
>> only allow use of CPUs which implement the CPS components, including the
>> GIC, and will otherwise exit with a message.
>>
>> +static void boston_mach_class_init(MachineClass *mc)
>> +{
>> +mc->desc = "MIPS Boston";
>> +mc->init = boston_mach_init;
>> +mc->block_default_type = IF_IDE;
>> +mc->default_ram_size = 2 * G_BYTE;
>> +mc->max_cpus = 16;
>> +}
> 
> I only just noticed, but this breaks "make check" on 32-bit hosts:
> 
> TEST: tests/qom-test... (pid=16740)
>   /mips64el/qom/boston:
> qemu-system-mips64el: at most 2047 MB RAM can be simulated
> Broken pipe
> FAIL
> 
> because vl.c has
> /* On 32-bit hosts, QEMU is limited by virtual address space */
> if (ram_size > (2047 << 20) && HOST_LONG_BITS == 32) {
> error_report("at most 2047 MB RAM can be simulated");
> exit(1);
> }
> 
> Unfortunately I didn't notice before I pushed your pull request.
> Luckily this is the last patch in the pullreq, so I can just
> revert this one commit (d3473e147a754e999718bf6fcb015d9978c6a1ee)...
> 
> Please fix the board model patch and resend it. You'll need
> the board to default to a ram size that works on all hosts.
> 
> (I'm taking the revert option because we're getting pretty
> close to freeze and I have a bunch of pull requests in the
> queue that I'd prefer not to delay while we fix this up.)
> 
> thanks
> -- PMM
> 

My apologies for the inconvenience! I missed to check it on the 32-bit hosts.

I am not seeing the revert commit on the master?
I've sent a modified patch with the fix and a pull request containing
reverting the commit by myself and the updated Boston patch.

Regards,
Yongbok

[Qemu-devel] [PULL 0/2] target-mips queue

[Yongbok Kim]

The following changes since commit 10f25e4844cb9b3f02fb032f88051dd5b65b4206:

  Merge remote-tracking branch 'remotes/yongbok/tags/mips-20170222' into 
staging (2017-02-23 09:59:40 +)

are available in the git repository at:

  git://github.com/yongbok/upstream-qemu.git tags/mips-20170224

for you to fetch changes up to 0288d4485392832b25e40551433377a36d56dd8c:

  hw/mips: MIPS Boston board support (2017-02-24 00:04:32 +)


MIPS patches 2017-02-24

Changes:
* Revert the boston model commit which breaks "make check" on 32-bit hosts
* Add the boston model with fixing the issue.



Paul Burton (1):
  hw/mips: MIPS Boston board support

Yongbok Kim (1):
  Revert "hw/mips: MIPS Boston board support"

 hw/mips/boston.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.7.4

[Qemu-devel] [PULL 2/2] hw/mips: MIPS Boston board support

[Yongbok Kim]

From: Paul Burton 

Introduce support for emulating the MIPS Boston development board. The
Boston board is built around an FPGA & 3 PCIe controllers, one of which
is connected to an Intel EG20T Platform Controller Hub. It is used
during the development & debug of new CPUs and the software intended to
run on them, and is essentially the successor to the older MIPS Malta
board.

This patch does not implement the EG20T, instead connecting an already
supported ICH-9 AHCI controller. Whilst this isn't accurate it's enough
for typical stock Boston software (eg. Linux kernels) to work with hard
disks given that both the ICH-9 & EG20T implement the AHCI
specification.

Boston boards typically boot kernels in the FIT image format, and this
patch will treat kernels provided to QEMU as such. When loading a kernel
directly, the board code will generate minimal firmware much as the
Malta board code does. This firmware will set up the CM, CPC & GIC
register base addresses then set argument registers & jump to the kernel
entry point. Alternatively, bootloader code may be loaded using the bios
argument in which case no firmware will be generated & execution will
proceed from the start of the boot code at the default MIPS boot
exception vector (offset 0x1fc0 into (c)kseg1).

Currently real Boston boards are always used with FPGA bitfiles that
include a Global Interrupt Controller (GIC), so the interrupt
configuration is only defined for such cases. Therefore the board will
only allow use of CPUs which implement the CPS components, including the
GIC, and will otherwise exit with a message.

Signed-off-by: Paul Burton 
Reviewed-by: Yongbok Kim 
[yongbok@imgtec.com:
  isolated boston machine support for mips64el.
  updated for recent Chardev changes.
  ignore missing bios/kernel for qtest.
  added default -drive to if=ide explicitly.
  changed default memory size into 1G due to make check failure
  on 32-bit hosts]
Signed-off-by: Yongbok Kim 
---
 configure|   2 +-
 default-configs/mips64el-softmmu.mak |   3 +
 hw/mips/Makefile.objs|   1 +
 hw/mips/boston.c | 577 +++
 4 files changed, 582 insertions(+), 1 deletion(-)
 create mode 100644 hw/mips/boston.c

diff --git a/configure b/configure
index 4b68861..8e8f18d 100755
--- a/configure
+++ b/configure
@@ -3378,7 +3378,7 @@ fi
 fdt_required=no
 for target in $target_list; do
   case $target in
-aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-softmmu)
+
aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-softmmu|mips64el-softmmu)
   fdt_required=yes
 ;;
   esac
diff --git a/default-configs/mips64el-softmmu.mak 
b/default-configs/mips64el-softmmu.mak
index 485e218..c2ae313 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -10,3 +10,6 @@ CONFIG_JAZZ=y
 CONFIG_G364FB=y
 CONFIG_JAZZ_LED=y
 CONFIG_VT82C686=y
+CONFIG_MIPS_BOSTON=y
+CONFIG_FITLOADER=y
+CONFIG_PCI_XILINX=y
diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs
index 9352a1c..48cd2ef 100644
--- a/hw/mips/Makefile.objs
+++ b/hw/mips/Makefile.objs
@@ -4,3 +4,4 @@ obj-$(CONFIG_JAZZ) += mips_jazz.o
 obj-$(CONFIG_FULONG) += mips_fulong2e.o
 obj-y += gt64xxx_pci.o
 obj-$(CONFIG_MIPS_CPS) += cps.o
+obj-$(CONFIG_MIPS_BOSTON) += boston.o
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
new file mode 100644
index 000..83f7b82
--- /dev/null
+++ b/hw/mips/boston.c
@@ -0,0 +1,577 @@
+/*
+ * MIPS Boston development board emulation.
+ *
+ * Copyright (c) 2016 Imagination Technologies
+ *
+ * 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 "qemu-common.h"
+
+#include "exec/address-spaces.h"
+#include "hw/boards.h"
+#include "hw/char/serial.h"
+#include "hw/hw.h"
+#include "hw/ide/pci.h"
+#include "hw/ide/ahci.h"
+#include "hw/loader.h"
+#include "hw/loader-fit.h"
+#include "hw/mips/cps.h"
+#include "hw/mips/cpudevs.h"
+#include "hw/pci-host/xilinx-pcie.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qemu/log.h"
+#include "sysemu/char.h"
+#include "sysemu/device_tree.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/qtest.h"
+
+#include 
+
+#define 

[Qemu-devel] [PULL 1/2] Revert "hw/mips: MIPS Boston board support"

[Yongbok Kim]

This reverts commit d3473e147a754e999718bf6fcb015d9978c6a1ee.

The commit breaks "make check" on 32-bit hosts.

Signed-off-by: Yongbok Kim 
---
 configure|   2 +-
 default-configs/mips64el-softmmu.mak |   3 -
 hw/mips/Makefile.objs|   1 -
 hw/mips/boston.c | 577 ---
 4 files changed, 1 insertion(+), 582 deletions(-)
 delete mode 100644 hw/mips/boston.c

diff --git a/configure b/configure
index 8e8f18d..4b68861 100755
--- a/configure
+++ b/configure
@@ -3378,7 +3378,7 @@ fi
 fdt_required=no
 for target in $target_list; do
   case $target in
-
aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-softmmu|mips64el-softmmu)
+aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-softmmu)
   fdt_required=yes
 ;;
   esac
diff --git a/default-configs/mips64el-softmmu.mak 
b/default-configs/mips64el-softmmu.mak
index c2ae313..485e218 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -10,6 +10,3 @@ CONFIG_JAZZ=y
 CONFIG_G364FB=y
 CONFIG_JAZZ_LED=y
 CONFIG_VT82C686=y
-CONFIG_MIPS_BOSTON=y
-CONFIG_FITLOADER=y
-CONFIG_PCI_XILINX=y
diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs
index 48cd2ef..9352a1c 100644
--- a/hw/mips/Makefile.objs
+++ b/hw/mips/Makefile.objs
@@ -4,4 +4,3 @@ obj-$(CONFIG_JAZZ) += mips_jazz.o
 obj-$(CONFIG_FULONG) += mips_fulong2e.o
 obj-y += gt64xxx_pci.o
 obj-$(CONFIG_MIPS_CPS) += cps.o
-obj-$(CONFIG_MIPS_BOSTON) += boston.o
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
deleted file mode 100644
index ce43289..000
--- a/hw/mips/boston.c
+++ /dev/null
@@ -1,577 +0,0 @@
-/*
- * MIPS Boston development board emulation.
- *
- * Copyright (c) 2016 Imagination Technologies
- *
- * 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 "qemu-common.h"
-
-#include "exec/address-spaces.h"
-#include "hw/boards.h"
-#include "hw/char/serial.h"
-#include "hw/hw.h"
-#include "hw/ide/pci.h"
-#include "hw/ide/ahci.h"
-#include "hw/loader.h"
-#include "hw/loader-fit.h"
-#include "hw/mips/cps.h"
-#include "hw/mips/cpudevs.h"
-#include "hw/pci-host/xilinx-pcie.h"
-#include "qapi/error.h"
-#include "qemu/cutils.h"
-#include "qemu/error-report.h"
-#include "qemu/log.h"
-#include "sysemu/char.h"
-#include "sysemu/device_tree.h"
-#include "sysemu/sysemu.h"
-#include "sysemu/qtest.h"
-
-#include 
-
-#define TYPE_MIPS_BOSTON "mips-boston"
-#define BOSTON(obj) OBJECT_CHECK(BostonState, (obj), TYPE_MIPS_BOSTON)
-
-typedef struct {
-SysBusDevice parent_obj;
-
-MachineState *mach;
-MIPSCPSState *cps;
-SerialState *uart;
-
-CharBackend lcd_display;
-char lcd_content[8];
-bool lcd_inited;
-
-hwaddr kernel_entry;
-hwaddr fdt_base;
-} BostonState;
-
-enum boston_plat_reg {
-PLAT_FPGA_BUILD = 0x00,
-PLAT_CORE_CL= 0x04,
-PLAT_WRAPPER_CL = 0x08,
-PLAT_SYSCLK_STATUS  = 0x0c,
-PLAT_SOFTRST_CTL= 0x10,
-#define PLAT_SOFTRST_CTL_SYSRESET   (1 << 4)
-PLAT_DDR3_STATUS= 0x14,
-#define PLAT_DDR3_STATUS_LOCKED (1 << 0)
-#define PLAT_DDR3_STATUS_CALIBRATED (1 << 2)
-PLAT_PCIE_STATUS= 0x18,
-#define PLAT_PCIE_STATUS_PCIE0_LOCKED   (1 << 0)
-#define PLAT_PCIE_STATUS_PCIE1_LOCKED   (1 << 8)
-#define PLAT_PCIE_STATUS_PCIE2_LOCKED   (1 << 16)
-PLAT_FLASH_CTL  = 0x1c,
-PLAT_SPARE0 = 0x20,
-PLAT_SPARE1 = 0x24,
-PLAT_SPARE2 = 0x28,
-PLAT_SPARE3 = 0x2c,
-PLAT_MMCM_DIV   = 0x30,
-#define PLAT_MMCM_DIV_CLK0DIV_SHIFT 0
-#define PLAT_MMCM_DIV_INPUT_SHIFT   8
-#define PLAT_MMCM_DIV_MUL_SHIFT 16
-#define PLAT_MMCM_DIV_CLK1DIV_SHIFT 24
-PLAT_BUILD_CFG  = 0x34,
-#define PLAT_BUILD_CFG_IOCU_EN  (1 << 0)
-#define PLAT_BUILD_CFG_PCIE0_EN (1 << 1)
-#define PLAT_BUILD_CFG_PCIE1_EN (1 << 2)
-#define PLAT_BUILD_CFG_PCIE2_EN (1 << 3)
-PLAT_DDR_CFG= 0x38,
-#define PLAT_DDR_CFG_SIZE   (0xf << 0)
-#define PLAT_DDR_CFG_MHZ(0xfff << 4)
-PLAT_NOC_PCIE0_ADDR = 0x3c,
-PLAT_NOC_PCIE1_ADDR = 0x40,
-PLAT_NOC_PCIE2_ADDR = 0x44,
-PLAT_SYS_CTL= 0x48,
-};
-
-static void boston_lcd_event(void *opaque, int event)
-{
-BostonState *s = opaque;
-   

Re: [Qemu-devel] [PATCH 13/29] 9pfs: local: remove: don't follow symlinks

[Greg Kurz]

On Thu, 23 Feb 2017 14:23:15 +
Stefan Hajnoczi  wrote:

> On Mon, Feb 20, 2017 at 03:41:00PM +0100, Greg Kurz wrote:
> > +dirfd = local_opendir_nofollow(ctx, dirpath);
> > +if (dirfd) {
> > +goto out;
> >  }
> >  
> > -buffer = rpath(ctx, path);
> > -err = remove(buffer);
> > -g_free(buffer);
> > +if (fstatat(dirfd, path, , AT_SYMLINK_NOFOLLOW) < 0) {
> > +goto err_out;
> > +}
> > +
> > +if (S_ISDIR(stbuf.st_mode)) {
> > +flags |= AT_REMOVEDIR;
> > +}
> > +
> > +err = local_unlinkat_common(ctx, dirfd, name, flags);  
> 
> The alternative is optimistically skip fstat but then do:
> 
>   if (err == EISDIR) {

It would be err == -1 && errno == EISDIR actually.

>   err = local_unlinkat_common(ctx, dirfd, name, flags | AT_REMOVEDIR);
>   }
> 
> It might be faster.
> 

This would work for passthrough and mapped modes indeed. For mapped-file
mode, things are more complicated though. If path points to a directory
and we call local_unlinkat_common() without AT_REMOVEDIR, it won't unlink
the metadata directory and unlinkat() will fail with ENOENT because
the directory isn't empty... I'd rather try to optimize in a followup
patch later to avoid the extra complexity.

> Reviewed-by: Stefan Hajnoczi 



pgpLfkfmBiM8T.pgp
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] hw/mips: MIPS Boston board support

[Yongbok Kim]

From: Paul Burton 

Introduce support for emulating the MIPS Boston development board. The
Boston board is built around an FPGA & 3 PCIe controllers, one of which
is connected to an Intel EG20T Platform Controller Hub. It is used
during the development & debug of new CPUs and the software intended to
run on them, and is essentially the successor to the older MIPS Malta
board.

This patch does not implement the EG20T, instead connecting an already
supported ICH-9 AHCI controller. Whilst this isn't accurate it's enough
for typical stock Boston software (eg. Linux kernels) to work with hard
disks given that both the ICH-9 & EG20T implement the AHCI
specification.

Boston boards typically boot kernels in the FIT image format, and this
patch will treat kernels provided to QEMU as such. When loading a kernel
directly, the board code will generate minimal firmware much as the
Malta board code does. This firmware will set up the CM, CPC & GIC
register base addresses then set argument registers & jump to the kernel
entry point. Alternatively, bootloader code may be loaded using the bios
argument in which case no firmware will be generated & execution will
proceed from the start of the boot code at the default MIPS boot
exception vector (offset 0x1fc0 into (c)kseg1).

Currently real Boston boards are always used with FPGA bitfiles that
include a Global Interrupt Controller (GIC), so the interrupt
configuration is only defined for such cases. Therefore the board will
only allow use of CPUs which implement the CPS components, including the
GIC, and will otherwise exit with a message.

Signed-off-by: Paul Burton 
Reviewed-by: Yongbok Kim 
[yongbok@imgtec.com:
  isolated boston machine support for mips64el.
  updated for recent Chardev changes.
  ignore missing bios/kernel for qtest.
  added default -drive to if=ide explicitly.
  changed default memory size into 1G due to make check failure
  on 32-bit hosts]
Signed-off-by: Yongbok Kim 
---
 configure|   2 +-
 default-configs/mips64el-softmmu.mak |   3 +
 hw/mips/Makefile.objs|   1 +
 hw/mips/boston.c | 577 +++
 4 files changed, 582 insertions(+), 1 deletion(-)
 create mode 100644 hw/mips/boston.c

diff --git a/configure b/configure
index 4b68861..8e8f18d 100755
--- a/configure
+++ b/configure
@@ -3378,7 +3378,7 @@ fi
 fdt_required=no
 for target in $target_list; do
   case $target in
-aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-softmmu)
+
aarch64*-softmmu|arm*-softmmu|ppc*-softmmu|microblaze*-softmmu|mips64el-softmmu)
   fdt_required=yes
 ;;
   esac
diff --git a/default-configs/mips64el-softmmu.mak 
b/default-configs/mips64el-softmmu.mak
index 485e218..c2ae313 100644
--- a/default-configs/mips64el-softmmu.mak
+++ b/default-configs/mips64el-softmmu.mak
@@ -10,3 +10,6 @@ CONFIG_JAZZ=y
 CONFIG_G364FB=y
 CONFIG_JAZZ_LED=y
 CONFIG_VT82C686=y
+CONFIG_MIPS_BOSTON=y
+CONFIG_FITLOADER=y
+CONFIG_PCI_XILINX=y
diff --git a/hw/mips/Makefile.objs b/hw/mips/Makefile.objs
index 9352a1c..48cd2ef 100644
--- a/hw/mips/Makefile.objs
+++ b/hw/mips/Makefile.objs
@@ -4,3 +4,4 @@ obj-$(CONFIG_JAZZ) += mips_jazz.o
 obj-$(CONFIG_FULONG) += mips_fulong2e.o
 obj-y += gt64xxx_pci.o
 obj-$(CONFIG_MIPS_CPS) += cps.o
+obj-$(CONFIG_MIPS_BOSTON) += boston.o
diff --git a/hw/mips/boston.c b/hw/mips/boston.c
new file mode 100644
index 000..83f7b82
--- /dev/null
+++ b/hw/mips/boston.c
@@ -0,0 +1,577 @@
+/*
+ * MIPS Boston development board emulation.
+ *
+ * Copyright (c) 2016 Imagination Technologies
+ *
+ * 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 "qemu-common.h"
+
+#include "exec/address-spaces.h"
+#include "hw/boards.h"
+#include "hw/char/serial.h"
+#include "hw/hw.h"
+#include "hw/ide/pci.h"
+#include "hw/ide/ahci.h"
+#include "hw/loader.h"
+#include "hw/loader-fit.h"
+#include "hw/mips/cps.h"
+#include "hw/mips/cpudevs.h"
+#include "hw/pci-host/xilinx-pcie.h"
+#include "qapi/error.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qemu/log.h"
+#include "sysemu/char.h"
+#include "sysemu/device_tree.h"
+#include "sysemu/sysemu.h"
+#include "sysemu/qtest.h"
+
+#include 
+
+#define 

Re: [Qemu-devel] [PATCH] memory: make ram device read/write endian sensitive

[Paul Mackerras]

On Thu, Feb 23, 2017 at 03:29:53PM +, Peter Maydell wrote:
> On 23 February 2017 at 15:21, Paolo Bonzini  wrote:
> >
> >
> > On 23/02/2017 15:35, Peter Maydell wrote:
> >> On 23 February 2017 at 12:53, Paolo Bonzini  wrote:
> >>>
> >>>
> >>> On 23/02/2017 13:26, Peter Maydell wrote:
>  On 23 February 2017 at 11:43, Paolo Bonzini  wrote:
> > On 23/02/2017 12:34, Peter Maydell wrote:
> >> We should probably update the doc comment to note that the
> >> pointer is to host-endianness memory (and that this is not
> >> like normal RAM which is target-endian)...
> >
> > I wouldn't call it host-endianness memory, and I disagree that normal
> > RAM is target-endian---in both cases it's just a bunch of bytes.
> >
> > However, the access done by the MemoryRegionOps callbacks needs to match
> > the endianness declared by the MemoryRegionOps themselves.
> 
>  Well, if the guest stores a bunch of integers to the memory, which
>  way round do you see them when you look at the bunch of bytes?
> >>>
> >>> You see them in whatever endianness the guest used.
> >>
> >> I'm confused. I said "normal RAM and this ramdevice memory are
> >> different", and you seem to be saying they're the same. I don't
> >> think they are (in particular I think with a BE guest on an
> >> LE host they'll look different).
> >
> > No, they look entirely the same.  The only difference is that they go
> > through MemoryRegionOps instead of memcpy.
> 
> Then we have a different problem, because the thing this patch
> is claiming to fix is that the memory the device is backed by
> (from vfio) is little-endian and we're not accessing it right.
> 
> RAM of the usual sort is target-endian (by which I mean "when the guest
> does a write of 32-bits 0x12345678, and you look at the memory byte
> by byte then the order of bytes is either 0x12 0x34 0x56 0x78 if
> TARGET_LITTLE_ENDIAN or 0x78 0x56 0x34 0x12 if TARGET_BIG_ENDIAN").
> 
> AIUI what we want for this VFIO case is "when the guest does
> a 32-bit write of 0x12345678 then the bytes are 0x12 0x34 0x56 0x78
> regardless of whether TARGET_BIG_ENDIAN or not".

At least in the case of KVM and MMIO emulation (which is the case
here), run->mmio.data should be considered as a byte stream without
endianness, and what is needed is for QEMU to transfer data between
run->mmio.data and the device (or whatever is backing the MMIO region)
without any net byte swap.

So if QEMU is doing a 32-bit host-endian load from run->mmio.data
(for an MMIO store), then it needs to do a 32-bit host-endian store
into the device memory.  In other words, the RAM memory region needs
to be considered host endian to match run->mmio.data being considered
host endian.

Paul.

Re: [Qemu-devel] [PATCH] intel_iommu: make sure its init before PCI dev

[Michael S. Tsirkin]

On Thu, Feb 23, 2017 at 01:42:34PM +0800, Peter Xu wrote:
> On Wed, Feb 22, 2017 at 08:24:51PM -0700, Alex Williamson wrote:
> 
> [...]
> 
> > > Now Jintack reported another issue, that we may have two default
> > > devices there if not specifying "-nodefaults", and that two devices
> > > will always be the first ones to be inited.
> > > 
> > > How about here we just explicitly check against vfio-pci devices, so
> > > we just make sure vfio-pci devices will be put after intel-iommu?
> > > Since actually vfio-pci devices are the only ones that we know we need
> > > to be inited explicitly after the VT-d device.
> > 
> > I was afraid you were going to come to this conclusion.  That works,
> > BUT it just means the problem gets ignored as a vfio problem when
> > really vfio is doing nothing wrong other than caring about the device
> > address space during its initialization.

And that part is broken at this stage.  We can add a flag
"uses_address_space_in_init". Would that be better?


>  Then users have a perfectly
> > working config, add a vfio-pci device and things explode.  If you want
> > to impose a user ordering requirement, do it consistently.

This is not a new thing however. vfio previously did not work with
an iommu either. It seems reasonable to make sure iommu
is initialized first.

But it also seems reasonable to enable that gradually
and not require removing ordering requirements upfront.


>  Thanks,
> 
> We cannot guarantee that guest won't "explode" only if we
> automatically order the device init, but that's something I am not
> quite sure about that we will need now, especially I don't know
> whether it would be a 2.9 material, considering that 2.9 soft freeze
> should be on Feb 28th. :(
> 
> I kind of understand your concern, but would that really a so-called
> explosion? The user will just be warned that he/she should move the
> intel-iommu line slightly higher. Imho that's tolerable, since the
> user is definitely adding something new to the parameters, and it's
> possible the new command line just don't work. Also, imho it's not
> anyone's fault if it happens, it's just a new rule that we need to
> make sure things work properly...
> 
> I just want to know what would be the most feasible approach that we
> can safely have the vtd vfio series in before 2.9. Any further input
> would be welcomed.
> 
> Thanks,
> 
> -- peterx

Re: [Qemu-devel] using fdt_setprop() to set properties to empty values

[David Gibson]

On Thu, Feb 23, 2017 at 08:52:01AM -0600, Eric Blake wrote:
> On 02/23/2017 06:33 AM, Peter Maydell wrote:
> > What's the right way to use libfdt's fdt_setprop to set a property
> > to have an empty value? At the moment in QEMU we tend to use
> >   fdt_setprop(fdt, nodeoffset, "propertyname", NULL, 0);

That's what I've always used..

> > and git grep 'fdt_setprop.*NULL' produces examples of this usage in
> > PPC and ARM fdt creation code.
> > 
> > However the fdt_setprop() documentation doesn't document that a NULL
> > value pointer is OK if the length is 0, and indeed the implementation
> > unconditionally calls memcpy(prop->data, val, len), which is
> > undefined behaviour, and warned about by clang sanitizers if you
> > build libfdt with them:
> >  dtc/libfdt/fdt_rw.c:288:21: runtime error: null pointer passed
> >as argument 2, which is declared to never be null

..but indeed that isn't really technically right.

> > So what's the best thing to do here? I can't offhand think of a
> > non-ugly/non-confusing way to pass a valid pointer here...
> 
> Does fdt_setprop(fdt, nodeoffset, "propertyname", "", 0) do the right thing?

That should work, but it's a bit clunky.

I guess I should add an fdt_setprop_empty() to libfdt.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[David Gibson]

On Fri, Feb 24, 2017 at 09:34:32AM +1100, Richard Henderson wrote:
> On 02/23/2017 05:40 PM, Nikunj A Dadhania wrote:
> > Richard Henderson  writes:
> > > These functions are becoming quite large.  Are they performance critical 
> > > enough
> > > that they need to stay as inline code, or should they be moved to helpers 
> > > and
> > > share code with cpu_read/write_xer?
> > 
> > Just to boot to login prompt, these are the numbers for gen_read/write_xer:
> > 
> > helper_myprint - rd_count 231103, wr_count 68897
> > 
> > And it keeps on incrementing, maybe scope of optimization here.
> 
> That's not very large considering the total number of instructions executed
> during a boot to prompt.
> 
> Thoughts, David?

Hm, I'm not clear if that's the number of executions, or the number of
translations.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-ppc] Proposal PCI/PCIe device placement on PAPR guests

[David Gibson]

On Thu, Feb 23, 2017 at 08:23:50AM +0100, Greg Kurz wrote:
> On Thu, 23 Feb 2017 13:11:52 +1100
> David Gibson  wrote:
> 
> > On Wed, Feb 22, 2017 at 12:08:25PM +0100, Greg Kurz wrote:
> > > David,
> > > 
> > > I don't see the "spapr_pci: Allow PCI-Express devices" patch in your
> > > ppc-for-2.9 tree. Do you still consider merging it ?  
> > 
> > No.  After discussions with Marcel Apfelbaum and others I'm looking at
> > a different approach for PCIe support.
> > 
> 
> Oh, I had missed that... Is it the following thread ?
> 
> https://lists.gnu.org/archive/html/qemu-ppc/2017-02/msg00203.html

That's the one.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 00/22] ppc/xics: simplify ICS and ICP creation

[David Gibson]

On Thu, Feb 23, 2017 at 08:19:31AM +0100, Cédric Le Goater wrote:
> > Apart from that I'm pretty happy with the endpoint you reach.  I'm a
> > bit less convinced about the path taken to get there.  I'm not sure if
> > it's worth the churn of doing this reorg, but I think we'd get there
> > more clearly and with less intermediate abstraction violations if it
> > was done by:
> > 
> >  1. Introduce the xics qom interface, but have it implemented by
> > the existing xics object
> >  2. Change the ics and icp to only interact with the xics object
> > via the qom interface
> >  3. Implement the qom interface in the spapr machine
> >  4. Change to spapr directly creating ics and icp objects,
> > pointing back to itself as the xics interface provider
> >  5. Remove the xics concrete object
> 
> So that's a full rewrite of the patchset to reach the same point. 
> I can only grumble for such a proposal :/ 

Yeah.. point taken.

> > This also has the advantage that the qom path changing parts are
> > isolated to step (4), meaning problems with migration should be easier
> > to localize.
> 
> and migration works.

Oh, that's a nice surprise.  Ok never mind about the rework, just
address the other comments and repost.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] intel_iommu: make sure its init before PCI dev

[Michael S. Tsirkin]

On Wed, Feb 22, 2017 at 01:49:25PM +0800, Peter Xu wrote:
> Intel vIOMMU devices are created with "-device" parameter, while here
> actually we need to make sure this device will be created before some
> other PCI devices (like vfio-pci devices) so that we know iommu_fn will
> be setup correctly before realizations of those PCI devices.
> Here we do explicit check to make sure intel-iommu device will be inited
> before all the rest of the PCI devices. This is done by checking against
> the devices dangled under current root PCIe bus and we should see
> nothing there besides integrated ICH9 ones.
> 
> If the user violated this rule, we abort the program.
> 
> Maybe one day we will be able to manage the ordering of device
> initialization, and then we can grant VT-d devices a higher init
> priority. But before that, let's have this explicit check to make sure
> of it.
> 
> Signed-off-by: Peter Xu 
> ---
>  hw/i386/intel_iommu.c | 40 
>  1 file changed, 40 insertions(+)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 22d8226..db74124 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -31,6 +31,7 @@
>  #include "hw/i386/apic-msidef.h"
>  #include "hw/boards.h"
>  #include "hw/i386/x86-iommu.h"
> +#include "hw/i386/ich9.h"
>  #include "hw/pci-host/q35.h"
>  #include "sysemu/kvm.h"
>  #include "hw/i386/apic_internal.h"
> @@ -2560,6 +2561,41 @@ static bool vtd_decide_config(IntelIOMMUState *s, 
> Error **errp)
>  return true;
>  }
>  
> +static bool vtd_has_inited_pci_devices(PCIBus *bus, Error **errp)
> +{
> +int i;
> +uint8_t func;
> +
> +/* We check against root bus */
> +assert(bus && pci_bus_is_root(bus));
> +
> +/*
> + * We need to make sure vIOMMU device is created before other PCI
> + * devices other than the integrated ICH9 ones,


Why wouldn't this apply to integrated ICH9 ones?


> so that they can
> + * get correct iommu_fn setup even during its realize(). Some
> + * devices (e.g., vfio-pci) will need a correct iommu_fn to work.

If there's something special about vfio devices, then just check
the device type.

> + */
> +for (i = 1; i < PCI_FUNC_MAX * PCI_SLOT_MAX; i++) {
> +/* Skip the checking against ICH9 integrated devices */
> +if (PCI_SLOT(i) == ICH9_LPC_DEV) {
> +func = PCI_FUNC(i);
> +if (func == ICH9_LPC_FUNC ||
> +func == ICH9_SATA1_FUNC ||
> +func == ICH9_SMB_FUNC) {
> +continue;
> +}
> +}
> +
> +if (bus->devices[i]) {
> +error_setg(errp, "Please init intel-iommu before "
> +   "other PCI devices");
> +return true;
> +}
> +}
> +
> +return false;
> +}
> +
>  static void vtd_realize(DeviceState *dev, Error **errp)
>  {
>  PCMachineState *pcms = PC_MACHINE(qdev_get_machine());
> @@ -2567,6 +2603,10 @@ static void vtd_realize(DeviceState *dev, Error **errp)
>  IntelIOMMUState *s = INTEL_IOMMU_DEVICE(dev);
>  X86IOMMUState *x86_iommu = X86_IOMMU_DEVICE(dev);
>  
> +if (vtd_has_inited_pci_devices(bus, errp)) {
> +return;
> +}
> +
>  VTD_DPRINTF(GENERAL, "");
>  x86_iommu->type = TYPE_INTEL;
>  
> -- 
> 2.7.4

Re: [Qemu-devel] [PATCH 03/21] qmp-test: New, covering basic QMP protocol

[Eric Blake]

On 02/23/2017 03:44 PM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster 
> ---
>  tests/Makefile.include |   5 +-
>  tests/libqtest.c   |  17 --
>  tests/libqtest.h   |   8 +++
>  tests/qmp-test.c   | 139 
> +
>  4 files changed, 163 insertions(+), 6 deletions(-)
>  create mode 100644 tests/qmp-test.c
> 

> +++ b/tests/libqtest.h
> @@ -32,6 +32,14 @@ extern QTestState *global_qtest;
>  QTestState *qtest_init(const char *extra_args);
>  
>  /**
> + * qtest_init:

Wrong name (too much copy-and-paste)

> + * @extra_args: other arguments to pass to QEMU.
> + *
> + * Returns: #QTestState instance.
> + */
> +QTestState *qtest_init_without_qmp_handshake(const char *extra_args);
> +
> +/**
>   * qtest_quit:
>   * @s: #QTestState instance to operate on.
>   *
> diff --git a/tests/qmp-test.c b/tests/qmp-test.c
> new file mode 100644
> index 000..405e49e
> --- /dev/null
> +++ b/tests/qmp-test.c

> +
> +static void test_malformed(void)
> +{
> +QDict *resp;
> +
> +/* Not even a dictionary */
> +resp = qmp("null");
> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> +QDECREF(resp);
> +

Shall we test an array, integer, and/or boolean literal as well?

> +/* No "execute" key */
> +resp = qmp("{}");
> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> +QDECREF(resp);
> +
> +/* "execute" isn't a string */
> +resp = qmp("{ 'execute': true }");
> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> +QDECREF(resp);
> +
> +/* "arguments" isn't a dictionary */
> +resp = qmp("{ 'execute': 'no-such-cmd', 'arguments': [] }");
> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> +QDECREF(resp);
> +
> +/* extra key */
> +resp = qmp("{ 'execute': 'no-such-cmd', 'extra': true }");
> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> +QDECREF(resp);

Worth testing "{ 'arguments': {} }"?

> +}
> +
> +static void test_qmp_protocol(void)
> +{
> +QDict *resp, *q, *ret;
> +QList *capabilities;
> +
> +global_qtest = qtest_init_without_qmp_handshake(common_args);
> +
> +/* Test greeting */
> +resp = qmp_receive();
> +q = qdict_get_qdict(resp, "QMP");
> +g_assert(q);
> +test_version(qdict_get(q, "version"));
> +capabilities = qdict_get_qlist(q, "capabilities");
> +g_assert(capabilities && qlist_empty(capabilities));
> +QDECREF(resp);
> +
> +/* Test valid command before handshake */
> +resp = qmp("{ 'execute': 'query-version' }");
> +g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
> +QDECREF(resp);
> +
> +/* Test malformed commands before handshake */
> +test_malformed();
> +
> +/* Test handshake */
> +resp = qmp("{ 'execute': 'qmp_capabilities' }");
> +ret = qdict_get_qdict(resp, "return");
> +g_assert(ret && !qdict_size(ret));
> +QDECREF(resp);
> +
> +/* Test repeated handshake */
> +resp = qmp("{ 'execute': 'qmp_capabilities' }");
> +g_assert_cmpstr(get_error_class(resp), ==, "CommandNotFound");
> +QDECREF(resp);
> +
> +/* Test valid command */
> +resp = qmp("{ 'execute': 'query-version' }");
> +test_version(qdict_get(resp, "return"));
> +QDECREF(resp);
> +
> +/* Test malformed commands */
> +test_malformed();
> +
> +/* Test 'id' */
> +resp = qmp("{ 'execute': 'query-name', 'id': 'cookie#1' }");
> +ret = qdict_get_qdict(resp, "return");
> +g_assert(ret);
> +g_assert_cmpstr(qdict_get_try_str(resp, "id"), ==, "cookie#1");
> +QDECREF(resp);
> +
> +/* Test command failure with 'id' */
> +resp = qmp("{ 'execute': 'human-monitor-command', 'id': 2 }");
> +g_assert_cmpstr(get_error_class(resp), ==, "GenericError");
> +g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);
> +QDECREF(resp);

id can be _any JSON_ (at one point, QMP crashed if id included a null
literal); so maybe we should have more id tests such as "'id': null,",
"'id': [ { }, true ]"

But any tests at all is better than our previous state of none, so even
if you don't add tests, but just fix the typo:

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] iommu emulation

[Jintack Lim]

[cc Bandan]

On Tue, Feb 21, 2017 at 5:33 AM, Jintack Lim 
wrote:

>
>
> On Wed, Feb 15, 2017 at 9:47 PM, Alex Williamson <
> alex.william...@redhat.com> wrote:
>
>> On Thu, 16 Feb 2017 10:28:39 +0800
>> Peter Xu  wrote:
>>
>> > On Wed, Feb 15, 2017 at 11:15:52AM -0700, Alex Williamson wrote:
>> >
>> > [...]
>> >
>> > > > Alex, do you like something like below to fix above issue that
>> Jintack
>> > > > has encountered?
>> > > >
>> > > > (note: this code is not for compile, only trying show what I
>> mean...)
>> > > >
>> > > > --8<---
>> > > > diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> > > > index 332f41d..4dca631 100644
>> > > > --- a/hw/vfio/pci.c
>> > > > +++ b/hw/vfio/pci.c
>> > > > @@ -1877,25 +1877,6 @@ static void vfio_add_ext_cap(VFIOPCIDevice
>> *vdev)
>> > > >   */
>> > > >  config = g_memdup(pdev->config, vdev->config_size);
>> > > >
>> > > > -/*
>> > > > - * Extended capabilities are chained with each pointing to the
>> next, so we
>> > > > - * can drop anything other than the head of the chain simply
>> by modifying
>> > > > - * the previous next pointer.  For the head of the chain, we
>> can modify the
>> > > > - * capability ID to something that cannot match a valid
>> capability.  ID
>> > > > - * 0 is reserved for this since absence of capabilities is
>> indicated by
>> > > > - * 0 for the ID, version, AND next pointer.  However,
>> pcie_add_capability()
>> > > > - * uses ID 0 as reserved for list management and will
>> incorrectly match and
>> > > > - * assert if we attempt to pre-load the head of the chain with
>> this ID.
>> > > > - * Use ID 0x temporarily since it is also seems to be
>> reserved in
>> > > > - * part for identifying absence of capabilities in a root
>> complex register
>> > > > - * block.  If the ID still exists after adding capabilities,
>> switch back to
>> > > > - * zero.  We'll mark this entire first dword as emulated for
>> this purpose.
>> > > > - */
>> > > > -pci_set_long(pdev->config + PCI_CONFIG_SPACE_SIZE,
>> > > > - PCI_EXT_CAP(0x, 0, 0));
>> > > > -pci_set_long(pdev->wmask + PCI_CONFIG_SPACE_SIZE, 0);
>> > > > -pci_set_long(vdev->emulated_config_bits +
>> PCI_CONFIG_SPACE_SIZE, ~0);
>> > > > -
>> > > >  for (next = PCI_CONFIG_SPACE_SIZE; next;
>> > > >   next = PCI_EXT_CAP_NEXT(pci_get_long(config + next))) {
>> > > >  header = pci_get_long(config + next);
>> > > > @@ -1917,6 +1898,8 @@ static void vfio_add_ext_cap(VFIOPCIDevice
>> *vdev)
>> > > >  switch (cap_id) {
>> > > >  case PCI_EXT_CAP_ID_SRIOV: /* Read-only VF BARs confuse
>> OVMF */
>> > > >  case PCI_EXT_CAP_ID_ARI: /* XXX Needs next function
>> virtualization */
>> > > > +/* keep this ecap header (4 bytes), but mask cap_id to
>> 0x */
>> > > > +...
>> > > >  trace_vfio_add_ext_cap_dropped(vdev->vbasedev.name,
>> cap_id, next);
>> > > >  break;
>> > > >  default:
>> > > > @@ -1925,11 +1908,6 @@ static void vfio_add_ext_cap(VFIOPCIDevice
>> *vdev)
>> > > >
>> > > >  }
>> > > >
>> > > > -/* Cleanup chain head ID if necessary */
>> > > > -if (pci_get_word(pdev->config + PCI_CONFIG_SPACE_SIZE) ==
>> 0x) {
>> > > > -pci_set_word(pdev->config + PCI_CONFIG_SPACE_SIZE, 0);
>> > > > -}
>> > > > -
>> > > >  g_free(config);
>> > > >  return;
>> > > >  }
>> > > > ->8-
>> > > >
>> > > > Since after all we need the assumption that 0x is reserved for
>> > > > cap_id. Then, we can just remove the "first 0x then 0x0" hack,
>> > > > which is imho error-prone and hacky.
>> > >
>> > > This doesn't fix the bug, which is that pcie_add_capability() uses a
>> > > valid capability ID for it's own internal tracking.  It's only doing
>> > > this to find the end of the capability chain, which we could do in a
>> > > spec complaint way by looking for a zero next pointer.  Fix that and
>> > > then vfio doesn't need to do this set to 0x then back to zero
>> > > nonsense at all.  Capability ID zero is valid.  Thanks,
>> >
>> > Yeah I see Michael's fix on the capability list stuff. However, imho
>> > these are two different issues? Or say, even if with that patch, we
>> > should still need this hack (first 0x0, then 0x) right? Since
>> > looks like that patch didn't solve the problem if the first pcie ecap
>> > is masked at 0x100.
>>
>> I thought the problem was that QEMU in the host exposes a device with a
>> capability ID of 0 to the L1 guest.  QEMU in the L1 guest balks at a
>> capability ID of 0 because that's how it finds the end of the chain.
>> Therefore if we make QEMU not use capability ID 0 for internal
>> purposes, things work.  vfio using 0x and swapping back to 0x0
>> becomes unnecessary, but doesn't hurt anything.  Thanks,
>>
>
> I've applied Peter's hack and Michael's patch below, but still can't 

Re: [Qemu-devel] [PATCH 02/21] libqtest: Work around a "QMP wants a newline" bug

[Eric Blake]

On 02/23/2017 03:44 PM, Markus Armbruster wrote:
> The next commit is going to add a test that calls qmp("null").
> Curiously, this hangs.  Here's why.
> 
> qmp_fd_sendv() doesn't send newlines.  Not even when @fmt contains
> some.  At first glance, the QMP parser seems to be fine with that.
> However, it turns out that it fails to react to input until it sees
> either a newline, an object or an array.  To reproduce, feed to a QMP
> monitor like this:
> 

> 
> Work around this QMP bug by having qmp_fd_sendv() append a newline.
> 
> Signed-off-by: Markus Armbruster 
> ---
>  tests/libqtest.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 

> +++ b/tests/libqtest.c
> @@ -442,14 +442,20 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
>  if (qobj) {
>  int log = getenv("QTEST_LOG") != NULL;
>  QString *qstr = qobject_to_json(qobj);
> -const char *str = qstring_get_str(qstr);
> -size_t size = qstring_get_length(qstr);
> +const char *str;
> +
> +/*
> + * BUG: QMP doesn't react to input until it sees a newline, an
> + * object, or an array.  Work-around: give it a newline.
> + */
> +qstring_append_chr(qstr, '\n');
> +str = qstring_get_str(qstr);
>  
>  if (log) {
>  fprintf(stderr, "%s", str);

Bonus - the log now has newlines, too :)

>  }
>  /* Send QMP request */
> -socket_send(fd, str, size);
> +socket_send(fd, str, qstring_get_length(qstr));

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 01/21] qga: Fix crash on non-dictionary QMP argument

[Eric Blake]

On 02/23/2017 04:46 PM, Eric Blake wrote:
> On 02/23/2017 03:44 PM, Markus Armbruster wrote:
>> The value of key 'arguments' must be a JSON object.  qemu-ga neglects
>> to check, and crashes.  To reproduce, send
>>
>> { 'execute': 'guest-sync', 'arguments': [] }
>>
>> to qemu-ga.
>>
>> do_qmp_dispatch() uses qdict_get_qdict() to get the arguments.  When
>> not a JSON object, this gets a null pointer, which flows through the
>> generated marshalling function to qobject_input_visitor_new(), where
>> it fails the assertion.  qmp_dispatch_check_obj() needs to catch this
>> error.
>>
>> QEMU isn't affected, because it runs qmp_check_input_obj() first,
>> which basically duplicates qmp_check_input_obj()'s checks, plus the

This sentence is weird (func A can't duplicate func A's checks; you're
missing a func B, but I'm not sure which instance is wrong, nor what B
should be).

>> missing one.
>>
>> Fix by copying the missing one from qmp_check_input_obj() to
>> qmp_dispatch_check_obj().
>>
>> Signed-off-by: Markus Armbruster 
>> Cc: Michael Roth 
>> ---
>>  qapi/qmp-dispatch.c | 8 +++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Eric Blake 
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 01/21] qga: Fix crash on non-dictionary QMP argument

[Eric Blake]

On 02/23/2017 03:44 PM, Markus Armbruster wrote:
> The value of key 'arguments' must be a JSON object.  qemu-ga neglects
> to check, and crashes.  To reproduce, send
> 
> { 'execute': 'guest-sync', 'arguments': [] }
> 
> to qemu-ga.
> 
> do_qmp_dispatch() uses qdict_get_qdict() to get the arguments.  When
> not a JSON object, this gets a null pointer, which flows through the
> generated marshalling function to qobject_input_visitor_new(), where
> it fails the assertion.  qmp_dispatch_check_obj() needs to catch this
> error.
> 
> QEMU isn't affected, because it runs qmp_check_input_obj() first,
> which basically duplicates qmp_check_input_obj()'s checks, plus the
> missing one.
> 
> Fix by copying the missing one from qmp_check_input_obj() to
> qmp_dispatch_check_obj().
> 
> Signed-off-by: Markus Armbruster 
> Cc: Michael Roth 
> ---
>  qapi/qmp-dispatch.c | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Eric Blake 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 00/22] target/openrisc updates

[Richard Henderson]

On 02/24/2017 01:09 AM, Stafford Horne wrote:

I posted a test image to the wiki

 http://wiki.qemu-project.org/Testing/System_Images

However, someone on the #qemu irc room mentioned its not ideal to post
any images on the wiki and existing images should be taken down due to
copyright issues.  I don't quite follow, should we be concerned?


I think that someone is confused.  All of those testing images are open source.

Thanks for building it!


r~

Re: [Qemu-devel] [PATCH] mttcg/i386: Patch instruction using async_safe_* framework

[Richard Henderson]

On 02/23/2017 12:20 PM, Pranith Kumar wrote:

In mttcg, calling pause_all_vcpus() during execution from the
generated TBs causes a deadlock if some vCPU is waiting for exclusive
execution in start_exclusive(). Fix this by using the aync_safe_*
framework instead of pausing vcpus for patching instructions.

CC: Richard Henderson 
CC: Peter Maydell 
CC: Alex Bennée 
Signed-off-by: Pranith Kumar 
---
 hw/i386/kvmvapic.c | 104 ++---
 1 file changed, 76 insertions(+), 28 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 82a4955..15eb39d 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -383,8 +383,7 @@ static void patch_byte(X86CPU *cpu, target_ulong addr, 
uint8_t byte)
 cpu_memory_rw_debug(CPU(cpu), addr, , 1, 1);
 }

-static void patch_call(VAPICROMState *s, X86CPU *cpu, target_ulong ip,
-   uint32_t target)
+static void patch_call(X86CPU *cpu, target_ulong ip, uint32_t target)
 {
 uint32_t offset;

@@ -393,17 +392,74 @@ static void patch_call(VAPICROMState *s, X86CPU *cpu, 
target_ulong ip,
 cpu_memory_rw_debug(CPU(cpu), ip + 1, (void *), sizeof(offset), 1);
 }

-static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
+struct PatchInfo {
+VAPICHandlers *handler;
+target_ulong ip;
+};
+
+static void do_patch_instruction(CPUState *cs, run_on_cpu_data data)
 {
-CPUState *cs = CPU(cpu);
-CPUX86State *env = >env;
-VAPICHandlers *handlers;
+X86CPU *x86_cpu = X86_CPU(cs);
+CPUX86State *env = _cpu->env;
+struct PatchInfo *info = (struct PatchInfo *) data.host_ptr;
+VAPICHandlers *handlers = info->handler;
+target_ulong ip = info->ip;
 uint8_t opcode[2];
 uint32_t imm32 = 0;
 target_ulong current_pc = 0;
 target_ulong current_cs_base = 0;
 uint32_t current_flags = 0;

+cpu_get_tb_cpu_state(env, _pc, _cs_base,
+ _flags);
+/* Account this instruction, because we will exit the tb.
+   This is the first instruction in the block. Therefore
+   there is no need in restoring CPU state. */
+if (use_icount) {
+--cs->icount_decr.u16.low;
+}
+
+cpu_memory_rw_debug(cs, ip, opcode, sizeof(opcode), 0);
+
+switch (opcode[0]) {
+case 0x89: /* mov r32 to r/m32 */
+patch_byte(x86_cpu, ip, 0x50 + modrm_reg(opcode[1]));  /* push reg */
+patch_call(x86_cpu, ip + 1, handlers->set_tpr);
+break;
+case 0x8b: /* mov r/m32 to r32 */
+patch_byte(x86_cpu, ip, 0x90);
+patch_call(x86_cpu, ip + 1, handlers->get_tpr[modrm_reg(opcode[1])]);
+break;
+case 0xa1: /* mov abs to eax */
+patch_call(x86_cpu, ip, handlers->get_tpr[0]);
+break;
+case 0xa3: /* mov eax to abs */
+patch_call(x86_cpu, ip, handlers->set_tpr_eax);
+break;
+case 0xc7: /* mov imm32, r/m32 (c7/0) */
+patch_byte(x86_cpu, ip, 0x68);  /* push imm32 */
+cpu_memory_rw_debug(cs, ip + 6, (void *), sizeof(imm32), 0);
+cpu_memory_rw_debug(cs, ip + 1, (void *), sizeof(imm32), 1);
+patch_call(x86_cpu, ip + 5, handlers->set_tpr);
+break;
+case 0xff: /* push r/m32 */
+patch_byte(x86_cpu, ip, 0x50); /* push eax */
+patch_call(x86_cpu, ip + 1, handlers->get_tpr_stack);
+break;
+default:
+abort();
+}
+
+g_free(info);
+}
+
+static void patch_instruction(VAPICROMState *s, X86CPU *cpu, target_ulong ip)
+{
+CPUState *cs = CPU(cpu);
+VAPICHandlers *handlers;
+uint8_t opcode[2];
+uint32_t imm32 = 0;
+
 if (smp_cpus == 1) {
 handlers = >rom_state.up;
 } else {
@@ -411,14 +467,14 @@ static void patch_instruction(VAPICROMState *s, X86CPU 
*cpu, target_ulong ip)
 }

 if (!kvm_enabled()) {
-cpu_get_tb_cpu_state(env, _pc, _cs_base,
- _flags);
-/* Account this instruction, because we will exit the tb.
-   This is the first instruction in the block. Therefore
-   there is no need in restoring CPU state. */
-if (use_icount) {
---cs->icount_decr.u16.low;
-}
+struct PatchInfo *info = g_new(struct PatchInfo, 1);
+const run_on_cpu_func fn = do_patch_instruction;
+info->handler = handlers;
+info->ip = ip;
+
+async_safe_run_on_cpu(cs, fn, RUN_ON_CPU_HOST_PTR(info));
+cs->exception_index = EXCP_INTERRUPT;
+cpu_loop_exit(cs);
 }

 pause_all_vcpus();
@@ -428,41 +484,33 @@ static void patch_instruction(VAPICROMState *s, X86CPU 
*cpu, target_ulong ip)
 switch (opcode[0]) {
 case 0x89: /* mov r32 to r/m32 */
 patch_byte(cpu, ip, 0x50 + modrm_reg(opcode[1]));  /* push reg */
-patch_call(s, cpu, ip + 1, handlers->set_tpr);
+patch_call(cpu, ip + 1, handlers->set_tpr);
 

Re: [Qemu-devel] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[David Gibson]

On Thu, Feb 23, 2017 at 12:32:44PM +0530, Nikunj A Dadhania wrote:
> David Gibson  writes:
> 
> > -static void gen_read_xer(TCGv dst)
> >> +static void gen_read_xer(DisasContext *ctx, TCGv dst)
> >>  {
> >>  TCGv t0 = tcg_temp_new();
> >>  TCGv t1 = tcg_temp_new();
> >> @@ -3715,15 +3719,30 @@ static void gen_read_xer(TCGv dst)
> >>  tcg_gen_or_tl(t0, t0, t1);
> >>  tcg_gen_or_tl(dst, dst, t2);
> >>  tcg_gen_or_tl(dst, dst, t0);
> >> +if (is_isa300(ctx)) {
> >> +tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
> >> +tcg_gen_or_tl(dst, dst, t0);
> >> +tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
> >> +tcg_gen_or_tl(dst, dst, t0);
> >
> > Could you use 2 deposits here, instead of 2 shifts and 2 ors?
> 
> I checked the implementation of tcg_gen_deposit_i64, resultant will have much
> more than 2 shifts + 2 ors.

Ok, fair enough.

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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[Richard Henderson]

On 02/23/2017 06:02 PM, Nikunj A Dadhania wrote:

David Gibson  writes:


-static void gen_read_xer(TCGv dst)

+static void gen_read_xer(DisasContext *ctx, TCGv dst)
 {
 TCGv t0 = tcg_temp_new();
 TCGv t1 = tcg_temp_new();
@@ -3715,15 +3719,30 @@ static void gen_read_xer(TCGv dst)
 tcg_gen_or_tl(t0, t0, t1);
 tcg_gen_or_tl(dst, dst, t2);
 tcg_gen_or_tl(dst, dst, t0);
+if (is_isa300(ctx)) {
+tcg_gen_shli_tl(t0, cpu_ov32, XER_OV32);
+tcg_gen_or_tl(dst, dst, t0);
+tcg_gen_shli_tl(t0, cpu_ca32, XER_CA32);
+tcg_gen_or_tl(dst, dst, t0);


Could you use 2 deposits here, instead of 2 shifts and 2 ors?


I checked the implementation of tcg_gen_deposit_i64, resultant will have much
more than 2 shifts + 2 ors.


Well, that depends on the host.  For a host that implements deposit, like 
aarch64 or ppc64, it will be one instruction.



r~

Re: [Qemu-devel] [PATCH v3 03/10] target/ppc: support for 32-bit carry and overflow

[Richard Henderson]

On 02/23/2017 05:40 PM, Nikunj A Dadhania wrote:

Richard Henderson  writes:

These functions are becoming quite large.  Are they performance critical enough
that they need to stay as inline code, or should they be moved to helpers and
share code with cpu_read/write_xer?


Just to boot to login prompt, these are the numbers for gen_read/write_xer:

helper_myprint - rd_count 231103, wr_count 68897

And it keeps on incrementing, maybe scope of optimization here.


That's not very large considering the total number of instructions executed 
during a boot to prompt.


Thoughts, David?


r~

Re: [Qemu-devel] [PATCH RESEND] aarch64: Change ext type to TCGType to fix warnings

[Richard Henderson]

On 02/18/2017 02:43 AM, Pranith Kumar wrote:

To fix the following warnings:

In file included from /users/pranith/qemu/tcg/tcg.c:255:
/users/pranith/qemu/tcg/aarch64/tcg-target.inc.c:879:24: warning: implicit 
conversion from enumeration type 'TCGMemOp' (aka 'enum TCGMemOp') to different 
enumeration type 'TCGType' (aka 'enum TCGType')
  [-Wenum-conversion]
tcg_out_cmp(s, ext, a, b, b_const);
~~~^~~
/users/pranith/qemu/tcg/aarch64/tcg-target.inc.c:893:36: warning: implicit 
conversion from enumeration type 'TCGMemOp' (aka 'enum TCGMemOp') to different 
enumeration type 'TCGType' (aka 'enum TCGType')
  [-Wenum-conversion]
tcg_out_insn(s, 3201, CBZ, ext, a, offset);
~~~^~~
/users/pranith/qemu/tcg/aarch64/tcg-target.inc.c:389:65: note: expanded from 
macro 'tcg_out_insn'
glue(tcg_out_insn_,FMT)(S, glue(glue(glue(I,FMT),_),OP), ## __VA_ARGS__)
^
/users/pranith/qemu/tcg/aarch64/tcg-target.inc.c:895:37: warning: implicit 
conversion from enumeration type 'TCGMemOp' (aka 'enum TCGMemOp') to different 
enumeration type 'TCGType' (aka 'enum TCGType')
  [-Wenum-conversion]
tcg_out_insn(s, 3201, CBNZ, ext, a, offset);
^~~
/users/pranith/qemu/tcg/aarch64/tcg-target.inc.c:389:65: note: expanded from 
macro 'tcg_out_insn'
glue(tcg_out_insn_,FMT)(S, glue(glue(glue(I,FMT),_),OP), ## __VA_ARGS__)
^
/users/pranith/qemu/tcg/aarch64/tcg-target.inc.c:1610:27: warning: implicit 
conversion from enumeration type 'TCGType' (aka 'enum TCGType') to different 
enumeration type 'TCGMemOp' (aka 'enum TCGMemOp')
  [-Wenum-conversion]
tcg_out_brcond(s, ext, a2, a0, a1, const_args[1], arg_label(args[3]));
~~^~~

Signed-off-by: Pranith Kumar 
---
 tcg/aarch64/tcg-target.inc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


I haven't seen this, but I've also just noticed that the gcc farm aarch64 
machine is using gcc 4.8.5, so that could be it.


Applied.


r~

Re: [Qemu-devel] [PATCH v2 0/3] i386: Add "max" CPU model to TCG and KVM

[Richard W.M. Jones]

On Thu, Feb 23, 2017 at 05:07:47PM -0300, Eduardo Habkost wrote:
> Ping?
> 
> As v1 was sitting on the list since Jan 19, if there are no
> objections I will merge this and include in my next pull request
> before soft freeze.

Do you have a copy which applies on top of current HEAD?  I get
loads of conflicts.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

[Qemu-devel] [PATCH 20/21] qapi: Make input visitors detect unvisited list tails

[Markus Armbruster]

Fix the design flaw demonstrated in the previous commit: new method
check_list() lets input visitors report that unvisited input remains
for a list, exactly like check_struct() lets them report that
unvisited input remains for a struct or union.

Implement the method for the qobject input visitor (straightforward),
and the string input visitor (less so, due to the magic list syntax
there).  The opts visitor's list magic is even more impenetrable, and
all I can do there today is a stub with a FIXME comment.  No worse
than before.

Signed-off-by: Markus Armbruster 
---
 hw/ppc/spapr_drc.c |  5 +
 include/qapi/visitor-impl.h|  3 +++
 include/qapi/visitor.h | 13 +
 qapi/opts-visitor.c| 11 +++
 qapi/qapi-visit-core.c |  8 
 qapi/qobject-input-visitor.c   | 35 +++
 qapi/string-input-visitor.c| 30 ++
 qapi/trace-events  |  1 +
 scripts/qapi-visit.py  |  3 +++
 tests/test-opts-visitor.c  |  2 +-
 tests/test-qobject-input-visitor.c |  4 +++-
 tests/test-string-input-visitor.c  |  3 ++-
 12 files changed, 111 insertions(+), 7 deletions(-)

diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 2de6377..fe7b988 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -326,6 +326,11 @@ static void prop_get_fdt(Object *obj, Visitor *v, const 
char *name,
 return;
 }
 }
+visit_check_list(v, );
+if (err) {
+error_propagate(errp, err);
+return;
+}
 visit_end_list(v, NULL);
 break;
 }
diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 962ba1d..e87709d 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -61,6 +61,9 @@ struct Visitor
 /* Must be set */
 GenericList *(*next_list)(Visitor *v, GenericList *tail, size_t size);
 
+/* Optional; intended for input visitors */
+void (*check_list)(Visitor *v, Error **errp);
+
 /* Must be set */
 void (*end_list)(Visitor *v, void **list);
 
diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 7c91a50..1a1b620 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -370,6 +370,19 @@ void visit_start_list(Visitor *v, const char *name, 
GenericList **list,
 GenericList *visit_next_list(Visitor *v, GenericList *tail, size_t size);
 
 /*
+ * Prepare for completing a list visit.
+ *
+ * @errp obeys typical error usage, and reports failures such as
+ * unvisited list tail remaining in the input stream.
+ *
+ * Should be called prior to visit_end_list() if all other
+ * intermediate visit steps were successful, to allow the visitor one
+ * last chance to report errors.  May be skipped on a cleanup path,
+ * where there is no need to check for further errors.
+ */
+void visit_check_list(Visitor *v, Error **errp);
+
+/*
  * Complete a list visit started earlier.
  *
  * @list must match what was passed to the paired visit_start_list().
diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index c50dc4b..026d25b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -273,6 +273,16 @@ opts_next_list(Visitor *v, GenericList *tail, size_t size)
 
 
 static void
+opts_check_list(Visitor *v, Error **errp)
+{
+/*
+ * FIXME should set error when unvisited elements remain.  Mostly
+ * harmless, as the generated visits always visit all elements.
+ */
+}
+
+
+static void
 opts_end_list(Visitor *v, void **obj)
 {
 OptsVisitor *ov = to_ov(v);
@@ -539,6 +549,7 @@ opts_visitor_new(const QemuOpts *opts)
 
 ov->visitor.start_list = _start_list;
 ov->visitor.next_list  = _next_list;
+ov->visitor.check_list = _check_list;
 ov->visitor.end_list   = _end_list;
 
 ov->visitor.type_int64  = _type_int64;
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index e6e93f0..43a09d1 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -90,6 +90,14 @@ GenericList *visit_next_list(Visitor *v, GenericList *tail, 
size_t size)
 return v->next_list(v, tail, size);
 }
 
+void visit_check_list(Visitor *v, Error **errp)
+{
+trace_visit_check_list(v);
+if (v->check_list) {
+v->check_list(v, errp);
+}
+}
+
 void visit_end_list(Visitor *v, void **obj)
 {
 trace_visit_end_list(v, obj);
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 97a27e2..0229046 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
 return container_of(v, QObjectInputVisitor, visitor);
 }
 
-static const char *full_name(QObjectInputVisitor *qiv, const char *name)
+static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
+ 

[Qemu-devel] [PATCH 19/21] tests: Cover partial input visit of list

[Markus Armbruster]

Demonstrates a design flaw: there is no way to for input visitors to
report that a list visit didn't visit the complete input list.  The
generated list visits always do, but manual visits needn't.

Signed-off-by: Markus Armbruster 
---
 tests/test-opts-visitor.c  | 41 ++
 tests/test-qobject-input-visitor.c | 19 ++
 tests/test-string-input-visitor.c  | 19 ++
 3 files changed, 79 insertions(+)

diff --git a/tests/test-opts-visitor.c b/tests/test-opts-visitor.c
index 0a9e75f..58d0eb1 100644
--- a/tests/test-opts-visitor.c
+++ b/tests/test-opts-visitor.c
@@ -172,6 +172,44 @@ expect_u64_max(OptsVisitorFixture *f, gconstpointer 
test_data)
 
 /* test cases */
 
+static void
+test_opts_range_unvisited(void)
+{
+intList *list = NULL;
+intList *tail;
+QemuOpts *opts;
+Visitor *v;
+
+opts = qemu_opts_parse(qemu_find_opts("userdef"), "ilist=0-2", false,
+   _abort);
+
+v = opts_visitor_new(opts);
+
+visit_start_struct(v, NULL, NULL, 0, _abort);
+
+/* Would be simpler if  opts visitor supported virtual list walks */
+visit_start_list(v, "ilist", (GenericList **), sizeof(*list),
+ _abort);
+tail = list;
+visit_type_int(v, NULL, >value, _abort);
+g_assert_cmpint(tail->value, ==, 0);
+tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list));
+g_assert(tail);
+visit_type_int(v, NULL, >value, _abort);
+g_assert_cmpint(tail->value, ==, 1);
+tail = (intList *)visit_next_list(v, (GenericList *)tail, sizeof(*list));
+g_assert(tail);
+visit_end_list(v, (void **));
+/* BUG: unvisited tail not reported; actually not reportable by design */
+
+visit_check_struct(v, _abort);
+visit_end_struct(v, NULL);
+
+qapi_free_intList(list);
+visit_free(v);
+qemu_opts_del(opts);
+}
+
 int
 main(int argc, char **argv)
 {
@@ -263,6 +301,9 @@ main(int argc, char **argv)
 add_test("/visitor/opts/i64/range/2big/full", _fail,
  "i64=-0x8000-0x7fff");
 
+g_test_add_func("/visitor/opts/range/unvisited",
+test_opts_range_unvisited);
+
 g_test_run();
 return 0;
 }
diff --git a/tests/test-qobject-input-visitor.c 
b/tests/test-qobject-input-visitor.c
index 32c6b3d..ac87e75 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -923,6 +923,23 @@ static void 
test_visitor_in_fail_struct_missing(TestInputVisitorData *data,
 visit_end_struct(v, NULL);
 }
 
+static void test_visitor_in_fail_list(TestInputVisitorData *data,
+  const void *unused)
+{
+int64_t i64 = -1;
+Visitor *v;
+
+v = visitor_input_test_init(data, "[ 1, 2, 3 ]");
+
+visit_start_list(v, NULL, NULL, 0, _abort);
+visit_type_int(v, NULL, , _abort);
+g_assert_cmpint(i64, ==, 1);
+visit_type_int(v, NULL, , _abort);
+g_assert_cmpint(i64, ==, 2);
+visit_end_list(v, NULL);
+/* BUG: unvisited tail not reported; actually not reportable by design */
+}
+
 static void test_visitor_in_fail_union_native_list(TestInputVisitorData *data,
const void *unused)
 {
@@ -1070,6 +1087,8 @@ int main(int argc, char **argv)
NULL, test_visitor_in_fail_struct_in_list);
 input_visitor_test_add("/visitor/input/fail/struct-missing",
NULL, test_visitor_in_fail_struct_missing);
+input_visitor_test_add("/visitor/input/fail/list",
+   NULL, test_visitor_in_fail_list);
 input_visitor_test_add("/visitor/input/fail/union-flat",
NULL, test_visitor_in_fail_union_flat);
 input_visitor_test_add("/visitor/input/fail/union-flat-no-discriminator",
diff --git a/tests/test-string-input-visitor.c 
b/tests/test-string-input-visitor.c
index 7f10e25..bb50473 100644
--- a/tests/test-string-input-visitor.c
+++ b/tests/test-string-input-visitor.c
@@ -94,6 +94,25 @@ static void test_visitor_in_intList(TestInputVisitorData 
*data,
 visit_type_int16List(v, NULL, , );
 error_free_or_abort();
 g_assert(!res);
+
+v = visitor_input_test_init(data, "0,2-3");
+
+/* Would be simpler if  opts visitor supported virtual list walks */
+visit_start_list(v, NULL, (GenericList **), sizeof(*res),
+ _abort);
+tmp = res;
+visit_type_int16(v, NULL, >value, _abort);
+g_assert_cmpint(tmp->value, ==, 0);
+tmp = (int16List *)visit_next_list(v, (GenericList *)tmp, sizeof(*res));
+g_assert(tmp);
+visit_type_int16(v, NULL, >value, _abort);
+g_assert_cmpint(tmp->value, ==, 2);
+tmp = (int16List *)visit_next_list(v, (GenericList *)tmp, sizeof(*res));
+g_assert(tmp);
+visit_end_list(v, (void **));
+/* BUG: unvisited tail not reported; actually not reportable by design */
+
+

[Qemu-devel] [PATCH 17/21] qapi: Drop unused non-strict qobject input visitor

[Markus Armbruster]

The split between tests/test-qobject-input-visitor.c and
tests/test-qobject-input-strict.c now makes less sense than ever.  The
next commit will take care of that.

Signed-off-by: Markus Armbruster 
---
 block/nbd.c  |  2 +-
 block/nfs.c  |  2 +-
 block/ssh.c  |  2 +-
 docs/qapi-code-gen.txt   |  2 +-
 include/qapi/qobject-input-visitor.h |  5 +
 qapi/qobject-input-visitor.c | 29 ++---
 qmp.c|  2 +-
 qom/qom-qobject.c|  2 +-
 scripts/qapi-commands.py |  2 +-
 target/s390x/cpu_models.c|  2 +-
 tests/check-qnull.c  |  2 +-
 tests/qmp-test.c |  2 +-
 tests/test-qmp-commands.c|  2 +-
 tests/test-qobject-input-strict.c|  2 +-
 tests/test-qobject-input-visitor.c   |  2 +-
 tests/test-visitor-serialization.c   |  2 +-
 16 files changed, 25 insertions(+), 37 deletions(-)

diff --git a/block/nbd.c b/block/nbd.c
index a7f9108..f478f80 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -278,7 +278,7 @@ static SocketAddress *nbd_config(BDRVNBDState *s, QDict 
*options, Error **errp)
 goto done;
 }
 
-iv = qobject_input_visitor_new(crumpled_addr, true);
+iv = qobject_input_visitor_new(crumpled_addr);
 visit_type_SocketAddress(iv, NULL, , _err);
 if (local_err) {
 error_propagate(errp, local_err);
diff --git a/block/nfs.c b/block/nfs.c
index 0cf115e..2884ad1 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -455,7 +455,7 @@ static NFSServer *nfs_config(QDict *options, Error **errp)
 goto out;
 }
 
-iv = qobject_input_visitor_new(crumpled_addr, true);
+iv = qobject_input_visitor_new(crumpled_addr);
 visit_type_NFSServer(iv, NULL, , _error);
 if (local_error) {
 error_propagate(errp, local_error);
diff --git a/block/ssh.c b/block/ssh.c
index 835932e..278e66f 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -601,7 +601,7 @@ static InetSocketAddress *ssh_config(QDict *options, Error 
**errp)
 goto out;
 }
 
-iv = qobject_input_visitor_new(crumpled_addr, true);
+iv = qobject_input_visitor_new(crumpled_addr);
 visit_type_InetSocketAddress(iv, NULL, , _error);
 if (local_error) {
 error_propagate(errp, local_error);
diff --git a/docs/qapi-code-gen.txt b/docs/qapi-code-gen.txt
index 7eb7be1..6746c10 100644
--- a/docs/qapi-code-gen.txt
+++ b/docs/qapi-code-gen.txt
@@ -1138,7 +1138,7 @@ Example:
 Visitor *v;
 UserDefOneList *arg1 = NULL;
 
-v = qobject_input_visitor_new(QOBJECT(args), true);
+v = qobject_input_visitor_new(QOBJECT(args));
 visit_start_struct(v, NULL, NULL, 0, );
 if (err) {
 goto out;
diff --git a/include/qapi/qobject-input-visitor.h 
b/include/qapi/qobject-input-visitor.h
index cde328d..21db9c4 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -21,10 +21,7 @@ typedef struct QObjectInputVisitor QObjectInputVisitor;
 
 /*
  * Return a new input visitor that converts a QObject to a QAPI object.
- *
- * Set @strict to reject a parse that doesn't consume all keys of a
- * dictionary; otherwise excess input is ignored.
  */
-Visitor *qobject_input_visitor_new(QObject *obj, bool strict);
+Visitor *qobject_input_visitor_new(QObject *obj);
 
 #endif
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 8015a98..97a27e2 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -43,9 +43,6 @@ struct QObjectInputVisitor {
  * QDict or QList). */
 QSLIST_HEAD(, StackObject) stack;
 
-/* True to reject parse in visit_end_struct() if unvisited keys remain. */
-bool strict;
-
 GString *errname;   /* Accumulator for full_name() */
 };
 
@@ -157,11 +154,12 @@ static const QListEntry 
*qobject_input_push(QObjectInputVisitor *qiv,
 tos->obj = obj;
 tos->qapi = qapi;
 
-if (qiv->strict && qobject_type(obj) == QTYPE_QDICT) {
+if (qobject_type(obj) == QTYPE_QDICT) {
 h = g_hash_table_new(g_str_hash, g_str_equal);
 qdict_iter(qobject_to_qdict(obj), qdict_add_key, h);
 tos->h = h;
-} else if (qobject_type(obj) == QTYPE_QLIST) {
+} else {
+assert(qobject_type(obj) == QTYPE_QLIST);
 tos->entry = qlist_first(qobject_to_qlist(obj));
 tos->index = -1;
 }
@@ -175,20 +173,15 @@ static void qobject_input_check_struct(Visitor *v, Error 
**errp)
 {
 QObjectInputVisitor *qiv = to_qiv(v);
 StackObject *tos = QSLIST_FIRST(>stack);
+GHashTableIter iter;
+const char *key;
 
 assert(tos && !tos->entry);
-if (qiv->strict) {
-GHashTable *const top_ht = tos->h;
-if (top_ht) {
-GHashTableIter iter;
-const char *key;
 
-g_hash_table_iter_init(, top_ht);
-if 

[Qemu-devel] [PATCH 18/21] tests-qobject-input-strict: Merge into test-qobject-input-visitor

[Markus Armbruster]

Much test-qobject-input-strict.c duplicates
test-qobject-input-strict.c less assertions on expected output:

* test_validate_struct() duplicates test_visitor_in_struct()

* test_validate_struct_nested() duplicates
  test_visitor_in_struct_nested()

* test_validate_list() duplicates the first half of
  test_visitor_in_list()

* test_validate_union_native_list() duplicates
  test_visitor_in_native_list_int()

* test_validate_union_flat() duplicates test_visitor_in_union_flat()

* test_validate_alternate() duplicates the first part of
  test_visitor_in_alternate()

Merge the remaining test cases into test-qobject-input-visitor.c, and
drop the now redundant test-qobject-input-strict.c.

Test case "/visitor/input-strict/fail/list" isn't really about lists,
it's about a bad struct nested in a list.  Rename accordingly.

Signed-off-by: Markus Armbruster 
---
 tests/Makefile.include |   4 +-
 tests/test-qobject-input-strict.c  | 381 -
 tests/test-qobject-input-visitor.c | 187 ++
 3 files changed, 188 insertions(+), 384 deletions(-)
 delete mode 100644 tests/test-qobject-input-strict.c

diff --git a/tests/Makefile.include b/tests/Makefile.include
index b212150..cb97473 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -28,7 +28,6 @@ check-unit-y += tests/test-clone-visitor$(EXESUF)
 gcov-files-test-clone-visitor-y = qapi/qapi-clone-visitor.c
 check-unit-y += tests/test-qobject-input-visitor$(EXESUF)
 gcov-files-test-qobject-input-visitor-y = qapi/qobject-input-visitor.c
-check-unit-y += tests/test-qobject-input-strict$(EXESUF)
 check-unit-y += tests/test-qmp-commands$(EXESUF)
 gcov-files-test-qmp-commands-y = qapi/qmp-dispatch.c
 check-unit-y += tests/test-string-input-visitor$(EXESUF)
@@ -490,7 +489,7 @@ test-obj-y = tests/check-qint.o tests/check-qstring.o 
tests/check-qdict.o \
tests/test-coroutine.o tests/test-string-output-visitor.o \
tests/test-string-input-visitor.o tests/test-qobject-output-visitor.o \
tests/test-clone-visitor.o \
-   tests/test-qobject-input-visitor.o tests/test-qobject-input-strict.o \
+   tests/test-qobject-input-visitor.o \
tests/test-qmp-commands.o tests/test-visitor-serialization.o \
tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
tests/test-opts-visitor.o tests/test-qmp-event.o \
@@ -599,7 +598,6 @@ tests/test-qmp-event$(EXESUF): tests/test-qmp-event.o 
$(test-qapi-obj-y)
 tests/test-qobject-output-visitor$(EXESUF): 
tests/test-qobject-output-visitor.o $(test-qapi-obj-y)
 tests/test-clone-visitor$(EXESUF): tests/test-clone-visitor.o 
$(test-qapi-obj-y)
 tests/test-qobject-input-visitor$(EXESUF): tests/test-qobject-input-visitor.o 
$(test-qapi-obj-y)
-tests/test-qobject-input-strict$(EXESUF): tests/test-qobject-input-strict.o 
$(test-qapi-obj-y)
 tests/test-qmp-commands$(EXESUF): tests/test-qmp-commands.o 
tests/test-qmp-marshal.o $(test-qapi-obj-y)
 tests/test-visitor-serialization$(EXESUF): tests/test-visitor-serialization.o 
$(test-qapi-obj-y)
 tests/test-opts-visitor$(EXESUF): tests/test-opts-visitor.o $(test-qapi-obj-y)
diff --git a/tests/test-qobject-input-strict.c 
b/tests/test-qobject-input-strict.c
deleted file mode 100644
index 7d26113..000
--- a/tests/test-qobject-input-strict.c
+++ /dev/null
@@ -1,381 +0,0 @@
-/*
- * QObject Input Visitor unit-tests (strict mode).
- *
- * Copyright (C) 2011-2012, 2015 Red Hat Inc.
- *
- * Authors:
- *  Luiz Capitulino 
- *  Paolo Bonzini 
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- */
-
-#include "qemu/osdep.h"
-
-#include "qemu-common.h"
-#include "qapi/error.h"
-#include "qapi/qobject-input-visitor.h"
-#include "test-qapi-types.h"
-#include "test-qapi-visit.h"
-#include "qapi/qmp/types.h"
-#include "qapi/qmp/qjson.h"
-#include "test-qmp-introspect.h"
-#include "qmp-introspect.h"
-#include "qapi-visit.h"
-
-typedef struct TestInputVisitorData {
-QObject *obj;
-Visitor *qiv;
-} TestInputVisitorData;
-
-static void validate_teardown(TestInputVisitorData *data,
-   const void *unused)
-{
-qobject_decref(data->obj);
-data->obj = NULL;
-
-if (data->qiv) {
-visit_free(data->qiv);
-data->qiv = NULL;
-}
-}
-
-/* The various test_init functions are provided instead of a test setup
-   function so that the JSON string used by the tests are kept in the test
-   functions (and not in main()). */
-static Visitor *validate_test_init_internal(TestInputVisitorData *data,
-const char *json_string,
-va_list *ap)
-{
-validate_teardown(data, NULL);
-
-data->obj = qobject_from_jsonv(json_string, ap);
-g_assert(data->obj);
-
-data->qiv = qobject_input_visitor_new(data->obj);
-

[Qemu-devel] [PATCH 21/21] qapi: Improve qobject visitor documentation

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 include/qapi/qobject-input-visitor.h  | 36 ++-
 include/qapi/qobject-output-visitor.h | 35 ++
 2 files changed, 66 insertions(+), 5 deletions(-)

diff --git a/include/qapi/qobject-input-visitor.h 
b/include/qapi/qobject-input-visitor.h
index 21db9c4..3ca233a 100644
--- a/include/qapi/qobject-input-visitor.h
+++ b/include/qapi/qobject-input-visitor.h
@@ -20,7 +20,41 @@
 typedef struct QObjectInputVisitor QObjectInputVisitor;
 
 /*
- * Return a new input visitor that converts a QObject to a QAPI object.
+ * Create a QObject input visitor for @obj
+ *
+ * A QObject input visitor visit builds a QAPI object from a QObject.
+ * This simultaneously walks the QAPI object being built and the
+ * QObject.  The latter walk starts at @obj.
+ *
+ * visit_type_FOO() creates an instance of QAPI type FOO.  The visited
+ * QObject must match FOO.  QDict matches struct/union types, QList
+ * matches list types, QString matches type 'str' and enumeration
+ * types, QInt matches integer types, QFloat matches type 'number',
+ * QBool matches type 'bool'.  Type 'any' is matched by QObject.  A
+ * QAPI alternate type is matched when one of its member types is.
+ *
+ * visit_start_struct() ... visit_end_struct() visits a QDict and
+ * creates a QAPI struct/union.  Visits in between visit the
+ * dictionary members.  visit_optional() is true when the QDict has
+ * this member.  visit_check_struct() fails if unvisited members
+ * remain.
+ *
+ * visit_start_list() ... visit_end_list() visits a QList and creates
+ * a QAPI list.  Visits in between visit list members, one after the
+ * other.  visit_next_list() returns NULL when all QList members have
+ * been visited.  visit_check_list() fails if unvisited members
+ * remain.
+ *
+ * visit_start_alternate() ... visit_end_alternate() visits a QObject
+ * and creates a QAPI alternate.  The visit in between visits the same
+ * QObject and initializes the alternate member that is in use.
+ *
+ * Error messages refer to parts of @obj in JavaScript/Python syntax.
+ * For example, 'a.b[2]' refers to the second member of the QList
+ * member 'b' of the QDict member 'a' of QDict @obj.
+ *
+ * The caller is responsible for freeing the visitor with
+ * visit_free().
  */
 Visitor *qobject_input_visitor_new(QObject *obj);
 
diff --git a/include/qapi/qobject-output-visitor.h 
b/include/qapi/qobject-output-visitor.h
index 8241877..9b990c3 100644
--- a/include/qapi/qobject-output-visitor.h
+++ b/include/qapi/qobject-output-visitor.h
@@ -19,11 +19,38 @@
 
 typedef struct QObjectOutputVisitor QObjectOutputVisitor;
 
-/*
- * Create a new QObject output visitor.
+/**
+ * Create a QObject output visitor for @obj
  *
- * If everything else succeeds, pass @result to visit_complete() to
- * collect the result of the visit.
+ * A QObject output visitor visit builds a QObject from QAPI Object.
+ * This simultaneously walks the QAPI object and the QObject being
+ * built.  The latter walk starts at @obj.
+ *
+ * visit_type_FOO() creates a QObject for QAPI type FOO.  It creates a
+ * QDict for struct/union types, a QList for list types, QString for
+ * type 'str' and enumeration types, QInt for integer types, QFloat
+ * for type 'number', QBool for type 'bool'.  For type 'any', it
+ * increments the QObject's reference count.  For QAPI alternate
+ * types, it creates the QObject for the member that is in use.
+ *
+ * visit_start_struct() ... visit_end_struct() visits a QAPI
+ * struct/union and creates a QDict.  Visits in between visit the
+ * members.  visit_optional() is true when the struct/union has this
+ * member.  visit_check_struct() does nothing.
+ *
+ * visit_start_list() ... visit_end_list() visits a QAPI list and
+ * creates a QList.  Visits in between visit list members, one after
+ * the other.  visit_next_list() returns NULL when all QAPI list
+ * members have been visited.  visit_check_list() does nothing.
+ *
+ * visit_start_alternate() ... visit_end_alternate() visits a QAPI
+ * alternate.  The visit in between creates the QObject for the
+ * alternate member that is in use.
+ *
+ * Errors are not expected to happen.
+ *
+ * The caller is responsible for freeing the visitor with
+ * visit_free().
  */
 Visitor *qobject_output_visitor_new(QObject **result);
 
-- 
2.7.4

[Qemu-devel] [PATCH 15/21] qom: Make object_property_set_qobject()'s input visitor strict

[Markus Armbruster]

Commit 240f64b made all qobject input visitors created outside tests
strict, except for the one in object_property_set_qobject().  That one
was left behind only because Eric couldn't spare the time to figure
out whether making it strict would break anything, with a TODO
comment.  Time to resolve it.

Strict makes a difference only for otherwise successful visits of QAPI
structs or unions.  Let's examine what the callers of
object_property_set_qobject() visit:

* object_property_set_str(), object_property_set_bool(),
  object_property_set_int() visit a QString, QBool, QInt,
  respectively.  Strictness can't matter.

* qmp_qom_set visits its @value argument.  Comes straight from QMP and
  can be anything ('any' in the QAPI schema).  Strictness matters when
  the property's set() method visits a struct or union QAPI type.

  No such methods exist, thus switching to strict can't break
  anything.

  If we acquire such methods in the future, we'll *want* the visitor
  to be strict, so that unexpected members get rejected as they should
  be.

Switch to strict.

Signed-off-by: Markus Armbruster 
---
 qom/qom-qobject.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qom/qom-qobject.c b/qom/qom-qobject.c
index 447e4a0..bbdedda 100644
--- a/qom/qom-qobject.c
+++ b/qom/qom-qobject.c
@@ -22,8 +22,8 @@ void object_property_set_qobject(Object *obj, QObject *value,
  const char *name, Error **errp)
 {
 Visitor *v;
-/* TODO: Should we reject, rather than ignore, excess input? */
-v = qobject_input_visitor_new(value, false);
+
+v = qobject_input_visitor_new(value, true);
 object_property_set(obj, v, name, errp);
 visit_free(v);
 }
-- 
2.7.4

[Qemu-devel] [PATCH 12/21] qapi: Improve qobject input visitor error reporting

[Markus Armbruster]

Error messages refer to nodes of the QObject being visited by name.
Trouble is the names are sometimes less than helpful:

* The name of the root QObject is whatever @name argument got passed
  to the visitor, except NULL gets mapped to "null".  We commonly pass
  NULL.  Not good.

  Avoiding errors "at the root" mitigates.  For instance,
  visit_start_struct() can only fail when the visited object is not a
  dictionary, and we commonly ensure it is beforehand.

* The name of a QDict's member is the member key.  Good enough only
  when this happens to be unique.

* The name of a QList's member is "null".  Not good.

Improve error messages by referring to nodes by path instead, as
follows:

* The path of the root QObject is whatever @name argument got passed
  to the visitor, except NULL gets mapped to "".

* The path of a root QDict's member is the member key.

* The path of a root QList's member is "[%zu]", where %zu is the list
  index, starting at zero.

* The path of a non-root QDict's member is the path of the QDict
  concatenated with "." and the member key.

* The path of a non-root QList's member is the path of the QList
  concatenated with "[%zu]", where %zu is the list index.

For example, the incorrect QMP command

{ "execute": "blockdev-add", "arguments": { "node-name": "foo", "driver": 
"raw", "file": {"driver": "file" } } }

now fails with

{"error": {"class": "GenericError", "desc": "Parameter 'file.filename' is 
missing"}}

instead of

{"error": {"class": "GenericError", "desc": "Parameter 'filename' is 
missing"}}

and

{ "execute": "input-send-event", "arguments": { "device": "bar", "events": 
[ [] ] } }

now fails with

{"error": {"class": "GenericError", "desc": "Invalid parameter type for 
'events[0]', expected: object"}}

instead of

{"error": {"class": "GenericError", "desc": "Invalid parameter type for 
'null', expected: QDict"}}

Aside: calling the thing "parameter" is suboptimal for QMP, because
the root object is "arguments" there.

The qobject output visitor doesn't have this problem because it should
not fail.  Same for dealloc and clone visitors.

The string visitors don't have this problem because they visit just
one value, whose name needs to be passed to the visitor as @name.  The
string output visitor shouldn't fail anyway.

The options visitor uses QemuOpts names.  Their name space is flat, so
the use of QDict member keys as names is fine.  NULL names used with
roots and lists could conceivably result in bad error messages.  Left
for another day.

Signed-off-by: Markus Armbruster 
---
 include/qapi/visitor.h   |   6 ---
 qapi/qobject-input-visitor.c | 121 +++
 2 files changed, 87 insertions(+), 40 deletions(-)

diff --git a/include/qapi/visitor.h b/include/qapi/visitor.h
index 9bb6cba..7c91a50 100644
--- a/include/qapi/visitor.h
+++ b/include/qapi/visitor.h
@@ -66,12 +66,6 @@
  * object, @name is the key associated with the value; and when
  * visiting a member of a list, @name is NULL.
  *
- * FIXME: Clients must pass NULL for @name when visiting a member of a
- * list, but this leads to poor error messages; it might be nicer to
- * require a non-NULL name such as "key.0" for '{ "key": [ "value" ]
- * }' if an error is encountered on "value" (or to have the visitor
- * core auto-generate the nicer name).
- *
  * The visit_type_FOO() functions expect a non-null @obj argument;
  * they allocate *@obj during input visits, leave it unchanged on
  * output visits, and recursively free any resources during a dealloc
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index d58696c..8015a98 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -21,19 +21,19 @@
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qerror.h"
 
-typedef struct StackObject
-{
-QObject *obj; /* Object being visited */
+typedef struct StackObject {
+const char *name;/* Name of @obj in its parent, if any */
+QObject *obj;/* QDict or QList being visited */
 void *qapi; /* sanity check that caller uses same pointer */
 
-GHashTable *h;   /* If obj is dict: unvisited keys */
-const QListEntry *entry; /* If obj is list: unvisited tail */
+GHashTable *h;  /* If @obj is QDict: unvisited keys */
+const QListEntry *entry;/* If @obj is QList: unvisited tail */
+unsigned index; /* If @obj is QList: list index of @entry */
 
-QSLIST_ENTRY(StackObject) node;
+QSLIST_ENTRY(StackObject) node; /* parent */
 } StackObject;
 
-struct QObjectInputVisitor
-{
+struct QObjectInputVisitor {
 Visitor visitor;
 
 /* Root of visit at visitor creation. */
@@ -45,6 +45,8 @@ struct QObjectInputVisitor
 
 /* True to reject parse in visit_end_struct() if unvisited keys remain. */
 bool strict;
+
+GString *errname;   /* Accumulator for full_name() */
 };
 
 static 

[Qemu-devel] [PATCH 07/21] qmp: Eliminate silly QERR_QMP_* macros

[Markus Armbruster]

The QERR_ macros are leftovers from the days of "rich" error objects.

QERR_QMP_BAD_INPUT_OBJECT, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
QERR_QMP_EXTRA_MEMBER are used in just one place now, except for one
use that has crept into qobject-input-visitor.c.

Drop these macros, to make the (bad) error messages more visible.

Signed-off-by: Markus Armbruster 
---
 include/qapi/qmp/qerror.h|  9 -
 qapi/qmp-dispatch.c  | 13 +++--
 qapi/qobject-input-visitor.c |  3 ++-
 3 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
index 6586c9f..c82360f 100644
--- a/include/qapi/qmp/qerror.h
+++ b/include/qapi/qmp/qerror.h
@@ -82,15 +82,6 @@
 #define QERR_QGA_COMMAND_FAILED \
 "Guest agent command failed, error was '%s'"
 
-#define QERR_QMP_BAD_INPUT_OBJECT \
-"Expected '%s' in QMP input"
-
-#define QERR_QMP_BAD_INPUT_OBJECT_MEMBER \
-"QMP input object member '%s' expects '%s'"
-
-#define QERR_QMP_EXTRA_MEMBER \
-"QMP input object member '%s' is unexpected"
-
 #define QERR_SET_PASSWD_FAILED \
 "Could not set password"
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 4fc1122..377ebb5 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 
 dict = qobject_to_qdict(request);
 if (!dict) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
+error_setg(errp, "Expected '%s' in QMP input", "object");
 return NULL;
 }
 
@@ -41,25 +41,26 @@ static QDict *qmp_dispatch_check_obj(const QObject 
*request, Error **errp)
 
 if (!strcmp(arg_name, "execute")) {
 if (qobject_type(arg_obj) != QTYPE_QSTRING) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER, "execute",
-   "string");
+error_setg(errp, "QMP input object member '%s' expects '%s'",
+   "execute", "string");
 return NULL;
 }
 has_exec_key = true;
 } else if (!strcmp(arg_name, "arguments")) {
 if (qobject_type(arg_obj) != QTYPE_QDICT) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+error_setg(errp, "QMP input object member '%s' expects '%s'",
"arguments", "object");
 return NULL;
 }
 } else {
-error_setg(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
+error_setg(errp, "QMP input object member '%s' is unexpected",
+   arg_name);
 return NULL;
 }
 }
 
 if (!has_exec_key) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "execute");
+error_setg(errp, "Expected '%s' in QMP input", "execute");
 return NULL;
 }
 
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 0063327..ed6c24c 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -140,7 +140,8 @@ static void qobject_input_check_struct(Visitor *v, Error 
**errp)
 
 g_hash_table_iter_init(, top_ht);
 if (g_hash_table_iter_next(, (void **), NULL)) {
-error_setg(errp, QERR_QMP_EXTRA_MEMBER, key);
+error_setg(errp, "QMP input object member '%s' is unexpected",
+   key);
 }
 }
 }
-- 
2.7.4

[Qemu-devel] [PATCH 13/21] qapi: Drop string input visitor method optional()

[Markus Armbruster]

visit_optional() is to be called only between visit_start_struct() and
visit_end_struct().  Visitors that don't support struct visits,
i.e. don't implement start_struct(), end_struct(), have no use for it.
Clarify documentation.

The string input visitor doesn't support struct visits.  Its
parse_optional() is therefore useless.  Drop it.

Signed-off-by: Markus Armbruster 
---
 include/qapi/visitor-impl.h |  4 ++--
 qapi/string-input-visitor.c | 13 -
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/include/qapi/visitor-impl.h b/include/qapi/visitor-impl.h
index 8bd47ee..962ba1d 100644
--- a/include/qapi/visitor-impl.h
+++ b/include/qapi/visitor-impl.h
@@ -102,8 +102,8 @@ struct Visitor
 /* Must be set to visit explicit null values.  */
 void (*type_null)(Visitor *v, const char *name, Error **errp);
 
-/* Must be set for input visitors, optional otherwise.  The core
- * takes care of the return type in the public interface. */
+/* Must be set for input visitors to visit structs, optional otherwise.
+   The core takes care of the return type in the public interface. */
 void (*optional)(Visitor *v, const char *name, bool *present);
 
 /* Must be set */
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 8dfa561..1a855c5 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -314,18 +314,6 @@ static void parse_type_number(Visitor *v, const char 
*name, double *obj,
 *obj = val;
 }
 
-static void parse_optional(Visitor *v, const char *name, bool *present)
-{
-StringInputVisitor *siv = to_siv(v);
-
-if (!siv->string) {
-*present = false;
-return;
-}
-
-*present = true;
-}
-
 static void string_input_free(Visitor *v)
 {
 StringInputVisitor *siv = to_siv(v);
@@ -351,7 +339,6 @@ Visitor *string_input_visitor_new(const char *str)
 v->visitor.start_list = start_list;
 v->visitor.next_list = next_list;
 v->visitor.end_list = end_list;
-v->visitor.optional = parse_optional;
 v->visitor.free = string_input_free;
 
 v->string = str;
-- 
2.7.4

[Qemu-devel] [PATCH 05/21] qmp: Clean up how we enforce capability negotiation

[Markus Armbruster]

To enforce capability negotiation before normal operation,
handle_qmp_command() inspects every command before it's handed off to
qmp_dispatch().  This is a bit of a layering violation, and results in
duplicated code.

Before capability negotiation (!cur_mon->in_command_mode), we fail
commands other than "qmp_capabilities".  This is what enforces
capability negotiation.

Afterwards, we fail command "qmp_capabilities".

Clean this up as follows.

The obvious place to fail a command is the command itself, so move the
"afterwards" check to qmp_qmp_capabilities().

We do the "before" check in every other command, but that would be
bothersome.  Instead, start without all the other commands, by
registering only qmp_qmp_capabilities().  Register the others in
qmp_qmp_capabilities().

Additionally, replace the generic human-readable error message for
CommandNotFound by one that reminds the user to run qmp_capabilities.
Without that, we'd regress commit 2d5a834.

Signed-off-by: Markus Armbruster 
---
 monitor.c | 70 ---
 1 file changed, 40 insertions(+), 30 deletions(-)

diff --git a/monitor.c b/monitor.c
index 5be71d6..dcf2de7 100644
--- a/monitor.c
+++ b/monitor.c
@@ -563,11 +563,6 @@ static void monitor_qapi_event_init(void)
 qmp_event_set_func_emit(monitor_qapi_event_queue);
 }
 
-void qmp_qmp_capabilities(Error **errp)
-{
-cur_mon->qmp.in_command_mode = true;
-}
-
 static void handle_hmp_command(Monitor *mon, const char *cmdline);
 
 static void monitor_data_init(Monitor *mon)
@@ -995,7 +990,7 @@ static void qmp_unregister_commands_hack(void)
 #endif
 }
 
-void monitor_init_qmp_commands(void)
+static void monitor_init_real_qmp_commands(void)
 {
 qmp_init_marshal();
 
@@ -1009,6 +1004,32 @@ void monitor_init_qmp_commands(void)
 qmp_unregister_commands_hack();
 }
 
+void monitor_init_qmp_commands(void)
+{
+/*
+ * Start with just qmp_capabilities, to enforce capability
+ * negotiation.  qmp_capabilities will register the other
+ * commands.  See also the error message replacement hack in
+ * handle_qmp_command().
+ */
+qmp_register_command("qmp_capabilities",
+ qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS);
+}
+
+void qmp_qmp_capabilities(Error **errp)
+{
+if (cur_mon->qmp.in_command_mode) {
+error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
+  "Capabilities negotiation is already complete, command "
+  "ignored");
+return;
+}
+
+cur_mon->qmp.in_command_mode = true;
+qmp_unregister_command("qmp_capabilities");
+monitor_init_real_qmp_commands();
+}
+
 /* set the current CPU defined by the user */
 int monitor_set_cpu(int cpu_index)
 {
@@ -3676,26 +3697,6 @@ static int monitor_can_read(void *opaque)
 return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
-static bool invalid_qmp_mode(const Monitor *mon, const char *cmd,
- Error **errp)
-{
-bool is_cap = g_str_equal(cmd, "qmp_capabilities");
-
-if (is_cap && mon->qmp.in_command_mode) {
-error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
-  "Capabilities negotiation is already complete, command "
-  "'%s' ignored", cmd);
-return true;
-}
-if (!is_cap && !mon->qmp.in_command_mode) {
-error_set(errp, ERROR_CLASS_COMMAND_NOT_FOUND,
-  "Expecting capabilities negotiation with "
-  "'qmp_capabilities' before command '%s'", cmd);
-return true;
-}
-return false;
-}
-
 /*
  * Input object checking rules
  *
@@ -3781,12 +3782,21 @@ static void handle_qmp_command(JSONMessageParser 
*parser, GQueue *tokens)
 cmd_name = qdict_get_str(qdict, "execute");
 trace_handle_qmp_command(mon, cmd_name);
 
-if (invalid_qmp_mode(mon, cmd_name, )) {
-goto err_out;
-}
-
 rsp = qmp_dispatch(req);
 
+qdict = qdict_get_qdict(qobject_to_qdict(rsp), "error");
+if (qdict) {
+if (!mon->qmp.in_command_mode
+&& !strcmp(qdict_get_try_str(qdict, "class"),
+   QapiErrorClass_lookup[ERROR_CLASS_COMMAND_NOT_FOUND])) {
+/* Provide a more useful error message */
+qdict_del(qdict, "desc");
+qdict_put(qdict, "desc",
+  qstring_from_str("Expecting capabilities negotiation"
+   " with 'qmp_capabilities'"));
+}
+}
+
 err_out:
 if (err) {
 qdict = qdict_new();
-- 
2.7.4

[Qemu-devel] [PATCH 02/21] libqtest: Work around a "QMP wants a newline" bug

[Markus Armbruster]

The next commit is going to add a test that calls qmp("null").
Curiously, this hangs.  Here's why.

qmp_fd_sendv() doesn't send newlines.  Not even when @fmt contains
some.  At first glance, the QMP parser seems to be fine with that.
However, it turns out that it fails to react to input until it sees
either a newline, an object or an array.  To reproduce, feed to a QMP
monitor like this:

$ echo -n 'null' | socat UNIX:/work/armbru/images/test-qmp STDIO
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 8, "major": 2}, 
"package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}

No output after the greeting.

Add a newline:

$ echo 'null' | socat UNIX:/work/armbru/images/test-qmp STDIO
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 8, "major": 2}, 
"package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}
{"error": {"class": "GenericError", "desc": "Expected 'object' in QMP 
input"}}

Correct output for input 'null'.

Add an object instead:

$ echo -n 'null { "execute": "qmp_capabilities" }' | socat UNIX:qmp-socket 
STDIO
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 8, "major": 2}, 
"package": " (v2.8.0-1195-gf84141e-dirty)"}, "capabilities": []}}
{"error": {"class": "GenericError", "desc": "Expected 'object' in QMP 
input"}}
{"return": {}}

Also correct output.

Work around this QMP bug by having qmp_fd_sendv() append a newline.

Signed-off-by: Markus Armbruster 
---
 tests/libqtest.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index e54354d..ad23ce9 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -442,14 +442,20 @@ void qmp_fd_sendv(int fd, const char *fmt, va_list ap)
 if (qobj) {
 int log = getenv("QTEST_LOG") != NULL;
 QString *qstr = qobject_to_json(qobj);
-const char *str = qstring_get_str(qstr);
-size_t size = qstring_get_length(qstr);
+const char *str;
+
+/*
+ * BUG: QMP doesn't react to input until it sees a newline, an
+ * object, or an array.  Work-around: give it a newline.
+ */
+qstring_append_chr(qstr, '\n');
+str = qstring_get_str(qstr);
 
 if (log) {
 fprintf(stderr, "%s", str);
 }
 /* Send QMP request */
-socket_send(fd, str, size);
+socket_send(fd, str, qstring_get_length(qstr));
 
 QDECREF(qstr);
 qobject_decref(qobj);
-- 
2.7.4

[Qemu-devel] [PATCH 10/21] qapi: Clean up after commit 3d344c2

[Markus Armbruster]

Drop unused QIV_STACK_SIZE and unused qobject_input_start_struct()
parameter errp.

Signed-off-by: Markus Armbruster 
---
 qapi/qobject-input-visitor.c | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index f3b6713..2c2f883 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -21,8 +21,6 @@
 #include "qapi/qmp/types.h"
 #include "qapi/qmp/qerror.h"
 
-#define QIV_STACK_SIZE 1024
-
 typedef struct StackObject
 {
 QObject *obj; /* Object being visited */
@@ -103,8 +101,7 @@ static void qdict_add_key(const char *key, QObject *obj, 
void *opaque)
 }
 
 static const QListEntry *qobject_input_push(QObjectInputVisitor *qiv,
-QObject *obj, void *qapi,
-Error **errp)
+QObject *obj, void *qapi)
 {
 GHashTable *h;
 StackObject *tos = g_new0(StackObject, 1);
@@ -170,7 +167,6 @@ static void qobject_input_start_struct(Visitor *v, const 
char *name, void **obj,
 {
 QObjectInputVisitor *qiv = to_qiv(v);
 QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
-Error *err = NULL;
 
 if (obj) {
 *obj = NULL;
@@ -184,11 +180,7 @@ static void qobject_input_start_struct(Visitor *v, const 
char *name, void **obj,
 return;
 }
 
-qobject_input_push(qiv, qobj, obj, );
-if (err) {
-error_propagate(errp, err);
-return;
-}
+qobject_input_push(qiv, qobj, obj);
 
 if (obj) {
 *obj = g_malloc0(size);
@@ -216,7 +208,7 @@ static void qobject_input_start_list(Visitor *v, const char 
*name,
 return;
 }
 
-entry = qobject_input_push(qiv, qobj, list, errp);
+entry = qobject_input_push(qiv, qobj, list);
 if (list) {
 if (entry) {
 *list = g_malloc0(size);
-- 
2.7.4

[Qemu-devel] [PATCH 14/21] qapi: Make string input and opts visitor require non-null input

[Markus Armbruster]

The string input visitor tries to cope with null input.  Null input
isn't used anywhere, and isn't covered by tests.  Unsurprisingly, it
doesn't fully work: start_list() crashes because it passes the input
via parse_str() to strtoll() unchecked.

Make string_input_visitor_new() assert its argument isn't null, and
drop the code trying to deal with null input.

The opts visitor crashes when you try to actually visit something with
null input.  Make opts_visitor_new() assert its argument isn't null,
mostly for clarity.

qobject_input_visitor_new() already asserts its argument isn't null.

Signed-off-by: Markus Armbruster 
---
 qapi/opts-visitor.c |  1 +
 qapi/string-input-visitor.c | 54 ++---
 2 files changed, 18 insertions(+), 37 deletions(-)

diff --git a/qapi/opts-visitor.c b/qapi/opts-visitor.c
index a0a7c0e..c50dc4b 100644
--- a/qapi/opts-visitor.c
+++ b/qapi/opts-visitor.c
@@ -528,6 +528,7 @@ opts_visitor_new(const QemuOpts *opts)
 {
 OptsVisitor *ov;
 
+assert(opts);
 ov = g_malloc0(sizeof *ov);
 
 ov->visitor.type = VISITOR_INPUT;
diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
index 1a855c5..f126cd9 100644
--- a/qapi/string-input-visitor.c
+++ b/qapi/string-input-visitor.c
@@ -182,12 +182,6 @@ static void parse_type_int64(Visitor *v, const char *name, 
int64_t *obj,
 {
 StringInputVisitor *siv = to_siv(v);
 
-if (!siv->string) {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
-   "integer");
-return;
-}
-
 if (parse_str(siv, name, errp) < 0) {
 return;
 }
@@ -242,13 +236,7 @@ static void parse_type_size(Visitor *v, const char *name, 
uint64_t *obj,
 Error *err = NULL;
 uint64_t val;
 
-if (siv->string) {
-parse_option_size(name, siv->string, , );
-} else {
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
-   "size");
-return;
-}
+parse_option_size(name, siv->string, , );
 if (err) {
 error_propagate(errp, err);
 return;
@@ -262,19 +250,17 @@ static void parse_type_bool(Visitor *v, const char *name, 
bool *obj,
 {
 StringInputVisitor *siv = to_siv(v);
 
-if (siv->string) {
-if (!strcasecmp(siv->string, "on") ||
-!strcasecmp(siv->string, "yes") ||
-!strcasecmp(siv->string, "true")) {
-*obj = true;
-return;
-}
-if (!strcasecmp(siv->string, "off") ||
-!strcasecmp(siv->string, "no") ||
-!strcasecmp(siv->string, "false")) {
-*obj = false;
-return;
-}
+if (!strcasecmp(siv->string, "on") ||
+!strcasecmp(siv->string, "yes") ||
+!strcasecmp(siv->string, "true")) {
+*obj = true;
+return;
+}
+if (!strcasecmp(siv->string, "off") ||
+!strcasecmp(siv->string, "no") ||
+!strcasecmp(siv->string, "false")) {
+*obj = false;
+return;
 }
 
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
@@ -285,13 +271,8 @@ static void parse_type_str(Visitor *v, const char *name, 
char **obj,
Error **errp)
 {
 StringInputVisitor *siv = to_siv(v);
-if (siv->string) {
-*obj = g_strdup(siv->string);
-} else {
-*obj = NULL;
-error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
-   "string");
-}
+
+*obj = g_strdup(siv->string);
 }
 
 static void parse_type_number(Visitor *v, const char *name, double *obj,
@@ -302,10 +283,8 @@ static void parse_type_number(Visitor *v, const char 
*name, double *obj,
 double val;
 
 errno = 0;
-if (siv->string) {
-val = strtod(siv->string, );
-}
-if (!siv->string || errno || endp == siv->string || *endp) {
+val = strtod(siv->string, );
+if (errno || endp == siv->string || *endp) {
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"number");
 return;
@@ -327,6 +306,7 @@ Visitor *string_input_visitor_new(const char *str)
 {
 StringInputVisitor *v;
 
+assert(str);
 v = g_malloc0(sizeof(*v));
 
 v->visitor.type = VISITOR_INPUT;
-- 
2.7.4

[Qemu-devel] [PATCH 06/21] qmp: Drop duplicated QMP command object checks

[Markus Armbruster]

qmp_check_input_obj() duplicates qmp_dispatch_check_obj(), except the
latter screws up an error message.  handle_qmp_command() runs first
the former, then the latter via qmp_dispatch(), masking the screwup.

qemu-ga also masks the screwup, because it also duplicates checks,
just differently.

qmp_check_input_obj() exists because handle_qmp_command() needs to
examine the command before dispatching it.  The previous commit got
rid of this need, except for a tracepoint, and a bit of "id" code that
relies on qdict not being null.

Fix up the error message in qmp_dispatch_check_obj(), drop
qmp_check_input_obj() and the tracepoint.  Protect the "id" code with
a conditional.

Signed-off-by: Markus Armbruster 
---
 monitor.c   | 74 +
 qapi/qmp-dispatch.c |  3 +--
 trace-events|  1 -
 3 files changed, 7 insertions(+), 71 deletions(-)

diff --git a/monitor.c b/monitor.c
index dcf2de7..d83888d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -3697,67 +3697,10 @@ static int monitor_can_read(void *opaque)
 return (mon->suspend_cnt == 0) ? 1 : 0;
 }
 
-/*
- * Input object checking rules
- *
- * 1. Input object must be a dict
- * 2. The "execute" key must exist
- * 3. The "execute" key must be a string
- * 4. If the "arguments" key exists, it must be a dict
- * 5. If the "id" key exists, it can be anything (ie. json-value)
- * 6. Any argument not listed above is considered invalid
- */
-static QDict *qmp_check_input_obj(QObject *input_obj, Error **errp)
-{
-const QDictEntry *ent;
-int has_exec_key = 0;
-QDict *input_dict;
-
-input_dict = qobject_to_qdict(input_obj);
-if (!input_dict) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
-return NULL;
-}
-
-
-for (ent = qdict_first(input_dict); ent; ent = qdict_next(input_dict, 
ent)){
-const char *arg_name = qdict_entry_key(ent);
-const QObject *arg_obj = qdict_entry_value(ent);
-
-if (!strcmp(arg_name, "execute")) {
-if (qobject_type(arg_obj) != QTYPE_QSTRING) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
-   "execute", "string");
-return NULL;
-}
-has_exec_key = 1;
-} else if (!strcmp(arg_name, "arguments")) {
-if (qobject_type(arg_obj) != QTYPE_QDICT) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
-   "arguments", "object");
-return NULL;
-}
-} else if (!strcmp(arg_name, "id")) {
-/* Any string is acceptable as "id", so nothing to check */
-} else {
-error_setg(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
-return NULL;
-}
-}
-
-if (!has_exec_key) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "execute");
-return NULL;
-}
-
-return input_dict;
-}
-
 static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
 {
 QObject *req, *rsp = NULL, *id = NULL;
 QDict *qdict = NULL;
-const char *cmd_name;
 Monitor *mon = cur_mon;
 Error *err = NULL;
 
@@ -3770,17 +3713,12 @@ static void handle_qmp_command(JSONMessageParser 
*parser, GQueue *tokens)
 goto err_out;
 }
 
-qdict = qmp_check_input_obj(req, );
-if (!qdict) {
-goto err_out;
-}
-
-id = qdict_get(qdict, "id");
-qobject_incref(id);
-qdict_del(qdict, "id");
-
-cmd_name = qdict_get_str(qdict, "execute");
-trace_handle_qmp_command(mon, cmd_name);
+qdict = qobject_to_qdict(req);
+if (qdict) {
+id = qdict_get(qdict, "id");
+qobject_incref(id);
+qdict_del(qdict, "id");
+} /* else will fail qmp_dispatch() */
 
 rsp = qmp_dispatch(req);
 
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 621922f..4fc1122 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,8 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 
 dict = qobject_to_qdict(request);
 if (!dict) {
-error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT,
-   "request is not a dictionary");
+error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT, "object");
 return NULL;
 }
 
diff --git a/trace-events b/trace-events
index 7288557..b07a09b 100644
--- a/trace-events
+++ b/trace-events
@@ -65,7 +65,6 @@ xen_remap_bucket(uint64_t index) "index %#"PRIx64
 xen_map_cache_return(void* ptr) "%p"
 
 # monitor.c
-handle_qmp_command(void *mon, const char *cmd_name) "mon %p cmd_name \"%s\""
 monitor_protocol_event_handler(uint32_t event, void *qdict) "event=%d data=%p"
 monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p"
 monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) 
"event=%d data=%p rate=%" PRId64
-- 
2.7.4

[Qemu-devel] [PATCH 08/21] qmp: Improve QMP dispatch error messages

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
---
 qapi/qmp-dispatch.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 377ebb5..4610b6d 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -30,7 +30,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 
 dict = qobject_to_qdict(request);
 if (!dict) {
-error_setg(errp, "Expected '%s' in QMP input", "object");
+error_setg(errp, "QMP input must be a JSON object");
 return NULL;
 }
 
@@ -41,15 +41,17 @@ static QDict *qmp_dispatch_check_obj(const QObject 
*request, Error **errp)
 
 if (!strcmp(arg_name, "execute")) {
 if (qobject_type(arg_obj) != QTYPE_QSTRING) {
-error_setg(errp, "QMP input object member '%s' expects '%s'",
-   "execute", "string");
+error_setg(errp,
+   "QMP input object member '%s' must be %s",
+   "execute", "a string");
 return NULL;
 }
 has_exec_key = true;
 } else if (!strcmp(arg_name, "arguments")) {
 if (qobject_type(arg_obj) != QTYPE_QDICT) {
-error_setg(errp, "QMP input object member '%s' expects '%s'",
-   "arguments", "object");
+error_setg(errp,
+   "QMP input object member '%s' must be %s",
+   "arguments", "an object");
 return NULL;
 }
 } else {
@@ -60,7 +62,7 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 }
 
 if (!has_exec_key) {
-error_setg(errp, "Expected '%s' in QMP input", "execute");
+error_setg(errp, "QMP input object lacks key 'execute'");
 return NULL;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 04/21] qmp: Dumb down how we run QMP command registration

[Markus Armbruster]

The way we get QMP commands registered is high tech:

* qapi-commands.py generates qmp_init_marshal() that does the actual work

* it also generates the magic to register it as a MODULE_INIT_QAPI
  function, so it runs when someone calls
  module_call_init(MODULE_INIT_QAPI)

* main() calls module_call_init()

QEMU needs to register a few non-qapified commands.  Same high tech
works: monitor.c has its own qmp_init_marshal() along with the magic
to make it run in module_call_init(MODULE_INIT_QAPI).

QEMU also needs to unregister commands that are not wanted in this
build's configuration (commit 5032a16).  Simple enough:
qmp_unregister_commands_hack().  The difficulty is to make it run
after the generated qmp_init_marshal().  We can't simply run it in
monitor.c's qmp_init_marshal(), because the order in which the
registered functions run is indeterminate.  So qmp_init_marshal()
registers qmp_init_marshal() separately.  Since registering *appends*
to the list of registered functions, this will make it run after all
the functions that have been registered already.

I suspect it takes a long and expensive computer science education to
not find this silly.

Dumb it down as follows:

* Drop MODULE_INIT_QAPI entirely

* Give the generated qmp_init_marshal() external linkage.

* Call it instead of module_call_init(MODULE_INIT_QAPI)

* Except in QEMU proper, call new monitor_init_qmp_commands() that in
  turn calls the generated qmp_init_marshal(), registers the
  additional commands and unregisters the unwanted ones.

Signed-off-by: Markus Armbruster 
---
 include/monitor/monitor.h | 1 +
 include/qemu/module.h | 2 --
 monitor.c | 9 -
 qga/main.c| 2 +-
 scripts/qapi-commands.py  | 5 ++---
 tests/test-qmp-commands.c | 2 +-
 vl.c  | 2 +-
 7 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index e64b944..d2b3aaf 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -16,6 +16,7 @@ extern Monitor *cur_mon;
 
 bool monitor_cur_is_qmp(void);
 
+void monitor_init_qmp_commands(void);
 void monitor_init(Chardev *chr, int flags);
 void monitor_cleanup(void);
 
diff --git a/include/qemu/module.h b/include/qemu/module.h
index 877cca7..56dd218 100644
--- a/include/qemu/module.h
+++ b/include/qemu/module.h
@@ -42,7 +42,6 @@ static void __attribute__((constructor)) do_qemu_init_ ## 
function(void)\
 typedef enum {
 MODULE_INIT_BLOCK,
 MODULE_INIT_OPTS,
-MODULE_INIT_QAPI,
 MODULE_INIT_QOM,
 MODULE_INIT_TRACE,
 MODULE_INIT_MAX
@@ -50,7 +49,6 @@ typedef enum {
 
 #define block_init(function) module_init(function, MODULE_INIT_BLOCK)
 #define opts_init(function) module_init(function, MODULE_INIT_OPTS)
-#define qapi_init(function) module_init(function, MODULE_INIT_QAPI)
 #define type_init(function) module_init(function, MODULE_INIT_QOM)
 #define trace_init(function) module_init(function, MODULE_INIT_TRACE)
 
diff --git a/monitor.c b/monitor.c
index 56867e8..5be71d6 100644
--- a/monitor.c
+++ b/monitor.c
@@ -995,8 +995,10 @@ static void qmp_unregister_commands_hack(void)
 #endif
 }
 
-static void qmp_init_marshal(void)
+void monitor_init_qmp_commands(void)
 {
+qmp_init_marshal();
+
 qmp_register_command("query-qmp-schema", qmp_query_qmp_schema,
  QCO_NO_OPTIONS);
 qmp_register_command("device_add", qmp_device_add,
@@ -1004,12 +1006,9 @@ static void qmp_init_marshal(void)
 qmp_register_command("netdev_add", qmp_netdev_add,
  QCO_NO_OPTIONS);
 
-/* call it after the rest of qapi_init() */
-register_module_init(qmp_unregister_commands_hack, MODULE_INIT_QAPI);
+qmp_unregister_commands_hack();
 }
 
-qapi_init(qmp_init_marshal);
-
 /* set the current CPU defined by the user */
 int monitor_set_cpu(int cpu_index)
 {
diff --git a/qga/main.c b/qga/main.c
index 538e4ee..6f8c614 100644
--- a/qga/main.c
+++ b/qga/main.c
@@ -1321,7 +1321,7 @@ int main(int argc, char **argv)
 
 config->log_level = G_LOG_LEVEL_ERROR | G_LOG_LEVEL_CRITICAL;
 
-module_call_init(MODULE_INIT_QAPI);
+qmp_init_marshal();
 
 init_dfl_pathnames();
 config_load(config);
diff --git a/scripts/qapi-commands.py b/scripts/qapi-commands.py
index 09e8467..a75946f 100644
--- a/scripts/qapi-commands.py
+++ b/scripts/qapi-commands.py
@@ -208,14 +208,12 @@ def gen_register_command(name, success_response):
 def gen_registry(registry):
 ret = mcgen('''
 
-static void qmp_init_marshal(void)
+void qmp_init_marshal(void)
 {
 ''')
 ret += registry
 ret += mcgen('''
 }
-
-qapi_init(qmp_init_marshal);
 ''')
 return ret
 
@@ -308,6 +306,7 @@ fdecl.write(mcgen('''
 #include "qapi/qmp/qdict.h"
 #include "qapi/error.h"
 
+void qmp_init_marshal(void);
 ''',
   prefix=prefix))
 
diff --git a/tests/test-qmp-commands.c b/tests/test-qmp-commands.c
index ff94481..c4e20d1 100644
--- 

[Qemu-devel] [PATCH 11/21] qapi: Make QObject input visitor set *list reliably

[Markus Armbruster]

qobject_input_start_struct() sets *list, except when it fails because
qobject_input_get_object() fails, i.e. the input object doesn't exist.

All the other input visitor start_struct(), start_list(),
start_alternate() always set *obj / *list.

Change qobject_input_start_struct() to match.

Signed-off-by: Markus Armbruster 
---
 qapi/qobject-input-visitor.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index 2c2f883..d58696c 100644
--- a/qapi/qobject-input-visitor.c
+++ b/qapi/qobject-input-visitor.c
@@ -196,25 +196,21 @@ static void qobject_input_start_list(Visitor *v, const 
char *name,
 QObject *qobj = qobject_input_get_object(qiv, name, true, errp);
 const QListEntry *entry;
 
+if (list) {
+*list = NULL;
+}
 if (!qobj) {
 return;
 }
 if (qobject_type(qobj) != QTYPE_QLIST) {
-if (list) {
-*list = NULL;
-}
 error_setg(errp, QERR_INVALID_PARAMETER_TYPE, name ? name : "null",
"list");
 return;
 }
 
 entry = qobject_input_push(qiv, qobj, list);
-if (list) {
-if (entry) {
-*list = g_malloc0(size);
-} else {
-*list = NULL;
-}
+if (entry && list) {
+*list = g_malloc0(size);
 }
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 01/21] qga: Fix crash on non-dictionary QMP argument

[Markus Armbruster]

The value of key 'arguments' must be a JSON object.  qemu-ga neglects
to check, and crashes.  To reproduce, send

{ 'execute': 'guest-sync', 'arguments': [] }

to qemu-ga.

do_qmp_dispatch() uses qdict_get_qdict() to get the arguments.  When
not a JSON object, this gets a null pointer, which flows through the
generated marshalling function to qobject_input_visitor_new(), where
it fails the assertion.  qmp_dispatch_check_obj() needs to catch this
error.

QEMU isn't affected, because it runs qmp_check_input_obj() first,
which basically duplicates qmp_check_input_obj()'s checks, plus the
missing one.

Fix by copying the missing one from qmp_check_input_obj() to
qmp_dispatch_check_obj().

Signed-off-by: Markus Armbruster 
Cc: Michael Roth 
---
 qapi/qmp-dispatch.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 48bec20..621922f 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -47,7 +47,13 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, 
Error **errp)
 return NULL;
 }
 has_exec_key = true;
-} else if (strcmp(arg_name, "arguments")) {
+} else if (!strcmp(arg_name, "arguments")) {
+if (qobject_type(arg_obj) != QTYPE_QDICT) {
+error_setg(errp, QERR_QMP_BAD_INPUT_OBJECT_MEMBER,
+   "arguments", "object");
+return NULL;
+}
+} else {
 error_setg(errp, QERR_QMP_EXTRA_MEMBER, arg_name);
 return NULL;
 }
-- 
2.7.4

[Qemu-devel] [PATCH 16/21] test-qobject-input-visitor: Use strict visitor

[Markus Armbruster]

The qobject input visitor comes in a strict and a non-strict variant.
This test is the non-strict variant's last user.  Turns out it relies
on non-strict only in test_visitor_in_null(), and just out of
laziness.  We don't actually test the non-strict behavior.

Clean up test_visitor_in_null(), and switch to the strict variant.
The next commit will drop the non-strict variant.

Signed-off-by: Markus Armbruster 
---
 tests/test-qobject-input-visitor.c | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/tests/test-qobject-input-visitor.c 
b/tests/test-qobject-input-visitor.c
index 945404a..125e34c 100644
--- a/tests/test-qobject-input-visitor.c
+++ b/tests/test-qobject-input-visitor.c
@@ -49,7 +49,7 @@ static Visitor 
*visitor_input_test_init_internal(TestInputVisitorData *data,
 data->obj = qobject_from_jsonv(json_string, ap);
 g_assert(data->obj);
 
-data->qiv = qobject_input_visitor_new(data->obj, false);
+data->qiv = qobject_input_visitor_new(data->obj, true);
 g_assert(data->qiv);
 return data->qiv;
 }
@@ -290,14 +290,14 @@ static void test_visitor_in_null(TestInputVisitorData 
*data,
  * when input is not null.
  */
 
-v = visitor_input_test_init(data, "{ 'a': null, 'b': '' }");
+v = visitor_input_test_init(data, "{ 'a': null, 'b': '', 'c': null }");
 visit_start_struct(v, NULL, NULL, 0, _abort);
 visit_type_null(v, "a", _abort);
-visit_type_str(v, "a", , );
-g_assert(!tmp);
-error_free_or_abort();
 visit_type_null(v, "b", );
 error_free_or_abort();
+visit_type_str(v, "c", , );
+g_assert(!tmp);
+error_free_or_abort();
 visit_check_struct(v, _abort);
 visit_end_struct(v, NULL);
 }
-- 
2.7.4