Re: [PATCH] .travis.yml: Add description to each job

[Thomas Huth]

On 25/01/2020 19.31, Philippe Mathieu-Daudé wrote:
> The NAME variable can be used to describe nicely a job (see [*]).
> As we currently have 32 jobs, use it. This helps for quickly
> finding a particular job.
> 
>   before: https://travis-ci.org/qemu/qemu/builds/639887646
>   after: https://travis-ci.org/philmd/qemu/builds/641795043

Very good idea, correlating a job in the GUI to an entry in the yml file
was really a pain, so far.

> [*] 
> https://docs.travis-ci.com/user/customizing-the-build/#naming-jobs-within-matrices
> 
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  .travis.yml | 101 ++--
>  1 file changed, 67 insertions(+), 34 deletions(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 6c1038a0f1..d68e35a2c5 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -94,24 +94,28 @@ after_script:
>  
>  matrix:
>include:
> -- env:
> +- name: "[x86] GCC static (user)"

Could you please drop the [x86] and other architectures from the names?
Travis already lists the build architecture in the job status page, so
this information is redundant.

[...]
>  # Alternate coroutines implementations are only really of interest to 
> KVM users
>  # However we can't test against KVM on Travis so we can only run unit 
> tests
> -- env:
> +- name: "[x86] check-unit coroutine=ucontext"
> +  env:
>  - CONFIG="--with-coroutine=ucontext --disable-tcg"
>  - TEST_CMD="make check-unit -j3 V=1"
>  
>  
> -- env:
> +- name: "[x86] check-unit coroutine=sigaltstack"
> +  env:
>  - CONFIG="--with-coroutine=sigaltstack --disable-tcg"
>  - TEST_CMD="make check-unit -j3 V=1"
>

Off-topic to your patch, but aren't coroutines something that is only
used in the softmmu targets? If so, we could add --disable-user to the
above two builds to speed things up a little bit.

>  
>  # Check we can build docs and tools (out of tree)
> -- env:
> +- name: "[x86] tools and docs"
> +  env:
>  - BUILD_DIR="out-of-tree/build/dir" SRC_DIR="../../.."

Also off-topic, but I think we can now remove the above line and fix the
comment - since all builds are now out-of-tree anyway, see commit
bc4486fb233573e.


> @@ -250,7 +271,8 @@ matrix:
>  
>  
>  # Python builds
> -- env:
> +- name: "[x86] GCC Python 3.5 (x86_64-softmmu)"
> +  env:

Off-topic again:
Python 3.5 is the default on xenial, and since we stopped using Python
2.7, I think we could remove this job now.

We could add some jobs with Bionic + Python 3.7 and 3.8 instead.

>  - CONFIG="--target-list=x86_64-softmmu"
>  - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
>language: python
> @@ -258,7 +280,8 @@ matrix:
>  - "3.5"
>  
>  
> -- env:
> +- name: "[x86] GCC Python 3.6 (x86_64-softmmu)"
> +  env:
>  - CONFIG="--target-list=x86_64-softmmu"
>  - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
>language: python

 Thomas

Performance hit in qemu-system-ppc

[Howard Spoelstra]

 Hi,

I noticed a considerable (~20%) slowdown in the cpu performance of
qemu-system-ppc.
Bisecting led me to this commit:

d03f140804b345a85973976506492027f703d82d is the first bad commit
commit d03f140804b345a85973976506492027f703d82d
Author: Richard Henderson 
Date:   Mon Dec 9 13:49:58 2019 -0800

cputlb: Move body of cpu_ldst_template.h out of line

With the tracing hooks, the inline functions are no longer
so simple.  Once out-of-line, the current tlb_entry lookup
is redundant with the one in the main load/store_helper.

This also begins the introduction of a new target facing
interface, with suffix *_mmuidx_ra.  This is not yet
official because the interface is not done for user-only.

Use abi_ptr instead of target_ulong in preparation for
user-only; the two types are identical for softmmu.

What remains in cpu_ldst_template.h are the expansions
for _code, _data, and MMU_MODE_SUFFIX.

Tested-by: Philippe Mathieu-Daudé 
Signed-off-by: Richard Henderson 

 accel/tcg/cputlb.c   | 116 
 include/exec/cpu_ldst.h  |  25 +++-
 include/exec/cpu_ldst_template.h | 125
---
 3 files changed, 166 insertions(+), 100 deletions(-)

Thanks for looking into this issue,
Howard

Re: Making QEMU easier for management tools and applications

[Christophe de Dinechin]

> On 23 Jan 2020, at 18:58, John Snow  wrote:
> 
> 
> 
> On 1/23/20 2:19 AM, Markus Armbruster wrote:
>> John Snow  writes:
>> 
>>> On 12/24/19 8:41 AM, Daniel P. Berrangé wrote:
> * scripts/qmp/qmp-shell
> 
>  Half-hearted attempt at a human-friendly wrapper around the JSON
>  syntax.  I have no use for this myself.
 I use this fairly often as its a useful debugging / experimentation
 / trouble shooting tool. There's similar ish functionality in
 virsh qemu-monitor-command. I think there's scope of a supported
 tool here that can talk to libvirt or a UNIX socket for doing
 QMP commands, with a friendlier syntax & pretty printing. 
 
>>> 
>>> qmp-shell is one of my go-to tools for working through bitmap workflows
>>> where we don't have convenience commands yet, as some of the setups
>>> required for fleecing et al involve quite a number of steps.
>>> 
>>> I can copy-paste raw JSON into a socket, but personally I like seeing my
>>> commands neatly organized in a format where I can visually reduce them
>>> to their components at a glance.
>>> 
>>> (What I mean is: It's hard to remember which QMP commands you've barfed
>>> into a terminal because JSON is hard to read and looks very visually
>>> repetitive.)
>>> 
>>> I tried to rewrite qmp-shell late last year, actually. I wanted to write
>>> a new REPL that was json-aware in some manner such that you could write
>>> multi-line commands like this:
>>> 
 example-command arg={
>>>  "hello": "world"
>>> }
>>> 
>>> This requires, sadly, a streamable JSON parser. Most JSON parsers built
>>> into Python as-is simply take a file pointer and consume the entirety of
>>> the rest of the stream -- they don't play very nice with incomplete
>>> input or input that may have trailing data, e.g.:
>>> 
 example-command arg={
>>>  "hello": "world"
>>> } arg2={
>>>  "oops!": "more json!"
>>> }
>> 
>> QMP is in the same boat: it needs to process input that isn't
>> necessarily full expressions (JSON-text in the RFC's grammar).
>> 
>> Any conventional parser can be made streaming by turning it into a
>> coroutine.  This is probably the simplest solution for handwritten
>> streaming LL parsers, because it permits recursive descent.  In Python,
>> I'd try a generator.
>> 
>> Our actual solution for QMP predates coroutine support in QEMU, and is
>> rather hamfisted:
>> 
>> * Streaming lexer: it gets fed characters one at a time, and when its
>>  state machine says "token complete", it feeds the token to the
>>  "streamer".
>> 
>> * "Streamer": gets fed tokens one at a time, buffers them up counting
>>  curly and square bracket nesting until the nesting is zero, then
>>  passes the buffered tokens to the parser.
>> 
>> * Non-streaming parser: it gets fed a sequence of tokens that constitute
>>  a full expression.
>> 
>> The best I can say about this is that it works.  The streamer's token
>> buffer eats a lot of memory compared to a real streaming parser, but in
>> practice, it's a drop in the bucket.
>> 
> 
> I looked into this at one point. I forget why I didn't like it. I had
> some notion that I should replace this one too, but forget exactly why.
> Maybe it wasn't that bad, if I've forgotten.
> 
>>> Also, due to the nature of JSON as being a single discrete object and
>>> never a stream of objects, no existing JSON parser really supports the
>>> idea of ever seeing more than one object per buffer.
>> 
>> That plainly sucks.
>> 
>>> ...So I investigated writing a proper grammar for qmp-shell.
>> 
>> Any parser must start with a proper grammar.  If it doesn't, it's a toy,
>> or a highway to madness.
>> 
>>> Unfortunately, this basically means including the JSON grammar as a
>>> subset of the shell grammar and writing your own parser for it entirely.
>> 
>> Because qmp-shell is a half-hearted wrapper: we ran out of wrapping
>> paper, so JSON sticks out left and right.
>> 
>> Scrap and start over.
>> 
>>> I looked into using Python's own lexer; but it's designed to lex
>>> *python*, not *json*. I got a prototype lexer working for this purpose
>>> under a grammar that I think reflects JSON, but I got that sinking
>>> feeling that it was all more trouble than it was worth, and scrapped
>>> working on it any further.
>> 
>> Parsing JSON is pretty simple.  Data point: QAPISchemaParser parses our
>> weird derivative of JSON in 239 SLOC.
>> 
>>> I did not find any other flex/yacc-like tools that seemed properly
>>> idiomatic or otherwise heavily specialized. I gave up on the idea of
>>> writing a new parser.
>> 
>> While I recommend use of tools for parsing non-trivial grammars (you'll
>> screw up, they won't), they're massive overkill for JSON.
>> 
>>> I'd love to offer a nice robust QMP shell that is available for use by
>>> end users, but the syntax of the shell will need some major considerations.
>> 
>> Scrap and start over.
>> 
>> [...]
>> 
> 
> Yes, I agree: Scrap and start over.
> 
> What SHOULD the syntax look like, though? 

Re: Integrating QOM into QAPI

[Peter Maydell]

On Sat, 25 Jan 2020 at 09:28, Paolo Bonzini  wrote:
>
> On 25/01/20 05:44, Marc-André Lureau wrote:
> > I try to find a good reason qom was chosen over gobject, and I can't
> > find it.
>
> The main reasons were integration with QAPI, and the object tree.
> Though everything I say here is a kind of reverse engineering of
> Anthony's brain because there aren't really any design documents besides
> what's in include/qom/object.h (and he overlooked some aspects, for
> example "unparent" was introduced a few months later).

I vagely recall that back at that time we were a lot less heavy
in our usage of glib also, so "just use the glib version of whatever"
would not have been quite as easy a sell as it might be today.
Anthony's original RFC email lists some "key differences" between
QOM and GObject, which presumably seemed to him at the time
to be sufficient to justify not using GObject:
https://lists.gnu.org/archive/html/qemu-devel/2011-07/msg01673.html

thanks
-- PMM

Re: [PATCH] bsd-user: improve support for sparc syscall flags

[Salvador Fandiño]

On 25/1/20 16:52, Laurent Vivier wrote:

Le 25/01/2020 à 12:55, no-re...@patchew.org a écrit :

Patchew URL: 
https://patchew.org/QEMU/20200125114753.61820-1-salva...@qindel.com/



Hi,

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


Salvador,

you can use scripts/checkpatch.pl in the QEMU directory to check your
patch for style before sending them.

And don't send them as a reply but as a new thread, using versioning in
the subject ("[PATCH v4]").


ok, I will do that in the future.

In any case, the style problems found by patchew for the last version of 
the patch I have submitted are for a fragment of a file copied verbatim 
from the NetBSD source code (in the same way the equivalent fragment was 
copied from OpenBSD when support for that OS was added). It is mostly 
the copyright header and a few defines. IMO, style shouldn't be 
corrected there.

[PATCH] .travis.yml: Drop superfluous use of --python=python3 parameter

[Philippe Mathieu-Daudé]

As we require Python3 since commit ddf9069963, we don't need to
explicit it with the --python=/usr/bin/python3 configure option.

Reported-by: Thomas Huth 
Signed-off-by: Philippe Mathieu-Daudé 
---
Cc: Wainer dos Santos Moschetta 
Cc: Eduardo Habkost 
Cc: Cleber Rosa 
---
 .travis.yml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a0f1..ee93180283 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -268,7 +268,7 @@ matrix:
 
 # Acceptance (Functional) tests
 - env:
-- CONFIG="--python=/usr/bin/python3 
--target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
+- 
CONFIG="--target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
 - TEST_CMD="make check-acceptance"
   after_script:
 - python3 -c 'import json; r = 
json.load(open("tests/results/latest/results.json")); [print(t["logfile"]) for 
t in r["tests"] if t["status"] not in ("PASS", "SKIP")]' | xargs cat
-- 
2.21.1

Re: [PATCH] tests/acceptance: Add boot tests for some of the QEMU advent calendar images

[Philippe Mathieu-Daudé]

On 1/25/20 5:43 PM, Thomas Huth wrote:
> On 24/01/2020 22.28, Wainer dos Santos Moschetta wrote:
>>
>> On 1/24/20 3:03 PM, Thomas Huth wrote:
>>> The 2018 edition of the QEMU advent calendar 2018 featured Linux images
>>> for various non-x86 machines. We can use them for a boot tests in our
>>> acceptance test suite.

Excellent idea!

>>>
>>> Let's also make sure that we build the corresponding machines in Travis,
>>> and while we're there, drop the superfluous --python parameter (python3
>>> is now the only supported version anyway).

I'd rather see this change as another commit.

>>
>> Yes, please, removal of --python was in my wish list.
>>
>>>
>>> Signed-off-by: Thomas Huth 
>>> ---
>>>   .travis.yml    |  2 +-
>>>   tests/acceptance/boot_linux_console.py | 96 ++
>>>   2 files changed, 97 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/.travis.yml b/.travis.yml
>>> index 6c1038a0f1..73ca12c921 100644
>>> --- a/.travis.yml
>>> +++ b/.travis.yml
>>> @@ -268,7 +268,7 @@ matrix:
>>>     # Acceptance (Functional) tests
>>>   - env:
>>> -    - CONFIG="--python=/usr/bin/python3
>>> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
>>>
>>> +    -
>>> CONFIG="--target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
>>>
>>
>>
>> Perhaps use MAIN_SOFTMMU_TARGETS in only append the other targets, like:
>>
>> --target-list=${MAIN_SOFTMMU_TARGETS},alpha-softmmu,sparc-softmmu,
> 
> Not sure ... while it is a nice way to shorten the line here, it adds a
> dependecy to that variable ... and MAIN_SOFTMMU_TARGETS has been changed
> a couple of times during the course of time, so we might risk to lose
> some testing coverage here in case someone removes a target from
> MAIN_SOFTMMU_TARGETS but forgets to add it here again...? I think we
> should better use the explicit list here instead.

Maybe related: "Split enterprise vs. hobbyist acceptance test job"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg644683.html

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

[PATCH] .travis.yml: Add description to each job

[Philippe Mathieu-Daudé]

The NAME variable can be used to describe nicely a job (see [*]).
As we currently have 32 jobs, use it. This helps for quickly
finding a particular job.

  before: https://travis-ci.org/qemu/qemu/builds/639887646
  after: https://travis-ci.org/philmd/qemu/builds/641795043

[*] 
https://docs.travis-ci.com/user/customizing-the-build/#naming-jobs-within-matrices

Signed-off-by: Philippe Mathieu-Daudé 
---
 .travis.yml | 101 ++--
 1 file changed, 67 insertions(+), 34 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 6c1038a0f1..d68e35a2c5 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -94,24 +94,28 @@ after_script:
 
 matrix:
   include:
-- env:
+- name: "[x86] GCC static (user)"
+  env:
 - CONFIG="--disable-system --static"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
 
 
 # we split the system builds as it takes a while to build them all
-- env:
+- name: "[x86] GCC (main-softmmu)"
+  env:
 - CONFIG="--disable-user --target-list=${MAIN_SOFTMMU_TARGETS}"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
 
 
-- env:
-- CONFIG="--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}"
+- name: "[x86] GCC (other-softmmu)"
+  env:
+   - CONFIG="--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
 
 
 # Just build tools and run minimal unit and softfloat checks
-- env:
+- name: "[x86] GCC check-softfloat (user)"
+  env:
 - BASE_CONFIG="--enable-tools"
 - CONFIG="--disable-user --disable-system"
 - TEST_CMD="make check-unit check-softfloat -j3"
@@ -119,41 +123,48 @@ matrix:
 
 
 # --enable-debug implies --enable-debug-tcg, also runs quite a bit slower
-- env:
+- name: "[x86] GCC debug (main-softmmu)"
+  env:
 - CONFIG="--enable-debug --target-list=${MAIN_SOFTMMU_TARGETS}"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug"
 
 
 # TCG debug can be run just on its own and is mostly agnostic to 
user/softmmu distinctions
-- env:
+- name: "[x86] GCC debug (user)"
+  env:
 - CONFIG="--enable-debug-tcg --disable-system"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-debug-tcg"
 
 
-- env:
+- name: "[x86] GCC some libs disabled (main-softmmu)"
+  env:
 - CONFIG="--disable-linux-aio --disable-cap-ng --disable-attr 
--disable-brlapi --disable-libusb --disable-replication 
--target-list=${MAIN_SOFTMMU_TARGETS}"
 
 
 # Module builds are mostly of interest to major distros
-- env:
+- name: "[x86] GCC modules (main-softmmu)"
+  env:
 - CONFIG="--enable-modules --target-list=${MAIN_SOFTMMU_TARGETS}"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-gcc-default"
 
 
 # Alternate coroutines implementations are only really of interest to KVM 
users
 # However we can't test against KVM on Travis so we can only run unit tests
-- env:
+- name: "[x86] check-unit coroutine=ucontext"
+  env:
 - CONFIG="--with-coroutine=ucontext --disable-tcg"
 - TEST_CMD="make check-unit -j3 V=1"
 
 
-- env:
+- name: "[x86] check-unit coroutine=sigaltstack"
+  env:
 - CONFIG="--with-coroutine=sigaltstack --disable-tcg"
 - TEST_CMD="make check-unit -j3 V=1"
 
 
 # Check we can build docs and tools (out of tree)
-- env:
+- name: "[x86] tools and docs"
+  env:
 - BUILD_DIR="out-of-tree/build/dir" SRC_DIR="../../.."
 - BASE_CONFIG="--enable-tools --enable-docs"
 - CONFIG="--target-list=x86_64-softmmu,aarch64-linux-user"
@@ -167,13 +178,15 @@ matrix:
 
 
 # Test with Clang for compile portability (Travis uses clang-5.0)
-- env:
+- name: "[x86] Clang (user)"
+  env:
 - CONFIG="--disable-system"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-clang-default"
   compiler: clang
 
 
-- env:
+- name: "[x86] Clang (main-softmmu)"
+  env:
 - CONFIG="--target-list=${MAIN_SOFTMMU_TARGETS} "
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-clang-sanitize"
   compiler: clang
@@ -182,52 +195,60 @@ matrix:
 - ${SRC_DIR}/configure ${CONFIG} --extra-cflags="-fsanitize=undefined 
-Werror" || { cat config.log && exit 1; }
 
 
-- env:
+- name: "[x86] Clang (other-softmmu)"
+  env:
 - CONFIG="--disable-user --target-list-exclude=${MAIN_SOFTMMU_TARGETS}"
 - CACHE_NAME="${TRAVIS_BRANCH}-linux-clang-default"
   compiler: clang
 
 
 # gprof/gcov are GCC features
-- env:
+- name: "[x86] GCC gprof/gcov"
+  env:
 - CONFIG="--enable-gprof --enable-gcov --disable-pie 
--target-list=${MAIN_SOFTMMU_TARGETS}"
   after_success:
 - ${SRC_DIR}/scripts/travis/coverage-summary.sh
 
 
 # We manually include builds which we disable "make check" for
-- env:
+- name: "[x86] GCC without-default-devices (softmmu)"
+   

Re: [PATCH v4 6/7] disas: mips: Add micromips R6 disassembler - infrastructure and 16-bit instructions

[Richard Henderson]

On 1/25/20 12:22 AM, Aleksandar Markovic wrote:
> 
> 
> On Saturday, January 25, 2020, Richard Henderson  > wrote:
> 
> On 1/24/20 1:38 PM, Aleksandar Markovic wrote:
> > On Friday, January 24, 2020, Richard Henderson
> mailto:richard.hender...@linaro.org>
> >  >> wrote:
> >     The thing I'm concerned about here is any future maintenance of this
> file.  One
> >     would surely prefer to edit the original decodetree input than this
> output.
> >
> >
> > Here is the deal: This dissasembler is meant to reflect the
>  documentation of a
> > particular ISA, and as the documentation largely stays constant (except
> adding
> > some insignificant errata), the disassembler will stay virtually 
> constant, we
> > wouldn't like even to touch it, and we like it this way.
> 
> No, this is neither right nor proper.
> 
> To review the code in this form is significantly harder than in its 
> decodetree
> form.  That is in fact the whole point of the decodetree form: otherwise 
> we'd
> still be writing these sorts of parsers by hand.
> 
> While there's no license on this new file (another problem), if as 
> assumed this
> is GPL 2+, then you are in violation of the GPL.  From section 3:
> 
>   # The source code for a work means the preferred form of
>   # the work for making modifications to it.
> 
> You cannot with a straight face claim that the generated c is the 
> preferred
> form for making modifications.
> 
> Finally, suppose we improve decodetree.py, so that it produces code with 
> which
> the compiler produces better code, for some metric of better.  We would 
> want
> this disassembler to benefit as well.
> 
> 
> I think you got some things upside-down. A tool developent and usage should be
> driven by the needs of users of a tool, and not dictated by the author. Users
> should be free to use the tool in any way they seem suitable, including its
> modification.

That's exactly right, provided that by "tool" you mean QEMU and by "users" you
mean everyone downstream of us.

Let me state for the record that I object to this generated file being merged
in this form, for the reasons I stated above.


r~

[PATCH v13 10/10] tests: Add virtio-iommu test

[Eric Auger]

This adds the framework to test the virtio-iommu-pci device
and tests exercising the attach/detach, map/unmap API.

To run the tests:
make tests/qtest/qos-test
QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 tests/qtest/qos-test V=1

Signed-off-by: Eric Auger 

---

v12 -> v13
- remove probe_size test
- move to qtest directory
---
 tests/qtest/Makefile.include  |   2 +
 tests/qtest/libqos/virtio-iommu.c | 177 +
 tests/qtest/libqos/virtio-iommu.h |  45 +
 tests/qtest/virtio-iommu-test.c   | 306 ++
 4 files changed, 530 insertions(+)
 create mode 100644 tests/qtest/libqos/virtio-iommu.c
 create mode 100644 tests/qtest/libqos/virtio-iommu.h
 create mode 100644 tests/qtest/virtio-iommu-test.c

diff --git a/tests/qtest/Makefile.include b/tests/qtest/Makefile.include
index e6bb4ab28c..fd6e8f43f7 100644
--- a/tests/qtest/Makefile.include
+++ b/tests/qtest/Makefile.include
@@ -189,6 +189,7 @@ qos-test-obj-y += tests/qtest/libqos/virtio-pci-modern.o
 qos-test-obj-y += tests/qtest/libqos/virtio-rng.o
 qos-test-obj-y += tests/qtest/libqos/virtio-scsi.o
 qos-test-obj-y += tests/qtest/libqos/virtio-serial.o
+qos-test-obj-y += tests/qtest/libqos/virtio-iommu.o
 
 # qos machines:
 qos-test-obj-y += tests/qtest/libqos/aarch64-xlnx-zcu102-machine.o
@@ -228,6 +229,7 @@ qos-test-obj-y += tests/qtest/virtio-net-test.o
 qos-test-obj-y += tests/qtest/virtio-rng-test.o
 qos-test-obj-y += tests/qtest/virtio-scsi-test.o
 qos-test-obj-y += tests/qtest/virtio-serial-test.o
+qos-test-obj-y += tests/qtest/virtio-iommu-test.o
 qos-test-obj-y += tests/qtest/vmxnet3-test.o
 
 check-unit-y += tests/test-qgraph$(EXESUF)
diff --git a/tests/qtest/libqos/virtio-iommu.c 
b/tests/qtest/libqos/virtio-iommu.c
new file mode 100644
index 00..b4e9ea44fb
--- /dev/null
+++ b/tests/qtest/libqos/virtio-iommu.c
@@ -0,0 +1,177 @@
+/*
+ * libqos driver framework
+ *
+ * Copyright (c) 2018 Emanuele Giuseppe Esposito 
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License version 2 as published by the Free Software Foundation.
+ *
+ * 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 "libqtest.h"
+#include "qemu/module.h"
+#include "libqos/qgraph.h"
+#include "libqos/virtio-iommu.h"
+#include "hw/virtio/virtio-iommu.h"
+
+static QGuestAllocator *alloc;
+
+/* virtio-iommu-device */
+static void *qvirtio_iommu_get_driver(QVirtioIOMMU *v_iommu,
+  const char *interface)
+{
+if (!g_strcmp0(interface, "virtio-iommu")) {
+return v_iommu;
+}
+if (!g_strcmp0(interface, "virtio")) {
+return v_iommu->vdev;
+}
+
+fprintf(stderr, "%s not present in virtio-iommu-device\n", interface);
+g_assert_not_reached();
+}
+
+static void *qvirtio_iommu_device_get_driver(void *object,
+ const char *interface)
+{
+QVirtioIOMMUDevice *v_iommu = object;
+return qvirtio_iommu_get_driver(_iommu->iommu, interface);
+}
+
+static void virtio_iommu_cleanup(QVirtioIOMMU *interface)
+{
+qvirtqueue_cleanup(interface->vdev->bus, interface->vq, alloc);
+}
+
+static void virtio_iommu_setup(QVirtioIOMMU *interface)
+{
+QVirtioDevice *vdev = interface->vdev;
+uint64_t features;
+
+features = qvirtio_get_features(vdev);
+features &= ~(QVIRTIO_F_BAD_FEATURE |
+  (1ull << VIRTIO_RING_F_INDIRECT_DESC) |
+  (1ull << VIRTIO_RING_F_EVENT_IDX) |
+  (1ull << VIRTIO_IOMMU_F_BYPASS));
+qvirtio_set_features(vdev, features);
+interface->vq = qvirtqueue_setup(interface->vdev, alloc, 0);
+qvirtio_set_driver_ok(interface->vdev);
+}
+
+static void qvirtio_iommu_device_destructor(QOSGraphObject *obj)
+{
+QVirtioIOMMUDevice *v_iommu = (QVirtioIOMMUDevice *) obj;
+QVirtioIOMMU *iommu = _iommu->iommu;
+
+virtio_iommu_cleanup(iommu);
+}
+
+static void qvirtio_iommu_device_start_hw(QOSGraphObject *obj)
+{
+QVirtioIOMMUDevice *v_iommu = (QVirtioIOMMUDevice *) obj;
+QVirtioIOMMU *iommu = _iommu->iommu;
+
+virtio_iommu_setup(iommu);
+}
+
+static void *virtio_iommu_device_create(void *virtio_dev,
+QGuestAllocator *t_alloc,
+void *addr)
+{
+QVirtioIOMMUDevice *virtio_rdevice = g_new0(QVirtioIOMMUDevice, 1);
+QVirtioIOMMU *interface = _rdevice->iommu;
+
+interface->vdev = virtio_dev;
+alloc = t_alloc;
+
+virtio_rdevice->obj.get_driver = 

[PATCH v13 07/10] virtio-iommu-pci: Add virtio iommu pci support

[Eric Auger]

This patch adds virtio-iommu-pci, which is the pci proxy for
the virtio-iommu device.

Signed-off-by: Eric Auger 
Reviewed-by: Jean-Philippe Brucker 

---

v11 -> v12:
- added Jean's R-b
- remove the array of intervals. Will be introduced later?

v10 -> v11:
- add the reserved_regions array property

v9 -> v10:
- include "hw/qdev-properties.h" header

v8 -> v9:
- add the msi-bypass property
- create virtio-iommu-pci.c
---
 hw/virtio/Makefile.objs  |  1 +
 hw/virtio/virtio-iommu-pci.c | 88 
 include/hw/pci/pci.h |  1 +
 include/hw/virtio/virtio-iommu.h |  1 +
 qdev-monitor.c   |  1 +
 5 files changed, 92 insertions(+)
 create mode 100644 hw/virtio/virtio-iommu-pci.c

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 2fd9da7410..4e4d39a0a4 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -29,6 +29,7 @@ obj-$(CONFIG_VIRTIO_INPUT_HOST) += virtio-input-host-pci.o
 obj-$(CONFIG_VIRTIO_INPUT) += virtio-input-pci.o
 obj-$(CONFIG_VIRTIO_RNG) += virtio-rng-pci.o
 obj-$(CONFIG_VIRTIO_BALLOON) += virtio-balloon-pci.o
+obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu-pci.o
 obj-$(CONFIG_VIRTIO_9P) += virtio-9p-pci.o
 obj-$(CONFIG_VIRTIO_SCSI) += virtio-scsi-pci.o
 obj-$(CONFIG_VIRTIO_BLK) += virtio-blk-pci.o
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
new file mode 100644
index 00..4cfae1f9df
--- /dev/null
+++ b/hw/virtio/virtio-iommu-pci.c
@@ -0,0 +1,88 @@
+/*
+ * Virtio IOMMU PCI Bindings
+ *
+ * Copyright (c) 2019 Red Hat, Inc.
+ * Written by Eric Auger
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License version 2 or
+ *  (at your option) any later version.
+ */
+
+#include "qemu/osdep.h"
+
+#include "virtio-pci.h"
+#include "hw/virtio/virtio-iommu.h"
+#include "hw/qdev-properties.h"
+
+typedef struct VirtIOIOMMUPCI VirtIOIOMMUPCI;
+
+/*
+ * virtio-iommu-pci: This extends VirtioPCIProxy.
+ *
+ */
+#define VIRTIO_IOMMU_PCI(obj) \
+OBJECT_CHECK(VirtIOIOMMUPCI, (obj), TYPE_VIRTIO_IOMMU_PCI)
+
+struct VirtIOIOMMUPCI {
+VirtIOPCIProxy parent_obj;
+VirtIOIOMMU vdev;
+};
+
+static Property virtio_iommu_pci_properties[] = {
+DEFINE_PROP_UINT32("class", VirtIOPCIProxy, class_code, 0),
+DEFINE_PROP_END_OF_LIST(),
+};
+
+static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
+{
+VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(vpci_dev);
+DeviceState *vdev = DEVICE(>vdev);
+
+qdev_set_parent_bus(vdev, BUS(_dev->bus));
+object_property_set_link(OBJECT(dev),
+ OBJECT(pci_get_bus(_dev->pci_dev)),
+ "primary-bus", errp);
+object_property_set_bool(OBJECT(vdev), true, "realized", errp);
+}
+
+static void virtio_iommu_pci_class_init(ObjectClass *klass, void *data)
+{
+DeviceClass *dc = DEVICE_CLASS(klass);
+VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
+PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
+k->realize = virtio_iommu_pci_realize;
+set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+dc->props = virtio_iommu_pci_properties;
+pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
+pcidev_k->device_id = PCI_DEVICE_ID_VIRTIO_IOMMU;
+pcidev_k->revision = VIRTIO_PCI_ABI_VERSION;
+pcidev_k->class_id = PCI_CLASS_OTHERS;
+}
+
+static void virtio_iommu_pci_instance_init(Object *obj)
+{
+VirtIOIOMMUPCI *dev = VIRTIO_IOMMU_PCI(obj);
+
+virtio_instance_init_common(obj, >vdev, sizeof(dev->vdev),
+TYPE_VIRTIO_IOMMU);
+}
+
+static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
+.base_name = TYPE_VIRTIO_IOMMU_PCI,
+.generic_name  = "virtio-iommu-pci",
+.transitional_name = "virtio-iommu-pci-transitional",
+.non_transitional_name = "virtio-iommu-pci-non-transitional",
+.instance_size = sizeof(VirtIOIOMMUPCI),
+.instance_init = virtio_iommu_pci_instance_init,
+.class_init= virtio_iommu_pci_class_init,
+};
+
+static void virtio_iommu_pci_register(void)
+{
+virtio_pci_types_register(_iommu_pci_info);
+}
+
+type_init(virtio_iommu_pci_register)
+
+
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 2acd8321af..cfedf5a995 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -86,6 +86,7 @@ extern bool pci_available;
 #define PCI_DEVICE_ID_VIRTIO_9P  0x1009
 #define PCI_DEVICE_ID_VIRTIO_VSOCK   0x1012
 #define PCI_DEVICE_ID_VIRTIO_PMEM0x1013
+#define PCI_DEVICE_ID_VIRTIO_IOMMU   0x1014
 
 #define PCI_VENDOR_ID_REDHAT 0x1b36
 #define PCI_DEVICE_ID_REDHAT_BRIDGE  0x0001
diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 2a2c2ecf83..f39aa0fbb4 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -25,6 +25,7 @@
 #include "hw/pci/pci.h"
 
 #define 

[PATCH v13 09/10] virtio-iommu: Support migration

[Eric Auger]

Add Migration support. We rely on recently added gtree and qlist
migration. We only migrate the domain gtree. The endpoint gtree
is re-constructed in a post-load operation.

Signed-off-by: Eric Auger 
Acked-by: Peter Xu 

---

v11 -> v12:
- do not migrate the endpoint gtree but reconstruct it from the
  domain gtree (Peter's suggestion)
- add MIG_PRI_IOMMU
---
 hw/virtio/virtio-iommu.c | 109 +++
 1 file changed, 99 insertions(+), 10 deletions(-)

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index ec8f42e167..8f5c6f17df 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -650,16 +650,6 @@ static uint64_t virtio_iommu_get_features(VirtIODevice 
*vdev, uint64_t f,
 return f;
 }
 
-/*
- * Migration is not yet supported: most of the state consists
- * of balanced binary trees which are not yet ready for getting
- * migrated
- */
-static const VMStateDescription vmstate_virtio_iommu_device = {
-.name = "virtio-iommu-device",
-.unmigratable = 1,
-};
-
 static gint int_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
 {
 guint ua = GPOINTER_TO_UINT(a);
@@ -743,9 +733,108 @@ static void virtio_iommu_instance_init(Object *obj)
 {
 }
 
+#define VMSTATE_INTERVAL   \
+{  \
+.name = "interval",\
+.version_id = 1,   \
+.minimum_version_id = 1,   \
+.fields = (VMStateField[]) {   \
+VMSTATE_UINT64(low, VirtIOIOMMUInterval),  \
+VMSTATE_UINT64(high, VirtIOIOMMUInterval), \
+VMSTATE_END_OF_LIST()  \
+}  \
+}
+
+#define VMSTATE_MAPPING   \
+{ \
+.name = "mapping",\
+.version_id = 1,  \
+.minimum_version_id = 1,  \
+.fields = (VMStateField[]) {  \
+VMSTATE_UINT64(phys_addr, VirtIOIOMMUMapping),\
+VMSTATE_UINT32(flags, VirtIOIOMMUMapping),\
+VMSTATE_END_OF_LIST() \
+},\
+}
+
+static const VMStateDescription vmstate_interval_mapping[2] = {
+VMSTATE_MAPPING,   /* value */
+VMSTATE_INTERVAL   /* key   */
+};
+
+static int domain_preload(void *opaque)
+{
+VirtIOIOMMUDomain *domain = opaque;
+
+domain->mappings = g_tree_new_full((GCompareDataFunc)interval_cmp,
+   NULL, g_free, g_free);
+return 0;
+}
+
+static const VMStateDescription vmstate_endpoint = {
+.name = "endpoint",
+.version_id = 1,
+.minimum_version_id = 1,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(id, VirtIOIOMMUEndpoint),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static const VMStateDescription vmstate_domain = {
+.name = "domain",
+.version_id = 1,
+.minimum_version_id = 1,
+.pre_load = domain_preload,
+.fields = (VMStateField[]) {
+VMSTATE_UINT32(id, VirtIOIOMMUDomain),
+VMSTATE_GTREE_V(mappings, VirtIOIOMMUDomain, 1,
+vmstate_interval_mapping,
+VirtIOIOMMUInterval, VirtIOIOMMUMapping),
+VMSTATE_QLIST_V(endpoint_list, VirtIOIOMMUDomain, 1,
+vmstate_endpoint, VirtIOIOMMUEndpoint, next),
+VMSTATE_END_OF_LIST()
+}
+};
+
+static gboolean reconstruct_endpoints(gpointer key, gpointer value,
+  gpointer data)
+{
+VirtIOIOMMU *s = (VirtIOIOMMU *)data;
+VirtIOIOMMUDomain *d = (VirtIOIOMMUDomain *)value;
+VirtIOIOMMUEndpoint *iter;
+
+QLIST_FOREACH(iter, >endpoint_list, next) {
+iter->domain = d;
+g_tree_insert(s->endpoints, GUINT_TO_POINTER(iter->id), iter);
+}
+return false; /* continue the domain traversal */
+}
+
+static int iommu_post_load(void *opaque, int version_id)
+{
+VirtIOIOMMU *s = opaque;
+
+g_tree_foreach(s->domains, reconstruct_endpoints, s);
+return 0;
+}
+
+static const VMStateDescription vmstate_virtio_iommu_device = {
+.name = "virtio-iommu-device",
+.minimum_version_id = 1,
+.version_id = 1,
+.post_load = iommu_post_load,
+.fields = (VMStateField[]) {
+VMSTATE_GTREE_DIRECT_KEY_V(domains, VirtIOIOMMU, 1,
+   _domain, VirtIOIOMMUDomain),
+VMSTATE_END_OF_LIST()
+},
+};
+
 static const VMStateDescription vmstate_virtio_iommu = {
 .name = "virtio-iommu",
 .minimum_version_id = 1,
+.priority = MIG_PRI_IOMMU,
 .version_id = 1,
 .fields = (VMStateField[]) {
 VMSTATE_VIRTIO_DEVICE,
-- 
2.20.1

[PATCH] migration: Simplify get_qlist

[Eric Auger]

Instead of inserting read elements at the head and
then reversing the list, it is simpler to add
each element after the previous one. Introduce
QLIST_RAW_INSERT_AFTER helper and use it in
get_qlist().

Signed-off-by: Eric Auger 
Suggested-by: Juan Quintela 

---
---
 include/qemu/queue.h  | 19 ++-
 migration/vmstate-types.c | 10 +++---
 2 files changed, 13 insertions(+), 16 deletions(-)

diff --git a/include/qemu/queue.h b/include/qemu/queue.h
index 4d4554a7ce..19425f973f 100644
--- a/include/qemu/queue.h
+++ b/include/qemu/queue.h
@@ -515,6 +515,12 @@ union {
 \
  (elm);
\
  (elm) = *QLIST_RAW_NEXT(elm, entry))
 
+#define QLIST_RAW_INSERT_AFTER(head, prev, elem, entry) do {   
\
+*QLIST_RAW_NEXT(prev, entry) = elem;   
\
+*QLIST_RAW_PREVIOUS(elem, entry) = QLIST_RAW_NEXT(prev, entry);
\
+*QLIST_RAW_NEXT(elem, entry) = NULL;   
\
+} while (0)
+
 #define QLIST_RAW_INSERT_HEAD(head, elm, entry) do {   
\
 void *first = *QLIST_RAW_FIRST(head);  
\
 *QLIST_RAW_FIRST(head) = elm;  
\
@@ -527,17 +533,4 @@ union {
 \
 }  
\
 } while (0)
 
-#define QLIST_RAW_REVERSE(head, elm, entry) do {   
\
-void *iter = *QLIST_RAW_FIRST(head), *prev = NULL, *next;  
\
-while (iter) { 
\
-next = *QLIST_RAW_NEXT(iter, entry);   
\
-*QLIST_RAW_PREVIOUS(iter, entry) = QLIST_RAW_NEXT(next, entry);
\
-*QLIST_RAW_NEXT(iter, entry) = prev;   
\
-prev = iter;   
\
-iter = next;   
\
-}  
\
-*QLIST_RAW_FIRST(head) = prev; 
\
-*QLIST_RAW_PREVIOUS(prev, entry) = QLIST_RAW_FIRST(head);  
\
-} while (0)
-
 #endif /* QEMU_SYS_QUEUE_H */
diff --git a/migration/vmstate-types.c b/migration/vmstate-types.c
index 1eee36773a..35e784c9d9 100644
--- a/migration/vmstate-types.c
+++ b/migration/vmstate-types.c
@@ -879,7 +879,7 @@ static int get_qlist(QEMUFile *f, void *pv, size_t 
unused_size,
 /* offset of the QLIST entry in a QLIST element */
 size_t entry_offset = field->start;
 int version_id = field->version_id;
-void *elm;
+void *elm, *prev = NULL;
 
 trace_get_qlist(field->name, vmsd->name, vmsd->version_id);
 if (version_id > vmsd->version_id) {
@@ -900,9 +900,13 @@ static int get_qlist(QEMUFile *f, void *pv, size_t 
unused_size,
 g_free(elm);
 return ret;
 }
-QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset);
+if (!prev) {
+QLIST_RAW_INSERT_HEAD(pv, elm, entry_offset);
+} else {
+QLIST_RAW_INSERT_AFTER(pv, prev, elm, entry_offset);
+}
+prev = elm;
 }
-QLIST_RAW_REVERSE(pv, elm, entry_offset);
 trace_get_qlist_end(field->name, vmsd->name);
 
 return ret;
-- 
2.20.1

[PATCH v13 04/10] virtio-iommu: Implement map/unmap

[Eric Auger]

This patch implements virtio_iommu_map/unmap.

Signed-off-by: Eric Auger 
Reviewed-by: Peter Xu 

---

v11 -> v12:
- check unmanaged managed flags on map
- removed 2 qemu_log_mask in unmap()
- fix leak

v10 -> v11:
- revisit the implementation of unmap according to Peter's suggestion
- removed virt_addr and size from viommu_mapping struct
- use g_tree_lookup_extended()
- return VIRTIO_IOMMU_S_RANGE in case a mapping were
  to be split on unmap (instead of INVAL)

v5 -> v6:
- use new v0.6 fields
- replace error_report by qemu_log_mask

v3 -> v4:
- implement unmap semantics as specified in v0.4
---
 hw/virtio/trace-events   |  1 +
 hw/virtio/virtio-iommu.c | 63 ++--
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 15595f8cd7..22162d6583 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -64,6 +64,7 @@ virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) 
"domain=%d endpoint=%d"
 virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, 
uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" 
virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
 virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) 
"domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
+virtio_iommu_unmap_done(uint32_t domain_id, uint64_t virt_start, uint64_t 
virt_end) "domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
 virtio_iommu_translate(const char *name, uint32_t rid, uint64_t iova, int 
flag) "mr=%s rid=%d addr=0x%"PRIx64" flag=%d"
 virtio_iommu_init_iommu_mr(char *iommu_mr) "init %s"
 virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc endpoint=%d"
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index e5cc94138b..e1e0c69473 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -18,6 +18,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "qemu/iov.h"
 #include "qemu-common.h"
 #include "hw/qdev-properties.h"
@@ -55,6 +56,11 @@ typedef struct VirtIOIOMMUInterval {
 uint64_t high;
 } VirtIOIOMMUInterval;
 
+typedef struct VirtIOIOMMUMapping {
+uint64_t phys_addr;
+uint32_t flags;
+} VirtIOIOMMUMapping;
+
 static inline uint16_t virtio_iommu_get_bdf(IOMMUDevice *dev)
 {
 return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
@@ -305,10 +311,39 @@ static int virtio_iommu_map(VirtIOIOMMU *s,
 uint64_t virt_start = le64_to_cpu(req->virt_start);
 uint64_t virt_end = le64_to_cpu(req->virt_end);
 uint32_t flags = le32_to_cpu(req->flags);
+VirtIOIOMMUDomain *domain;
+VirtIOIOMMUInterval *interval;
+VirtIOIOMMUMapping *mapping;
+
+if (flags & ~VIRTIO_IOMMU_MAP_F_MASK) {
+return VIRTIO_IOMMU_S_INVAL;
+}
+
+domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
+if (!domain) {
+return VIRTIO_IOMMU_S_NOENT;
+}
+
+interval = g_malloc0(sizeof(*interval));
+
+interval->low = virt_start;
+interval->high = virt_end;
+
+mapping = g_tree_lookup(domain->mappings, (gpointer)interval);
+if (mapping) {
+g_free(interval);
+return VIRTIO_IOMMU_S_INVAL;
+}
 
 trace_virtio_iommu_map(domain_id, virt_start, virt_end, phys_start, flags);
 
-return VIRTIO_IOMMU_S_UNSUPP;
+mapping = g_malloc0(sizeof(*mapping));
+mapping->phys_addr = phys_start;
+mapping->flags = flags;
+
+g_tree_insert(domain->mappings, interval, mapping);
+
+return VIRTIO_IOMMU_S_OK;
 }
 
 static int virtio_iommu_unmap(VirtIOIOMMU *s,
@@ -317,10 +352,34 @@ static int virtio_iommu_unmap(VirtIOIOMMU *s,
 uint32_t domain_id = le32_to_cpu(req->domain);
 uint64_t virt_start = le64_to_cpu(req->virt_start);
 uint64_t virt_end = le64_to_cpu(req->virt_end);
+VirtIOIOMMUMapping *iter_val;
+VirtIOIOMMUInterval interval, *iter_key;
+VirtIOIOMMUDomain *domain;
+int ret = VIRTIO_IOMMU_S_OK;
 
 trace_virtio_iommu_unmap(domain_id, virt_start, virt_end);
 
-return VIRTIO_IOMMU_S_UNSUPP;
+domain = g_tree_lookup(s->domains, GUINT_TO_POINTER(domain_id));
+if (!domain) {
+return VIRTIO_IOMMU_S_NOENT;
+}
+interval.low = virt_start;
+interval.high = virt_end;
+
+while (g_tree_lookup_extended(domain->mappings, ,
+  (void **)_key, (void**)_val)) {
+uint64_t current_low = iter_key->low;
+uint64_t current_high = iter_key->high;
+
+if (interval.low <= current_low && interval.high >= current_high) {
+g_tree_remove(domain->mappings, iter_key);
+trace_virtio_iommu_unmap_done(domain_id, current_low, 
current_high);
+} else {
+ret = VIRTIO_IOMMU_S_RANGE;
+break;
+}
+}
+return ret;
 }
 
 static int virtio_iommu_iov_to_req(struct iovec *iov,
-- 
2.20.1

[PATCH v13 06/10] virtio-iommu: Implement fault reporting

[Eric Auger]

The event queue allows to report asynchronous errors.
The translate function now injects faults when relevant.

Signed-off-by: Eric Auger 

---
v12 -> v13:
- return on virtio_error()

v11 -> v12:
- reporting the addr associated with the fault and set the
  VIRTIO_IOMMU_FAULT_F_ADDRESS flag.
- added cpu_to_le

v10 -> v11:
- change a virtio_error into an error_report_once
  (no buffer available for output faults)
---
 hw/virtio/trace-events   |  1 +
 hw/virtio/virtio-iommu.c | 73 +---
 2 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 095aa8b509..e83500bee9 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -72,3 +72,4 @@ virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
 virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
 virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
 virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t 
sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
+virtio_iommu_report_fault(uint8_t reason, uint32_t flags, uint32_t endpoint, 
uint64_t addr) "FAULT reason=%d flags=%d endpoint=%d address =0x%"PRIx64
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index cc0715284f..ec8f42e167 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -472,6 +472,51 @@ out:
 }
 }
 
+static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
+  int flags, uint32_t endpoint,
+  uint64_t address)
+{
+VirtIODevice *vdev = >parent_obj;
+VirtQueue *vq = viommu->event_vq;
+struct virtio_iommu_fault fault;
+VirtQueueElement *elem;
+size_t sz;
+
+memset(, 0, sizeof(fault));
+fault.reason = reason;
+fault.flags = cpu_to_le32(flags);
+fault.endpoint = cpu_to_le32(endpoint);
+fault.address = cpu_to_le64(address);
+
+for (;;) {
+elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+
+if (!elem) {
+error_report_once(
+"no buffer available in event queue to report event");
+return;
+}
+
+if (iov_size(elem->in_sg, elem->in_num) < sizeof(fault)) {
+virtio_error(vdev, "error buffer of wrong size");
+virtqueue_detach_element(vq, elem, 0);
+g_free(elem);
+return;
+}
+break;
+}
+/* we have a buffer to fill in */
+sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
+  , sizeof(fault));
+assert(sz == sizeof(fault));
+
+trace_virtio_iommu_report_fault(reason, flags, endpoint, address);
+virtqueue_push(vq, elem, sz);
+virtio_notify(vdev, vq);
+g_free(elem);
+
+}
+
 static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 IOMMUAccessFlags flag,
 int iommu_idx)
@@ -480,9 +525,10 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 VirtIOIOMMUInterval interval, *mapping_key;
 VirtIOIOMMUMapping *mapping_value;
 VirtIOIOMMU *s = sdev->viommu;
+bool read_fault, write_fault;
 VirtIOIOMMUEndpoint *ep;
+uint32_t sid, flags;
 bool bypass_allowed;
-uint32_t sid;
 bool found;
 
 interval.low = addr;
@@ -508,6 +554,9 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 if (!ep) {
 if (!bypass_allowed) {
 error_report_once("%s sid=%d is not known!!", __func__, sid);
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_UNKNOWN,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
 } else {
 entry.perm = flag;
 }
@@ -519,6 +568,9 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 error_report_once("%s %02x:%02x.%01x not attached to any domain",
   __func__, PCI_BUS_NUM(sid),
   PCI_SLOT(sid), PCI_FUNC(sid));
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_DOMAIN,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
 } else {
 entry.perm = flag;
 }
@@ -531,15 +583,26 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 if (!found) {
 error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
   __func__, addr, sid);
+virtio_iommu_report_fault(s, VIRTIO_IOMMU_FAULT_R_MAPPING,
+  VIRTIO_IOMMU_FAULT_F_ADDRESS,
+  sid, addr);
 goto unlock;
 }
 
-if (((flag & IOMMU_RO) &&
-!(mapping_value->flags & 

[PATCH v13 08/10] hw/arm/virt: Add the virtio-iommu device tree mappings

[Eric Auger]

Adds the "virtio,pci-iommu" node in the host bridge node and
the RID mapping, excluding the IOMMU RID.

Signed-off-by: Eric Auger 
Reviewed-by: Jean-Philippe Brucker 

---

v11 -> v12:
- Added Jean's R-b

v10 -> v11:
- remove msi_bypass

v8 -> v9:
- disable msi-bypass property
- addition of the subnode is handled is the hotplug handler
  and IOMMU RID is notimposed anymore

v6 -> v7:
- align to the smmu instantiation code

v4 -> v5:
- VirtMachineClass no_iommu added in this patch
- Use object_resolve_path_type
---
 hw/arm/virt.c | 54 ---
 include/hw/arm/virt.h |  2 ++
 2 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 656b0081c2..6cffae7024 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -32,6 +32,7 @@
 #include "qemu-common.h"
 #include "qemu/units.h"
 #include "qemu/option.h"
+#include "monitor/qdev.h"
 #include "qapi/error.h"
 #include "hw/sysbus.h"
 #include "hw/boards.h"
@@ -54,6 +55,7 @@
 #include "qemu/error-report.h"
 #include "qemu/module.h"
 #include "hw/pci-host/gpex.h"
+#include "hw/virtio/virtio-pci.h"
 #include "hw/arm/sysbus-fdt.h"
 #include "hw/platform-bus.h"
 #include "hw/qdev-properties.h"
@@ -71,6 +73,7 @@
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
 #include "hw/acpi/generic_event_device.h"
+#include "hw/virtio/virtio-iommu.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
 static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1180,6 +1183,30 @@ static void create_smmu(const VirtMachineState *vms,
 g_free(node);
 }
 
+static void create_virtio_iommu(VirtMachineState *vms, Error **errp)
+{
+const char compat[] = "virtio,pci-iommu";
+uint16_t bdf = vms->virtio_iommu_bdf;
+char *node;
+
+vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
+
+node = g_strdup_printf("%s/virtio_iommu@%d", vms->pciehb_nodename, bdf);
+qemu_fdt_add_subnode(vms->fdt, node);
+qemu_fdt_setprop(vms->fdt, node, "compatible", compat, sizeof(compat));
+qemu_fdt_setprop_sized_cells(vms->fdt, node, "reg",
+ 1, bdf << 8, 1, 0, 1, 0,
+ 1, 0, 1, 0);
+
+qemu_fdt_setprop_cell(vms->fdt, node, "#iommu-cells", 1);
+qemu_fdt_setprop_cell(vms->fdt, node, "phandle", vms->iommu_phandle);
+g_free(node);
+
+qemu_fdt_setprop_cells(vms->fdt, vms->pciehb_nodename, "iommu-map",
+   0x0, vms->iommu_phandle, 0x0, bdf,
+   bdf + 1, vms->iommu_phandle, bdf + 1, 0x - bdf);
+}
+
 static void create_pcie(VirtMachineState *vms)
 {
 hwaddr base_mmio = vms->memmap[VIRT_PCIE_MMIO].base;
@@ -1258,7 +1285,7 @@ static void create_pcie(VirtMachineState *vms)
 }
 }
 
-nodename = g_strdup_printf("/pcie@%" PRIx64, base);
+nodename = vms->pciehb_nodename = g_strdup_printf("/pcie@%" PRIx64, base);
 qemu_fdt_add_subnode(vms->fdt, nodename);
 qemu_fdt_setprop_string(vms->fdt, nodename,
 "compatible", "pci-host-ecam-generic");
@@ -1301,13 +1328,16 @@ static void create_pcie(VirtMachineState *vms)
 if (vms->iommu) {
 vms->iommu_phandle = qemu_fdt_alloc_phandle(vms->fdt);
 
-create_smmu(vms, pci->bus);
-
-qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map",
-   0x0, vms->iommu_phandle, 0x0, 0x1);
+switch (vms->iommu) {
+case VIRT_IOMMU_SMMUV3:
+create_smmu(vms, pci->bus);
+qemu_fdt_setprop_cells(vms->fdt, nodename, "iommu-map",
+   0x0, vms->iommu_phandle, 0x0, 0x1);
+break;
+default:
+g_assert_not_reached();
+}
 }
-
-g_free(nodename);
 }
 
 static void create_platform_bus(VirtMachineState *vms)
@@ -1971,6 +2001,13 @@ static void virt_machine_device_plug_cb(HotplugHandler 
*hotplug_dev,
 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
 virt_memory_plug(hotplug_dev, dev, errp);
 }
+if (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI)) {
+PCIDevice *pdev = PCI_DEVICE(dev);
+
+vms->iommu = VIRT_IOMMU_VIRTIO;
+vms->virtio_iommu_bdf = pci_get_bdf(pdev);
+create_virtio_iommu(vms, errp);
+}
 }
 
 static void virt_machine_device_unplug_request_cb(HotplugHandler *hotplug_dev,
@@ -1984,7 +2021,8 @@ static HotplugHandler 
*virt_machine_get_hotplug_handler(MachineState *machine,
 DeviceState *dev)
 {
 if (object_dynamic_cast(OBJECT(dev), TYPE_SYS_BUS_DEVICE) ||
-   (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM))) {
+   (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) ||
+   (object_dynamic_cast(OBJECT(dev), TYPE_VIRTIO_IOMMU_PCI))) {
 return HOTPLUG_HANDLER(machine);
 }
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 38f0c33c77..165035fa8f 100644

[PATCH v13 02/10] virtio-iommu: Decode the command payload

[Eric Auger]

This patch adds the command payload decoding and
introduces the functions that will do the actual
command handling. Those functions are not yet implemented.

Signed-off-by: Eric Auger 
Reviewed-by: Jean-Philippe Brucker 
Reviewed-by: Peter Xu 

---

v11 -> v12:
- ADded Jean and Peter's R-b

v10 -> v11:
- use a macro for handle command functions

v9 -> v10:
- make virtio_iommu_handle_* more compact and
  remove get_payload_size

v7 -> v8:
- handle new domain parameter in detach
- remove reserved checks

v5 -> v6:
- change map/unmap semantics (remove size)

v4 -> v5:
- adopt new v0.5 terminology

v3 -> v4:
- no flags field anymore in struct virtio_iommu_req_unmap
- test reserved on attach/detach, change trace proto
- rebase on v2.10.0.
---
 hw/virtio/trace-events   |  4 +++
 hw/virtio/virtio-iommu.c | 76 +---
 2 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 02d93d7f63..f7141aa2f6 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -60,3 +60,7 @@ virtio_iommu_get_features(uint64_t features) "device supports 
features=0x%"PRIx6
 virtio_iommu_device_status(uint8_t status) "driver status = %d"
 virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, 
uint32_t domain_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" 
start=0x%"PRIx64" end=0x%"PRIx64" domain_range=%d probe_size=0x%x"
 virtio_iommu_set_config(uint64_t page_size_mask, uint64_t start, uint64_t end, 
uint32_t domain_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" 
start=0x%"PRIx64" end=0x%"PRIx64" domain_bits=%d probe_size=0x%x"
+virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
+virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
+virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, 
uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" 
virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
+virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) 
"domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 7e55eda67e..9d1b997df7 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -34,31 +34,83 @@
 /* Max size */
 #define VIOMMU_DEFAULT_QUEUE_SIZE 256
 
-static int virtio_iommu_handle_attach(VirtIOIOMMU *s,
-  struct iovec *iov,
-  unsigned int iov_cnt)
+static int virtio_iommu_attach(VirtIOIOMMU *s,
+   struct virtio_iommu_req_attach *req)
 {
+uint32_t domain_id = le32_to_cpu(req->domain);
+uint32_t ep_id = le32_to_cpu(req->endpoint);
+
+trace_virtio_iommu_attach(domain_id, ep_id);
+
 return VIRTIO_IOMMU_S_UNSUPP;
 }
-static int virtio_iommu_handle_detach(VirtIOIOMMU *s,
-  struct iovec *iov,
-  unsigned int iov_cnt)
+
+static int virtio_iommu_detach(VirtIOIOMMU *s,
+   struct virtio_iommu_req_detach *req)
 {
+uint32_t domain_id = le32_to_cpu(req->domain);
+uint32_t ep_id = le32_to_cpu(req->endpoint);
+
+trace_virtio_iommu_detach(domain_id, ep_id);
+
 return VIRTIO_IOMMU_S_UNSUPP;
 }
-static int virtio_iommu_handle_map(VirtIOIOMMU *s,
-   struct iovec *iov,
-   unsigned int iov_cnt)
+
+static int virtio_iommu_map(VirtIOIOMMU *s,
+struct virtio_iommu_req_map *req)
 {
+uint32_t domain_id = le32_to_cpu(req->domain);
+uint64_t phys_start = le64_to_cpu(req->phys_start);
+uint64_t virt_start = le64_to_cpu(req->virt_start);
+uint64_t virt_end = le64_to_cpu(req->virt_end);
+uint32_t flags = le32_to_cpu(req->flags);
+
+trace_virtio_iommu_map(domain_id, virt_start, virt_end, phys_start, flags);
+
 return VIRTIO_IOMMU_S_UNSUPP;
 }
-static int virtio_iommu_handle_unmap(VirtIOIOMMU *s,
- struct iovec *iov,
- unsigned int iov_cnt)
+
+static int virtio_iommu_unmap(VirtIOIOMMU *s,
+  struct virtio_iommu_req_unmap *req)
 {
+uint32_t domain_id = le32_to_cpu(req->domain);
+uint64_t virt_start = le64_to_cpu(req->virt_start);
+uint64_t virt_end = le64_to_cpu(req->virt_end);
+
+trace_virtio_iommu_unmap(domain_id, virt_start, virt_end);
+
 return VIRTIO_IOMMU_S_UNSUPP;
 }
 
+static int virtio_iommu_iov_to_req(struct iovec *iov,
+   unsigned int iov_cnt,
+   void *req, size_t req_sz)
+{
+size_t sz, payload_sz = req_sz - sizeof(struct virtio_iommu_req_tail);
+
+sz = iov_to_buf(iov, iov_cnt, 0, req, payload_sz);
+if (unlikely(sz != payload_sz)) {
+return 

[PATCH v13 05/10] virtio-iommu: Implement translate

[Eric Auger]

This patch implements the translate callback

Signed-off-by: Eric Auger 
Reviewed-by: Jean-Philippe Brucker 

---

v11 -> v12:
- Added Jean's R-b
- s/qemu_log_mask/error_report_once

v10 -> v11:
- take into account the new value struct and use
  g_tree_lookup_extended
- switched to error_report_once

v6 -> v7:
- implemented bypass-mode

v5 -> v6:
- replace error_report by qemu_log_mask

v4 -> v5:
- check the device domain is not NULL
- s/printf/error_report
- set flags to IOMMU_NONE in case of all translation faults
---
 hw/virtio/trace-events   |  1 +
 hw/virtio/virtio-iommu.c | 60 +++-
 2 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index 22162d6583..095aa8b509 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -71,3 +71,4 @@ virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc endpoint=%d"
 virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
 virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
 virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
+virtio_iommu_translate_out(uint64_t virt_addr, uint64_t phys_addr, uint32_t 
sid) "0x%"PRIx64" -> 0x%"PRIx64 " for sid=%d"
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index e1e0c69473..cc0715284f 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -477,19 +477,77 @@ static IOMMUTLBEntry 
virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
 int iommu_idx)
 {
 IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
+VirtIOIOMMUInterval interval, *mapping_key;
+VirtIOIOMMUMapping *mapping_value;
+VirtIOIOMMU *s = sdev->viommu;
+VirtIOIOMMUEndpoint *ep;
+bool bypass_allowed;
 uint32_t sid;
+bool found;
+
+interval.low = addr;
+interval.high = addr + 1;
 
 IOMMUTLBEntry entry = {
 .target_as = _space_memory,
 .iova = addr,
 .translated_addr = addr,
-.addr_mask = ~(hwaddr)0,
+.addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1,
 .perm = IOMMU_NONE,
 };
 
+bypass_allowed = virtio_vdev_has_feature(>parent_obj,
+ VIRTIO_IOMMU_F_BYPASS);
+
 sid = virtio_iommu_get_bdf(sdev);
 
 trace_virtio_iommu_translate(mr->parent_obj.name, sid, addr, flag);
+qemu_mutex_lock(>mutex);
+
+ep = g_tree_lookup(s->endpoints, GUINT_TO_POINTER(sid));
+if (!ep) {
+if (!bypass_allowed) {
+error_report_once("%s sid=%d is not known!!", __func__, sid);
+} else {
+entry.perm = flag;
+}
+goto unlock;
+}
+
+if (!ep->domain) {
+if (!bypass_allowed) {
+error_report_once("%s %02x:%02x.%01x not attached to any domain",
+  __func__, PCI_BUS_NUM(sid),
+  PCI_SLOT(sid), PCI_FUNC(sid));
+} else {
+entry.perm = flag;
+}
+goto unlock;
+}
+
+found = g_tree_lookup_extended(ep->domain->mappings, (gpointer)(),
+   (void **)_key,
+   (void **)_value);
+if (!found) {
+error_report_once("%s no mapping for 0x%"PRIx64" for sid=%d",
+  __func__, addr, sid);
+goto unlock;
+}
+
+if (((flag & IOMMU_RO) &&
+!(mapping_value->flags & VIRTIO_IOMMU_MAP_F_READ)) ||
+((flag & IOMMU_WO) &&
+!(mapping_value->flags & VIRTIO_IOMMU_MAP_F_WRITE))) {
+error_report_once("%s permission error on 0x%"PRIx64"(%d): allowed=%d",
+  __func__, addr, flag, mapping_value->flags);
+goto unlock;
+}
+entry.translated_addr = addr - mapping_key->low + mapping_value->phys_addr;
+entry.perm = flag;
+trace_virtio_iommu_translate_out(addr, entry.translated_addr, sid);
+
+unlock:
+qemu_mutex_unlock(>mutex);
 return entry;
 }
 
-- 
2.20.1

[PATCH v13 03/10] virtio-iommu: Implement attach/detach command

[Eric Auger]

This patch implements the endpoint attach/detach to/from
a domain.

Domain and endpoint internal datatypes are introduced.
Both are stored in RB trees. The domain owns a list of
endpoints attached to it. Also helpers to get/put
end points and domains are introduced.

As for the IOMMU memory regions, a callback is called on
PCI bus enumeration that initializes for a given device
on the bus hierarchy an IOMMU memory region. The PCI bus
hierarchy is stored locally in IOMMUPciBus and IOMMUDevice
objects.

At the time of the enumeration, the bus number may not be
computed yet.

So operations that will need to retrieve the IOMMUdevice
and its IOMMU memory region from the bus number and devfn,
once the bus number is garanteed to be frozen, use an array
of IOMMUPciBus, lazily populated.

Signed-off-by: Eric Auger 

---

v12 -> v13:
- squashed v12 4, 5, 6 into this patch
- rename virtio_iommu_get_sid into virtio_iommu_get_bdf

v11 -> v12:
- check the device is protected by the iommu on attach
- on detach, check the domain id the device is attached to matches
  the one used in the detach command
- fix mapping ref counter and destroy the domain when no end-points
  are attached anymore
---
 hw/virtio/trace-events   |   6 +
 hw/virtio/virtio-iommu.c | 315 ++-
 include/hw/virtio/virtio-iommu.h |   3 +
 3 files changed, 322 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index f7141aa2f6..15595f8cd7 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -64,3 +64,9 @@ virtio_iommu_attach(uint32_t domain_id, uint32_t ep_id) 
"domain=%d endpoint=%d"
 virtio_iommu_detach(uint32_t domain_id, uint32_t ep_id) "domain=%d endpoint=%d"
 virtio_iommu_map(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end, 
uint64_t phys_start, uint32_t flags) "domain=%d virt_start=0x%"PRIx64" 
virt_end=0x%"PRIx64 " phys_start=0x%"PRIx64" flags=%d"
 virtio_iommu_unmap(uint32_t domain_id, uint64_t virt_start, uint64_t virt_end) 
"domain=%d virt_start=0x%"PRIx64" virt_end=0x%"PRIx64
+virtio_iommu_translate(const char *name, uint32_t rid, uint64_t iova, int 
flag) "mr=%s rid=%d addr=0x%"PRIx64" flag=%d"
+virtio_iommu_init_iommu_mr(char *iommu_mr) "init %s"
+virtio_iommu_get_endpoint(uint32_t ep_id) "Alloc endpoint=%d"
+virtio_iommu_put_endpoint(uint32_t ep_id) "Free endpoint=%d"
+virtio_iommu_get_domain(uint32_t domain_id) "Alloc domain=%d"
+virtio_iommu_put_domain(uint32_t domain_id) "Free domain=%d"
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 9d1b997df7..e5cc94138b 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -23,6 +23,8 @@
 #include "hw/qdev-properties.h"
 #include "hw/virtio/virtio.h"
 #include "sysemu/kvm.h"
+#include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "trace.h"
 
 #include "standard-headers/linux/virtio_ids.h"
@@ -30,19 +32,234 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 #include "hw/virtio/virtio-iommu.h"
+#include "hw/pci/pci_bus.h"
+#include "hw/pci/pci.h"
 
 /* Max size */
 #define VIOMMU_DEFAULT_QUEUE_SIZE 256
 
+typedef struct VirtIOIOMMUDomain {
+uint32_t id;
+GTree *mappings;
+QLIST_HEAD(, VirtIOIOMMUEndpoint) endpoint_list;
+} VirtIOIOMMUDomain;
+
+typedef struct VirtIOIOMMUEndpoint {
+uint32_t id;
+VirtIOIOMMUDomain *domain;
+QLIST_ENTRY(VirtIOIOMMUEndpoint) next;
+} VirtIOIOMMUEndpoint;
+
+typedef struct VirtIOIOMMUInterval {
+uint64_t low;
+uint64_t high;
+} VirtIOIOMMUInterval;
+
+static inline uint16_t virtio_iommu_get_bdf(IOMMUDevice *dev)
+{
+return PCI_BUILD_BDF(pci_bus_num(dev->bus), dev->devfn);
+}
+
+/**
+ * The bus number is used for lookup when SID based operations occur.
+ * In that case we lazily populate the IOMMUPciBus array from the bus hash
+ * table. At the time the IOMMUPciBus is created (iommu_find_add_as), the bus
+ * numbers may not be always initialized yet.
+ */
+static IOMMUPciBus *iommu_find_iommu_pcibus(VirtIOIOMMU *s, uint8_t bus_num)
+{
+IOMMUPciBus *iommu_pci_bus = s->iommu_pcibus_by_bus_num[bus_num];
+
+if (!iommu_pci_bus) {
+GHashTableIter iter;
+
+g_hash_table_iter_init(, s->as_by_busptr);
+while (g_hash_table_iter_next(, NULL, (void **)_pci_bus)) {
+if (pci_bus_num(iommu_pci_bus->bus) == bus_num) {
+s->iommu_pcibus_by_bus_num[bus_num] = iommu_pci_bus;
+return iommu_pci_bus;
+}
+}
+return NULL;
+}
+return iommu_pci_bus;
+}
+
+static IOMMUMemoryRegion *virtio_iommu_mr(VirtIOIOMMU *s, uint32_t sid)
+{
+uint8_t bus_n, devfn;
+IOMMUPciBus *iommu_pci_bus;
+IOMMUDevice *dev;
+
+bus_n = PCI_BUS_NUM(sid);
+iommu_pci_bus = iommu_find_iommu_pcibus(s, bus_n);
+if (iommu_pci_bus) {
+devfn = sid & PCI_DEVFN_MAX;
+dev = iommu_pci_bus->pbdev[devfn];
+if (dev) {
+return >iommu_mr;
+}
+}

[PATCH v13 01/10] virtio-iommu: Add skeleton

[Eric Auger]

This patchs adds the skeleton for the virtio-iommu device.

Signed-off-by: Eric Auger 
Reviewed-by: Peter Xu 

---

v12 -> v13
- removed IOMMU_PCI_BUS_MAX and IOMMU_PCI_DEVFN_MAX

v11 -> v12:
- remove s_by_bus_num
- drop set_features (rely on default implementation) and
  acked_features

v9 -> v10:
- mutex initialized here
- initialize tail
- included hw/qdev-properties.h
- removed g_memdup
- removed s->config.domain_range.start = 0;

v9 -> v10:
- expose VIRTIO_IOMMU_F_MMIO feature
- s/domain_bits/domain_range struct
- change error codes
- enforce unmigratable
- Kconfig

v7 -> v8:
- expose VIRTIO_IOMMU_F_BYPASS and VIRTIO_F_VERSION_1
  features
- set_config dummy implementation + tracing
- add trace in get_features
- set the features on realize() and store the acked ones
- remove inclusion of linux/virtio_iommu.h

v6 -> v7:
- removed qapi-event.h include
- add primary_bus and associated property

v4 -> v5:
- use the new v0.5 terminology (domain, endpoint)
- add the event virtqueue

v3 -> v4:
- use page_size_mask instead of page_sizes
- added set_features()
- added some traces (reset, set_status, set_features)
- empty virtio_iommu_set_config() as the driver MUST NOT
  write to device configuration fields
- add get_config trace

v2 -> v3:
- rebase on 2.10-rc0, ie. use IOMMUMemoryRegion and remove
  iommu_ops.
- advertise VIRTIO_IOMMU_F_MAP_UNMAP feature
- page_sizes set to TARGET_PAGE_SIZE

Conflicts:
hw/virtio/trace-events
---
 hw/virtio/Kconfig|   5 +
 hw/virtio/Makefile.objs  |   1 +
 hw/virtio/trace-events   |   7 +
 hw/virtio/virtio-iommu.c | 265 +++
 include/hw/virtio/virtio-iommu.h |  57 +++
 5 files changed, 335 insertions(+)
 create mode 100644 hw/virtio/virtio-iommu.c
 create mode 100644 include/hw/virtio/virtio-iommu.h

diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
index f87def27a6..d29525b36f 100644
--- a/hw/virtio/Kconfig
+++ b/hw/virtio/Kconfig
@@ -9,6 +9,11 @@ config VIRTIO_RNG
 default y
 depends on VIRTIO
 
+config VIRTIO_IOMMU
+bool
+default y
+depends on VIRTIO
+
 config VIRTIO_PCI
 bool
 default y if PCI_DEVICES
diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index de0f5fc39b..2fd9da7410 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -16,6 +16,7 @@ obj-$(call land,$(CONFIG_VIRTIO_CRYPTO),$(CONFIG_VIRTIO_PCI)) 
+= virtio-crypto-p
 obj-$(CONFIG_VIRTIO_PMEM) += virtio-pmem.o
 common-obj-$(call land,$(CONFIG_VIRTIO_PMEM),$(CONFIG_VIRTIO_PCI)) += 
virtio-pmem-pci.o
 obj-$(call land,$(CONFIG_VHOST_USER_FS),$(CONFIG_VIRTIO_PCI)) += 
vhost-user-fs-pci.o
+obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o
 
 ifeq ($(CONFIG_VIRTIO_PCI),y)
diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events
index e28ba48da6..02d93d7f63 100644
--- a/hw/virtio/trace-events
+++ b/hw/virtio/trace-events
@@ -53,3 +53,10 @@ virtio_mmio_write_offset(uint64_t offset, uint64_t value) 
"virtio_mmio_write off
 virtio_mmio_guest_page(uint64_t size, int shift) "guest page size 0x%" PRIx64 
" shift %d"
 virtio_mmio_queue_write(uint64_t value, int max_size) "mmio_queue write 0x%" 
PRIx64 " max %d"
 virtio_mmio_setting_irq(int level) "virtio_mmio setting IRQ %d"
+
+# hw/virtio/virtio-iommu.c
+virtio_iommu_device_reset(void) "reset!"
+virtio_iommu_get_features(uint64_t features) "device supports 
features=0x%"PRIx64
+virtio_iommu_device_status(uint8_t status) "driver status = %d"
+virtio_iommu_get_config(uint64_t page_size_mask, uint64_t start, uint64_t end, 
uint32_t domain_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" 
start=0x%"PRIx64" end=0x%"PRIx64" domain_range=%d probe_size=0x%x"
+virtio_iommu_set_config(uint64_t page_size_mask, uint64_t start, uint64_t end, 
uint32_t domain_range, uint32_t probe_size) "page_size_mask=0x%"PRIx64" 
start=0x%"PRIx64" end=0x%"PRIx64" domain_bits=%d probe_size=0x%x"
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
new file mode 100644
index 00..7e55eda67e
--- /dev/null
+++ b/hw/virtio/virtio-iommu.c
@@ -0,0 +1,265 @@
+/*
+ * virtio-iommu device
+ *
+ * Copyright (c) 2017 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/iov.h"
+#include "qemu-common.h"
+#include "hw/qdev-properties.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"

[PATCH v13 00/10] VIRTIO-IOMMU device

[Eric Auger]

This series implements the QEMU virtio-iommu device.

This matches the v0.12 spec (voted) and the corresponding
virtio-iommu driver upstreamed in 5.3. All kernel dependencies
are resolved for DT integration. The virtio-iommu can be
instantiated in ARM virt using "-device virtio-iommu-pci".

Non DT mode is not yet supported as it has non resolved kernel
dependencies [1].

This feature targets 5.0.

Integration with vhost devices and vfio devices is not part
of this series. Please follow Bharat's respins [2].

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/v4.2-virtio-iommu-v13

References:
[1] [RFC 00/13] virtio-iommu on non-devicetree platforms
[2] [PATCH RFC v5 0/5] virtio-iommu: VFIO integration

Testing:
- tested with guest using virtio-net-pci
  (,vhost=off,iommu_platform,disable-modern=off,disable-legacy=on)
  and virtio-blk-pci
- migration

History:

v12 -> v13:
- Take into account Peter's comments
- fix qtest error and accomodate for directory changes in
  test
- remove "[PATCH v12 01/13] migration: Support QLIST migration"
  which is now upstream
- fix iommu_find_iommu_pcibus()
- squash commits as requested by Peter
- remove spurious guest log

v11 -> v12:
- took into account Peter and Jean's comments
  - use guest features
  - restore as_by_bus_num and when attaching devices, check they are
actually protected by the IOMMU. Updated the tests accordingly.
  - fix the mapping ref counting and make sure mappings are properly
cleaned.
  - Use CamelCase for data types
  - simplify postload callback as suggested by Peter
  - add R-bs
- fix mingw compilation issue
- add IOMMU migration priority
- qlist migration load simplified following Juan's suggestion

v10 -> v11:
- introduce virtio_iommu_handle_req macro
- migration support
- introduce DEFINE_PROP_INTERVAL and pass reserved regions
  through an array of those
- domain gtree simplification

v9 -> v10:
- rebase on 4.1.0-rc2, compliance with 0.12 spec
- removed ACPI part
- cleanup (see individual change logs)
- moved to a PATCH series

v8 -> v9:
- virtio-iommu-pci device needs to be instantiated from the command
  line (RID is not imposed anymore).
- tail structure properly initialized

v7 -> v8:
- virtio-iommu-pci added
- virt instantiation modified
- DT and ACPI modified to exclude the iommu RID from the mapping
- VIRTIO_IOMMU_F_BYPASS, VIRTIO_F_VERSION_1 features exposed

v6 -> v7:
- rebase on qemu 3.0.0-rc3
- minor update against v0.7
- fix issue with EP not on pci.0 and ACPI probing
- change the instantiation method

v5 -> v6:
- minor update against v0.6 spec
- fix g_hash_table_lookup in virtio_iommu_find_add_as
- replace some error_reports by qemu_log_mask(LOG_GUEST_ERROR, ...)

v4 -> v5:
- event queue and fault reporting
- we now return the IOAPIC MSI region if the virtio-iommu is instantiated
  in a PC machine.
- we bypass transactions on MSI HW region and fault on reserved ones.
- We support ACPI boot with mach-virt (based on IORT proposal)
- We moved to the new driver naming conventions
- simplified mach-virt instantiation
- worked around the disappearing of pci_find_primary_bus
- in virtio_iommu_translate, check the dev->as is not NULL
- initialize as->device_list in virtio_iommu_get_as
- initialize bufstate.error to false in virtio_iommu_probe

v3 -> v4:
- probe request support although no reserved region is returned at
  the moment
- unmap semantics less strict, as specified in v0.4
- device registration, attach/detach revisited
- split into smaller patches to ease review
- propose a way to inform the IOMMU mr about the page_size_mask
  of underlying HW IOMMU, if any
- remove warning associated with the translation of the MSI doorbell

v2 -> v3:
- rebase on top of 2.10-rc0 and especially
  [PATCH qemu v9 0/2] memory/iommu: QOM'fy IOMMU MemoryRegion
- add mutex init
- fix as->mappings deletion using g_tree_ref/unref
- when a dev is attached whereas it is already attached to
  another address space, first detach it
- fix some error values
- page_sizes = TARGET_PAGE_MASK;
- I haven't changed the unmap() semantics yet, waiting for the
  next virtio-iommu spec revision.

v1 -> v2:
- fix redefinition of viommu_as typedef


Eric Auger (10):
  virtio-iommu: Add skeleton
  virtio-iommu: Decode the command payload
  virtio-iommu: Implement attach/detach command
  virtio-iommu: Implement map/unmap
  virtio-iommu: Implement translate
  virtio-iommu: Implement fault reporting
  virtio-iommu-pci: Add virtio iommu pci support
  hw/arm/virt: Add the virtio-iommu device tree mappings
  virtio-iommu: Support migration
  tests: Add virtio-iommu test

 hw/arm/virt.c |  54 +-
 hw/virtio/Kconfig |   5 +
 hw/virtio/Makefile.objs   |   2 +
 hw/virtio/trace-events|  20 +
 hw/virtio/virtio-iommu-pci.c  |  88 +++
 hw/virtio/virtio-iommu.c  | 897 ++
 include/hw/arm/virt.h |   2 +
 include/hw/pci/pci.h  | 

Re: Making QEMU easier for management tools and applications

[Paolo Bonzini]

Il mer 15 gen 2020, 10:21 Markus Armbruster  ha scritto:

> > We don’t want the QAPI to let arbitrary fields of a QOM object
> > be modified, do we?
>
> We already do: QMP command qom-set.  If it breaks your guest, you get to
> keep the pieces.
>

That's not true. We chose not to make that a "recommended" interface, and
instead we add new commands. However that's mostly to avoid tying our hands
and not making too much of QOM part of the API. But I would be very
surprised if a guest could be broken with qom-set.

This was definitely not the case when QOM was introduced in a half-baked
state, but let's not indulge in self-flagellation more than it's actually
necessary.



> > As for the “public” aspects of a QOM object, that is static if it
> > comes from QAPI, so why couldn’t we define it there?
>
> QAPI specifies a certain kind of data types.
>
> QOM "specifies" data types as an imperative program for creating them.
> Maximally flexible, and fundamentally at odds with static analysis.
> I've hated this since day one.
>
> There's no hard reason why QOM could not specify static aspects
> declaratively.  It just doesn't, and changing it now would be a
> monumental task.
>
> The imperative program mostly creates identical data types every time.
> In other words, the data types are static.  Two problems.  One,
> converting such a program to an equivalent declaration takes manual
> labor.  Two, there are exceptions, and identifying them is more manual
> labor.
>
> >>> the same way, QAPI can't say anything about the structure of a QOM
> >>> object, and I think that's a problem.
> >>>
> > - which, as far as I
> > understand, is mainly because QOM properties aren't necessarily
> static,
> > so we can't provide a statically defined interface for them. Probably
> > solvable in QEMU, but not without a major effort.
> 
>  Or maybe extend the language so that it’s internal semantics
>  knows about this aspect of QOM?
> >>>
> >>> My point is that for example you can't generate a C struct (which is
> >>> statically defined) from something that has a dynamic structure. The
> >>> best you could do is fall back to key=value even in the C source, both
> >>> key and value being strings. But then you still have to parse these
> >>> strings manually, so it doesn't actually help much compared to a state
> >>> without C bindings.
> >
> > I really don’t understand that point. To me, all these operations
> > seem relatively simple to generate.
>
> Yes, if the QAPI fairy gives us a declarative specification equivalent
> to the existing imperative one, then replacing the hand-written
> imperative code by code generated from the declarative specification
> would be relatively simple.
>
> > I think that what confuses me is when you write “something that has a
> > dynamic structure”. I understand that as referring to the structure
> > defined in the schema. But the schema itself is static. So you can
> > generate static code from it, and it’s already done today.
>
> QAPI data types are static.
>
> QOM types are not.  They're effectively almost static in practice.
>
> > Another hypothesis on my side is that we don’t want, ever, to
> > have the API provide for example the option to create its own
> > arbitrary QOM classes, or to tag arbitrary properties to an object,
> > where by “arbitrary” I mean not explicitly mentioned somewhere in
> > something like the schema.
> >
> > So I suspect you are talking about something else.
>
> Kevin's talking about the imperative code creating different QOM
> properties depending on how and in what context it is run.
>
> QOM is perfectly capable of supporting a QMP command to add arbitrary
> QOM properties to an object at run time, or even add arbitrary QOM
> types, but as you wrote we don't want that.
>
> >> QOM and QAPI sabotage each other.  Ironic, considering they were dreamed
> >> up by the same guy :)
> >>
> >> QAPI is compile-time static by design.
> >>
> >> QOM is run-time dynamic by design.  Some support for static definitions
> >> has been grafted on since.
> >>
> >> We use QAPI type system escapes for QOM.  Defeats QAPI introspection and
> >> doc generation.  We provide separate QOM introspection instead, which is
> >> clumsier and less expressive.  QOM documentation doesn't exist.
> >
> > But back to the original discussion about management tools,
> > do we let upper layers tag their own arbitrary stuff in QOM objects?
>
> We don't want management applications to add QOM properties for their
> own purposes.
>
> >>> Maybe what could be done is covering at least the static properties and
> >>> then having key=value for the dynamic part (which should be the
> >>> exception).
> >>
> >> To make this worthwhile, we'd have to replace dynamic QOM properties by
> >> static ones when possible.  Monumental task.
> >
> > I’m sure you are right, but it’s hard for me to evaluate, given how
> > many ways there are to access an object. Naively, grepping for
> > set_prop and for 

Re: [PATCH rc2 01/25] target/avr: Add outward facing interfaces and core CPU logic

[Thomas Huth]

On 25/01/2020 11.48, Aleksandar Markovic wrote:
> 
> On Friday, January 24, 2020, Philippe Mathieu-Daudé  > wrote:
> 
> From: Michael Rolnik mailto:mrol...@gmail.com>>
> 
> This includes:
> - CPU data structures
> - object model classes and functions
> - migration functions
> - GDB hooks
> 
> I have an objection over this patch.
> 
> It contains many diverse logical units squashed into a patch, and
> therefore is not in accordance to our submission giidelines.

I think you have to decide on a case by case basis. This is a new target
for a hobbyist board ... so I think you don't have to thaaat strict here.

> If we accept this patch, we will be setting a bad precedent, that may
> misled future platform contributors. additionally, this patch may be
> singled out in our countribution guidelines as the example how not to do
> a parch, which is probably not what Michael want to be exposed to.

Honestly, I'd leave that decision to the person who picks up the patch
and sends a pull request, i.e. Richard in this case, I assume.

> Splitting patches is tedius, but overall not that difficult or time
> consuming task.

OTOH hand, we've seen more than 40 iterations of the patch series
already. Michael invested a lot of time into this series, so if you make
him respin forever, he'll certainly rather lose interest at one point in
time. So I'd say if Richard is fine with the series, I'd rather not like
to see yet another iteration on the mailing list again and rather see a
pull request for these patches instead.

 Thomas

Re: [PATCH] tests/acceptance: Add boot tests for some of the QEMU advent calendar images

[Thomas Huth]

On 24/01/2020 22.28, Wainer dos Santos Moschetta wrote:
> 
> On 1/24/20 3:03 PM, Thomas Huth wrote:
>> The 2018 edition of the QEMU advent calendar 2018 featured Linux images
>> for various non-x86 machines. We can use them for a boot tests in our
>> acceptance test suite.
>>
>> Let's also make sure that we build the corresponding machines in Travis,
>> and while we're there, drop the superfluous --python parameter (python3
>> is now the only supported version anyway).
> 
> Yes, please, removal of --python was in my wish list.
> 
>>
>> Signed-off-by: Thomas Huth 
>> ---
>>   .travis.yml    |  2 +-
>>   tests/acceptance/boot_linux_console.py | 96 ++
>>   2 files changed, 97 insertions(+), 1 deletion(-)
>>
>> diff --git a/.travis.yml b/.travis.yml
>> index 6c1038a0f1..73ca12c921 100644
>> --- a/.travis.yml
>> +++ b/.travis.yml
>> @@ -268,7 +268,7 @@ matrix:
>>     # Acceptance (Functional) tests
>>   - env:
>> -    - CONFIG="--python=/usr/bin/python3
>> --target-list=x86_64-softmmu,mips-softmmu,mips64el-softmmu,aarch64-softmmu,arm-softmmu,s390x-softmmu,alpha-softmmu,ppc-softmmu,ppc64-softmmu,m68k-softmmu,sparc-softmmu"
>>
>> +    -
>> CONFIG="--target-list=aarch64-softmmu,alpha-softmmu,arm-softmmu,m68k-softmmu,microblaze-softmmu,mips-softmmu,mips64el-softmmu,nios2-softmmu,or1k-softmmu,ppc-softmmu,ppc64-softmmu,s390x-softmmu,sparc-softmmu,x86_64-softmmu,xtensa-softmmu"
>>
> 
> 
> Perhaps use MAIN_SOFTMMU_TARGETS in only append the other targets, like:
> 
> --target-list=${MAIN_SOFTMMU_TARGETS},alpha-softmmu,sparc-softmmu,

Not sure ... while it is a nice way to shorten the line here, it adds a
dependecy to that variable ... and MAIN_SOFTMMU_TARGETS has been changed
a couple of times during the course of time, so we might risk to lose
some testing coverage here in case someone removes a target from
MAIN_SOFTMMU_TARGETS but forgets to add it here again...? I think we
should better use the explicit list here instead.

 Thomas

Re: [PATCH] bsd-user: improve support for sparc syscall flags

[Laurent Vivier]

Le 25/01/2020 à 12:55, no-re...@patchew.org a écrit :
> Patchew URL: 
> https://patchew.org/QEMU/20200125114753.61820-1-salva...@qindel.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 

Salvador,

you can use scripts/checkpatch.pl in the QEMU directory to check your
patch for style before sending them.

And don't send them as a reply but as a new thread, using versioning in
the subject ("[PATCH v4]").

Thanks,
Laurent

Re: [PATCH] bsd-user: improve support for sparc syscall flags

[no-reply]

Patchew URL: 
https://patchew.org/QEMU/20200125114753.61820-1-salva...@qindel.com/



Hi,

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

Type: series
Message-id: 20200125114753.61820-1-salva...@qindel.com
Subject: [PATCH] bsd-user: improve support for sparc syscall flags

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]  patchew/20200124005131.16276-1-f4...@amsat.org -> 
patchew/20200124005131.16276-1-f4...@amsat.org
 * [new tag] patchew/20200125114753.61820-1-salva...@qindel.com -> 
patchew/20200125114753.61820-1-salva...@qindel.com
Switched to a new branch 'test'
e256a0f bsd-user: improve support for sparc syscall flags

=== OUTPUT BEGIN ===
ERROR: code indent should never use tabs
#132: FILE: bsd-user/netbsd/syscall_nr.h:375:
+/*^I$NetBSD: trap.h,v 1.18 2011/03/27 18:47:08 martin Exp $ */$

ERROR: code indent should never use tabs
#136: FILE: bsd-user/netbsd/syscall_nr.h:379:
+ *^IThe Regents of the University of California.  All rights reserved.$

ERROR: code indent should never use tabs
#144: FILE: bsd-user/netbsd/syscall_nr.h:387:
+ *^IThis product includes software developed by the University of$

ERROR: code indent should never use tabs
#145: FILE: bsd-user/netbsd/syscall_nr.h:388:
+ *^ICalifornia, Lawrence Berkeley Laboratory.$

ERROR: code indent should never use tabs
#171: FILE: bsd-user/netbsd/syscall_nr.h:414:
+ *^I@(#)trap.h^I8.1 (Berkeley) 6/11/93$

ERROR: line over 90 characters
#180: FILE: bsd-user/netbsd/syscall_nr.h:423:
+#defineTARGET_NETBSD_SYSCALL_G2RFLAG   0x400   /* on success, return 
to %g2 rather than npc */

ERROR: code indent should never use tabs
#180: FILE: bsd-user/netbsd/syscall_nr.h:423:
+#define^ITARGET_NETBSD_SYSCALL_G2RFLAG^I0x400^I/* on success, return to %g2 
rather than npc */$

WARNING: line over 80 characters
#181: FILE: bsd-user/netbsd/syscall_nr.h:424:
+#defineTARGET_NETBSD_SYSCALL_G7RFLAG   0x800   /* use %g7 as above 
(deprecated) */

ERROR: code indent should never use tabs
#181: FILE: bsd-user/netbsd/syscall_nr.h:424:
+#define^ITARGET_NETBSD_SYSCALL_G7RFLAG^I0x800^I/* use %g7 as above 
(deprecated) */$

ERROR: line over 90 characters
#182: FILE: bsd-user/netbsd/syscall_nr.h:425:
+#defineTARGET_NETBSD_SYSCALL_G5RFLAG   0xc00   /* use %g5 as above 
(only ABI compatible way) */

ERROR: code indent should never use tabs
#182: FILE: bsd-user/netbsd/syscall_nr.h:425:
+#define^ITARGET_NETBSD_SYSCALL_G5RFLAG^I0xc00^I/* use %g5 as above (only ABI 
compatible way) */$

total: 10 errors, 1 warnings, 149 lines checked

Commit e256a0ff22ca (bsd-user: improve support for sparc syscall flags) has 
style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200125114753.61820-1-salva...@qindel.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-de...@redhat.com

Re: Making QEMU easier for management tools and applications

[Paolo Bonzini]

On 24/12/19 14:41, Daniel P. Berrangé wrote:
> I'm wondering if we need to think about a temporary *throwaway* fork of
> QEMU. I wasn't involved in the work, but as an outside observer I think
> it is interesting to see how the NEMU project fork ultimately led to QEMU
> moving forward with a new KConfig approach, and the integration of microvm.
> They had the freedom to develop these new approaches with zero regard
> to back compat or broader QEMU needs that might otherwise hold back dev.

Just a quick note: none of these were developed first in NEMU.  KConfig
was completed *for* NEMU (to make their lives easier) but the code
already existed, and microvm is inspired by Firecracker.  NEMU's main
merit in my opinion was to make QEMU's perception issues clearer, and to
cause more thought on those issues and how to approach it.

That said, I agree that a throwaway experiment on command line and
configuration is worthwhile.  The main thing to think about is to make
convenience options syntactic sugar for something else.  For example, a
couple ideas mentioned in the thread or elsewhere in hallway discussions:

- Make default devices expressible as QemuOpts, so that -writeconfig
able to write out the default devices as well and -readconfig would
imply -nodefaults.

- Reduce the scope of -drive by dropping if=none in favor of -blockdev.
 Leave -drive as a quick way of configuring frontend and backend.

- And somewhat unrelated to configuration files: generate bindings for
QMP clients from the schema.

In the meanwhile, we should deprecate -readconfig/-writeconfig.  Can't
go wrong with that.

Paolo

Re: Making QEMU easier for management tools and applications

[Paolo Bonzini]

On 24/01/20 10:50, Daniel P. Berrangé wrote:
>   * qemu-launcher-$TARGET
> 
> A binary that is able to launch qemu-runtime-$TARGET
> with jailers active.
> 
> This has no command line arguments except for a pair
> of UNIX socket paths. One is a QMP server, the other
> is the path for the QMP of qemu-runtime-$TARGET.
> 
> Commands it processes will be in automatically proxied
> through to the qemu-runtime-$TARGET QMP, with appropriate
> jailer updates being done in between.
> 

What would be the advantage of this over the Libvirt embedded driver?
Especially if you include in the picture something like libvirt-go-xml
(or libvirt-GObject, does it still exist?) that hides the XML from the
code that uses it.

The main complication in the launcher is hotplug, which means that a
simple "do a couple bind mounts, unshare, drop privileges and forget
about it" approach doesn't work.  Proxying QMP commands doesn't seem
that easy, and I don't see much code being shared between the launcher
and QEMU; if the existing QEMU code is not suitable for Libvirt, it
wouldn't be suitable for a qemu.git launcher either.

Also, as you mentioned earlier, QEMU wants to keep its vocabulary
lower-level, and therefore the launcher's vocabulary would end up
diverging from QEMU.  Some example:

- QEMU wants a qemu-pr-helper socket path, the launcher would take care
of launching qemu-pr-helper itself

- QEMU wants the complete configuration on the migration destination,
the launcher might take care of sending it from the source?

At this point, you get something that looks very much like Libvirt and,
especially if you include live migration, it has to take into account
the same compatibility considerations as Libvirt.

Thanks,

Paolo

[PATCH] bsd-user: improve support for sparc syscall flags

[salvador]

From: Salvador Fandino 

Under sparc and sparc64, both NetBSD and OpenSSH use two bits of the
syscall number as flags. Until now, those bits where only supported
for sparc64 when emulating OpenBSD.

This patch extends support for syscall flags to the sparc architecture
and NetBSD emulation. It had allowed my to run simple NetBSD sparc
applications with qemu-sparc on a FreeBSD x64 machine.

The code supporting OpenBSD sparc and sparc64 emulation has been
refactored in order to make it simpler and similar to the new one for
NetBSD.

Signed-off-by: Salvador Fandino 
---
 bsd-user/main.c  | 66 +++-
 bsd-user/netbsd/syscall_nr.h | 52 
 2 files changed, 95 insertions(+), 23 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 770c2b267a..5706261f15 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -491,7 +491,7 @@ static void flush_windows(CPUSPARCState *env)
 void cpu_loop(CPUSPARCState *env)
 {
 CPUState *cs = env_cpu(env);
-int trapnr, ret, syscall_nr;
+int trapnr, ret, syscall_nr, syscall_flags;
 //target_siginfo_t info;
 
 while (1) {
@@ -511,21 +511,27 @@ void cpu_loop(CPUSPARCState *env)
 case 0x100:
 #endif
 syscall_nr = env->gregs[1];
-if (bsd_type == target_freebsd)
+if (bsd_type == target_freebsd) {
 ret = do_freebsd_syscall(env, syscall_nr,
  env->regwptr[0], env->regwptr[1],
  env->regwptr[2], env->regwptr[3],
  env->regwptr[4], env->regwptr[5], 0, 
0);
-else if (bsd_type == target_netbsd)
+} else if (bsd_type == target_netbsd) {
+syscall_flags = syscall_nr & (TARGET_NETBSD_SYSCALL_G7RFLAG |
+  TARGET_NETBSD_SYSCALL_G5RFLAG |
+  TARGET_NETBSD_SYSCALL_G2RFLAG);
+syscall_nr &= ~(TARGET_NETBSD_SYSCALL_G7RFLAG |
+TARGET_NETBSD_SYSCALL_G5RFLAG |
+TARGET_NETBSD_SYSCALL_G2RFLAG);
 ret = do_netbsd_syscall(env, syscall_nr,
 env->regwptr[0], env->regwptr[1],
 env->regwptr[2], env->regwptr[3],
 env->regwptr[4], env->regwptr[5]);
-else { //if (bsd_type == target_openbsd)
-#if defined(TARGET_SPARC64)
-syscall_nr &= ~(TARGET_OPENBSD_SYSCALL_G7RFLAG |
-TARGET_OPENBSD_SYSCALL_G2RFLAG);
-#endif
+} else { /* if (bsd_type == target_openbsd) */
+syscall_flags = syscall_nr & (TARGET_OPENBSD_SYSCALL_G2RFLAG |
+  TARGET_OPENBSD_SYSCALL_G7RFLAG);
+syscall_nr &= ~(TARGET_OPENBSD_SYSCALL_G2RFLAG |
+TARGET_OPENBSD_SYSCALL_G7RFLAG);
 ret = do_openbsd_syscall(env, syscall_nr,
  env->regwptr[0], env->regwptr[1],
  env->regwptr[2], env->regwptr[3],
@@ -547,23 +553,37 @@ void cpu_loop(CPUSPARCState *env)
 }
 env->regwptr[0] = ret;
 /* next instruction */
-#if defined(TARGET_SPARC64)
-if (bsd_type == target_openbsd &&
-env->gregs[1] & TARGET_OPENBSD_SYSCALL_G2RFLAG) {
-env->pc = env->gregs[2];
-env->npc = env->pc + 4;
-} else if (bsd_type == target_openbsd &&
-   env->gregs[1] & TARGET_OPENBSD_SYSCALL_G7RFLAG) {
-env->pc = env->gregs[7];
-env->npc = env->pc + 4;
-} else {
+if (bsd_type == target_openbsd) {
+switch (syscall_flags) {
+case 0:
+env->pc = env->npc;
+break;
+case TARGET_OPENBSD_SYSCALL_G7RFLAG:
+env->pc = env->gregs[7];
+break;
+default: /* G2 or G2|G7 */
+env->pc = env->gregs[2];
+break;
+}
+} else if (bsd_type == target_netbsd) {
+switch (syscall_flags) {
+case 0:
+env->pc = env->npc;
+break;
+case TARGET_NETBSD_SYSCALL_G7RFLAG:
+env->pc = env->gregs[7];
+break;
+case TARGET_NETBSD_SYSCALL_G5RFLAG:
+env->pc = env->gregs[5];
+break;
+case TARGET_NETBSD_SYSCALL_G2RFLAG:
+env->pc = env->gregs[2];
+break;
+}
+} else  {  /* if (bsd_type == target_freebsd) */
 

Re: Making QEMU easier for management tools and applications

[Paolo Bonzini]

On 20/01/20 10:55, Stefan Hajnoczi wrote:
>>
>> [1] https://qemu.readthedocs.io/en/latest/interop/live-block-operations.html
> John and I discussed async events in the past.  qmp-shell currently uses
> the input() built-in function.  If we modify it with a
> select(2)/poll(2)-style function that monitors both stdin and the QMP
> socket then it could print QMP events as soon as they are received.

I think it should be rewritten using async/await.  A simple example:

import asyncio
import sys
from concurrent.futures import ThreadPoolExecutor

async def ainput(prompt: str = ""):
with ThreadPoolExecutor(1, "ainput") as executor:
return (await asyncio.get_event_loop().run_in_executor(
executor, sys.stdin.readline
)).rstrip()

async def numbers():
i = 1
while True:
print(i)
i = i + 1
await asyncio.sleep(1)

async def main():
name = await ainput("What's your name? ")
print("Hello, {}!".format(name))

asyncio.get_event_loop().create_task(numbers())
asyncio.get_event_loop().run_until_complete(main())

This would be a great Summer of Code project.  Even an autocompletion
interface using readline should be possible.

Paolo

Re: [PATCH rc2 01/25] target/avr: Add outward facing interfaces and core CPU logic

[Aleksandar Markovic]

On Friday, January 24, 2020, Philippe Mathieu-Daudé  wrote:

> From: Michael Rolnik 
>
> This includes:
> - CPU data structures
> - object model classes and functions
> - migration functions
> - GDB hooks
>
>
I have an objection over this patch.

It contains many diverse logical units squashed into a patch, and therefore
is not in accordance to our submission giidelines.

I already outlined one possible way to split the patch into several ones in
one of prevoius reviews, but Michael did seem to be responsive.

If we accept this patch, we will be setting a bad precedent, that may
misled future platform contributors. additionally, this patch may be
singled out in our countribution guidelines as the example how not to do a
parch, which is probably not what Michael want to be exposed to.

Splitting patches is tedius, but overall not that difficult or time
consuming task.

Yours,
Aleksandar




> Co-developed-by: Michael Rolnik 
> Co-developed-by: Sarah Harris 
> Signed-off-by: Michael Rolnik 
> Signed-off-by: Sarah Harris 
> Signed-off-by: Michael Rolnik 
> Acked-by: Igor Mammedov 
> Tested-by: Philippe Mathieu-Daudé 
> Message-Id: <20200118191416.19934-2-mrol...@gmail.com>
> Signed-off-by: Richard Henderson 
> ---
>  target/avr/cpu-param.h |  37 ++
>  target/avr/cpu-qom.h   |  54 +++
>  target/avr/cpu.h   | 258 +
>  target/avr/cpu.c   | 826 +
>  target/avr/gdbstub.c   |  84 +
>  target/avr/machine.c   | 121 ++
>  gdb-xml/avr-cpu.xml|  49 +++
>  7 files changed, 1429 insertions(+)
>  create mode 100644 target/avr/cpu-param.h
>  create mode 100644 target/avr/cpu-qom.h
>  create mode 100644 target/avr/cpu.h
>  create mode 100644 target/avr/cpu.c
>  create mode 100644 target/avr/gdbstub.c
>  create mode 100644 target/avr/machine.c
>  create mode 100644 gdb-xml/avr-cpu.xml
>
> diff --git a/target/avr/cpu-param.h b/target/avr/cpu-param.h
> new file mode 100644
> index 00..0c29ce4223
> --- /dev/null
> +++ b/target/avr/cpu-param.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU AVR CPU
> + *
> + * Copyright (c) 2019 Michael Rolnik
> + *
> + * 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.1 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
> + * 
> + */
> +
> +#ifndef AVR_CPU_PARAM_H
> +#define AVR_CPU_PARAM_H
> +
> +#define TARGET_LONG_BITS 32
> +/*
> + * TARGET_PAGE_BITS cannot be more than 8 bits because
> + * 1.  all IO registers occupy [0x .. 0x00ff] address range, and they
> + * should be implemented as a device and not memory
> + * 2.  SRAM starts at the address 0x0100
> + */
> +#define TARGET_PAGE_BITS 8
> +#define TARGET_PHYS_ADDR_SPACE_BITS 24
> +#define TARGET_VIRT_ADDR_SPACE_BITS 24
> +#define NB_MMU_MODES 2
> +
> +
> +#endif
> diff --git a/target/avr/cpu-qom.h b/target/avr/cpu-qom.h
> new file mode 100644
> index 00..e28b58c897
> --- /dev/null
> +++ b/target/avr/cpu-qom.h
> @@ -0,0 +1,54 @@
> +/*
> + * QEMU AVR CPU
> + *
> + * Copyright (c) 2019 Michael Rolnik
> + *
> + * 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.1 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
> + * 
> + */
> +
> +#ifndef QEMU_AVR_QOM_H
> +#define QEMU_AVR_QOM_H
> +
> +#include "hw/core/cpu.h"
> +
> +#define TYPE_AVR_CPU "avr-cpu"
> +
> +#define AVR_CPU_CLASS(klass) \
> +OBJECT_CLASS_CHECK(AVRCPUClass, (klass), TYPE_AVR_CPU)
> +#define AVR_CPU(obj) \
> +OBJECT_CHECK(AVRCPU, (obj), TYPE_AVR_CPU)
> +#define AVR_CPU_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(AVRCPUClass, (obj), TYPE_AVR_CPU)
> +
> +/**
> + *  AVRCPUClass:
> + *  @parent_realize: The parent class' realize handler.
> + *  @parent_reset: The parent class' reset handler.
> + *  @vr: Version Register value.
> + *
> + *  A AVR CPU model.
> + */
> +typedef struct AVRCPUClass {
> +

Re: [PATCH v4 6/7] disas: mips: Add micromips R6 disassembler - infrastructure and 16-bit instructions

[Aleksandar Markovic]

On Saturday, January 25, 2020, Richard Henderson <
richard.hender...@linaro.org> wrote:

> On 1/24/20 1:38 PM, Aleksandar Markovic wrote:
> > On Friday, January 24, 2020, Richard Henderson <
> richard.hender...@linaro.org
> > > wrote:
> > The thing I'm concerned about here is any future maintenance of this
> file.  One
> > would surely prefer to edit the original decodetree input than this
> output.
> >
> >
> > Here is the deal: This dissasembler is meant to reflect the
>  documentation of a
> > particular ISA, and as the documentation largely stays constant (except
> adding
> > some insignificant errata), the disassembler will stay virtually
> constant, we
> > wouldn't like even to touch it, and we like it this way.
>
> No, this is neither right nor proper.
>
> To review the code in this form is significantly harder than in its
> decodetree
> form.  That is in fact the whole point of the decodetree form: otherwise
> we'd
> still be writing these sorts of parsers by hand.
>
> While there's no license on this new file (another problem), if as assumed
> this
> is GPL 2+, then you are in violation of the GPL.  From section 3:
>
>   # The source code for a work means the preferred form of
>   # the work for making modifications to it.
>
> You cannot with a straight face claim that the generated c is the preferred
> form for making modifications.
>
> Finally, suppose we improve decodetree.py, so that it produces code with
> which
> the compiler produces better code, for some metric of better.  We would
> want
> this disassembler to benefit as well.
>
>
I think you got some things upside-down. A tool developent and usage should
be driven by the needs of users of a tool, and not dictated by the author.
Users should be free to use the tool in any way they seem suitable,
including its modification.



> r~
>

Re: Making QEMU easier for management tools and applications

[Markus Armbruster]

Kevin Wolf  writes:

> Am 24.01.2020 um 11:27 hat Daniel P. Berrangé geschrieben:
>> On Fri, Jan 24, 2020 at 08:59:41AM +0100, Markus Armbruster wrote:
>> > John Snow  writes:
>> > 
>> > > On 1/23/20 2:01 PM, Daniel P. Berrangé wrote:
>> > >> So when configuring objects you'll always provide a JSON/YAML doc.
>> > >> They've got some clever stuff for updating objects where you can
>> > >> provide a JSON patch for only the bits which need changing.
>> > >> 
>> > >> When querying/listing objects by default it displays only a small
>> > >> subset of their config information in a human friendly-ish format.
>> > >> If you want to see everything then you ask for it in JSON/YAML
>> > >> format. There's also an expression language that lets you extract
>> > >> particular pieces of information based on requested properties,
>> > >> and you can filter the list of objects based on attributes and so
>> > >> on.
>> > >> 
>> > >> I think it is fair to say the structure of kubernetes object config
>> > >> is on a par with hierarchical complexity of QEMU. The lack of a simple
>> > >> human targetted data input format does not appear to have negatively
>> > >> impacted the adoption of Kubernetes. It is worth questioning why this
>> > >> is the case, while we feel the human CLI syntax for QEMU is so
>> > >> critically important to QEMU's future ?
>> > 
>> > I consider human CLI syntax for QEMU a mostly solved *design* problem:
>> > dotted keys.  It's an unsolved *implementation* problem: the CLI is a
>> > tangled mess of almost two decades' worth of ideas, and only (some of)
>> > the latest strands actually use dotted keys infrastructure.  The
>> > proposed solution is CLI QAPIfication.  Gives us configuration file(s)
>> > and introspection.
>> > 
>> > Dotted keys are merely yet another concrete syntax.  They're designed to
>> > satisfy the CLI requirements we have, which include a measure of
>> > compatibility to what's in the tangled mess.  They're reasonably usable
>> > for simple stuff, but complex stuff can be too verbose to be readable.
>> > They can't express all of the abstract syntax.  Tolerable, since they
>> > provide an escape to JSON.  I recommend programs use the JSON escape
>> > always.  Awkward for humans due to shell quoting.
>> 
>> I agree that the dotted key syntax is our chosen / solved design
>> for expressing JSON on the CLI. I would also say that, in retrospect,
>> this was a incorrect design decision that is one of the key things
>> responsible for QEMU having a bad reputation for complexity.
>
> I doubt this. Whenever I get a bug report with a command line created by
> libvirt, the command line is huge, but basically nothing in it uses
> dotted syntax. Yes, you may have cache.direct=on in it somewhere, but
> that's not actual nesting.
>
> The problem is the amount of options that is specified by management
> tools, and then humans are looking at it and feel it's way too complex.
>
> Command lines written by human users are usually much simpler because
> they just use QEMU's defaults instead of explicitly specifying
> everything.

Yes, machine-generated configuration is more verbose than what humans
produce.  Machines like it explicit.  It's simpler for them than relying
on defaults.  Bonus: immunity to changing defaults.

But no, verbosity is not the core problem, it merely aggravates the core
problem.  Complex configuration is much harder to read in a CLI syntax
than in a half-decent config file.  Yes, it's prohibitively harder just
for lengthy configurations.  That doesn't make it not harder for short
configurations.

>> We should simply never have tried to invent a way to map the full
>> hiearchy of JSON onto the CLI as the result will always be unpleasant.

It's what I had to do to secure a beach head for QAPI on the command
line coast.

>> The dotted notation is the most verbose way to do this type of
>> configuration, because of the string repetition it requires for
>> nested structures.
>
> True. I would have liked a different syntax that used some kind of
> brackets (at least optionally), but Markus didn't like adding another
> character that must be escaped.

Design thread:

Subject: Non-flat command line option argument syntax
Date: Thu, 02 Feb 2017 20:42:33 +0100
Message-ID: <87bmukmlau@dusky.pond.sub.org>
https://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg00555.html

A few quick quotes:

* On JSON vs. dotted keys in CLI: "both variants are basically
  illegible.  This is simply something that belongs into a config file
  rather than the command line.  In a config file, JSON would be a
  better choice."

* On dotted keys vs. structured values: "dotted keys are weird and ugly,
  but at least they don't add to the quoting mess.  Structured values
  look better, except when they do add to the quoting mess.  I'm having
  a hard time deciding which one I like less :)"

* Final verdict: "the whole non-flat command line argument design has
  been an exercise in 

Re: [PATCH rc2 21/25] hw/avr: Add some Arduino boards

[Joaquin de Andres]

On 1/24/20 1:51 AM, Philippe Mathieu-Daudé wrote:
> Arduino boards are build with AVR chipsets.
> Add some of the popular boards:
> 
> - Arduino Duemilanove
> - Arduino Uno
> - Arduino Mega
> 
> For more information:
>   https://www.arduino.cc/en/Main/Products
>   https://store.arduino.cc/arduino-genuino/most-popular
> 
> Reviewed-by: Igor Mammedov 
> Signed-off-by: Philippe Mathieu-Daudé 
> Message-Id: <20200120220107.17825-15-f4...@amsat.org>
> Signed-off-by: Richard Henderson 
> ---
> rc2:
> - Use avr_load_firmware (Aleksandar)
> - No default machine on AVR (Richard)
> - Add entry in MAINTAINERS (Michael is still the maintainer of hw/avr/)
> ---
>  hw/avr/arduino.c | 151 +++
>  MAINTAINERS  |   6 ++
>  hw/avr/Kconfig   |   4 ++
>  hw/avr/Makefile.objs |   1 +
>  4 files changed, 162 insertions(+)
>  create mode 100644 hw/avr/arduino.c
> 
> diff --git a/hw/avr/arduino.c b/hw/avr/arduino.c
> new file mode 100644
> index 00..2fb2e96ffe
> --- /dev/null
> +++ b/hw/avr/arduino.c
> @@ -0,0 +1,151 @@
> +/*
> + * QEMU Arduino boards
> + *
> + * Copyright (c) 2019 Philippe Mathieu-Daudé
> + *
> + * This work is licensed under the terms of the GNU GPLv2 or later.
> + * See the COPYING file in the top-level directory.
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +/* TODO: Implement the use of EXTRAM */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "hw/boards.h"
> +#include "atmel_atmega.h"
> +#include "boot.h"
> +
> +typedef struct ArduinoMachineState {
> +/*< private >*/
> +MachineState parent_obj;
> +/*< public >*/
> +AtmegaMcuState mcu;
> +} ArduinoMachineState;
> +
> +typedef struct ArduinoMachineClass {
> +/*< private >*/
> +MachineClass parent_class;
> +/*< public >*/
> +const char *mcu_type;
> +uint64_t xtal_hz;
> +} ArduinoMachineClass;
> +
> +#define TYPE_ARDUINO_MACHINE \
> +MACHINE_TYPE_NAME("arduino")
> +#define ARDUINO_MACHINE(obj) \
> +OBJECT_CHECK(ArduinoMachineState, (obj), TYPE_ARDUINO_MACHINE)
> +#define ARDUINO_MACHINE_CLASS(klass) \
> +OBJECT_CLASS_CHECK(ArduinoMachineClass, (klass), 
> TYPE_ARDUINO_MACHINE)
> +#define ARDUINO_MACHINE_GET_CLASS(obj) \
> +OBJECT_GET_CLASS(ArduinoMachineClass, (obj), TYPE_ARDUINO_MACHINE)
> +
> +static void arduino_machine_init(MachineState *machine)
> +{
> +ArduinoMachineClass *amc = ARDUINO_MACHINE_GET_CLASS(machine);
> +ArduinoMachineState *ams = ARDUINO_MACHINE(machine);
> +
> +sysbus_init_child_obj(OBJECT(machine), "mcu", >mcu, 
> sizeof(ams->mcu),
> +  amc->mcu_type);
> +object_property_set_uint(OBJECT(>mcu), amc->xtal_hz,
> + "xtal-frequency-hz", _abort);
> +object_property_set_bool(OBJECT(>mcu), true, "realized",
> + _abort);
> +
> +if (machine->firmware) {
> +if (!avr_load_firmware(>mcu.cpu, machine,
> +   >mcu.flash, machine->firmware)) {
> +exit(1);
> +}
> +}
> +}
> +
> +static void arduino_machine_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +
> +mc->init = arduino_machine_init;
> +mc->default_cpus = 1;
> +mc->min_cpus = mc->default_cpus;
> +mc->max_cpus = mc->default_cpus;
> +mc->no_floppy = 1;
> +mc->no_cdrom = 1;
> +mc->no_parallel = 1;
> +}
> +
> +static void arduino_duemilanove_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
> +
> +/* https://www.arduino.cc/en/Main/ArduinoBoardDuemilanove */
> +mc->desc= "Arduino Duemilanove (ATmega168)",
> +mc->alias   = "2009";
> +amc->mcu_type   = TYPE_ATMEGA168_MCU;
> +amc->xtal_hz= 16 * 1000 * 1000;
> +};

Hi! According to the page this board could be used with Atmega328 too.
Maybe you can define both?

The rest of this patch looks good to me, so:
Reviewed-by: Joaquin de Andres 

> +
> +static void arduino_uno_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
> +
> +/* https://store.arduino.cc/arduino-uno-rev3 */
> +mc->desc= "Arduino UNO (ATmega328P)";
> +mc->alias   = "uno";
> +amc->mcu_type   = TYPE_ATMEGA328_MCU;
> +amc->xtal_hz= 16 * 1000 * 1000;
> +};
> +
> +static void arduino_mega_class_init(ObjectClass *oc, void *data)
> +{
> +MachineClass *mc = MACHINE_CLASS(oc);
> +ArduinoMachineClass *amc = ARDUINO_MACHINE_CLASS(oc);
> +
> +/* https://www.arduino.cc/en/Main/ArduinoBoardMega */
> +mc->desc= "Arduino Mega (ATmega1280)";
> +mc->alias   = "mega";
> +amc->mcu_type   = TYPE_ATMEGA1280_MCU;
> +amc->xtal_hz= 16 * 1000 * 1000;
> +};
> +
> +static void 

Re: Integrating QOM into QAPI

[Paolo Bonzini]

On 25/01/20 05:44, Marc-André Lureau wrote:
> On 22/01/20 13:42, Marc-André Lureau wrote:
> > From the top of my mind, this is the pain point when trying to use
> GObject:
> > - static/inlined object, not supported by GObject, unlikely to ever be
> > - few users in qemu, transition possible.
> > - 64k limit of GObject, for some reason, unlikely to change but I will
> > take a look. Some users in qemu, code adaptation possible.
> > - dynamic properties, possible in GObject with hacks, but not
> > recommended and going to be deprecated from what I remember
> > - "array" properties - would need extra layer/tweaks for compatibility
> > - link properties - would need special handling
> > - different limitations for type names and properties names
> 
> The properties in general are very different between QOM and QAPI.  They
> have different limitations and features as Marc-André mentioned, but an
> especially important one is the integration with QAPI visitors.  This is
> what allows us to support -object and object-add with the same code, and
> is what separates QOM from GObject the most.
> 
> Maybe it would be possible to build an adapter, but having written in
> the past code that uses GType to do marshalling and unmarshalling, I'm
> not really fond of repeating the experience...
> 
> I agree it is one of the things that look very different from gobject.
> At the same time, I think defining conventions/types or interface to
> describe hierarchy isn't so difficult, and then adapting the visitors
> shouldn't be either.

Note that there are a few "structured" properties that export a QAPI
struct rather than a scalar.  Those would be a bit more complex.  Links
might also be tricky.

Another small difference that came to mind is the different object
liveness model; GObject is mostly plain old reference counting (plus the
"floating reference" idea), while in QOM reference counting is secondary
and lifetime is mostly dictated by the object tree and "unparenting".
This is more of a conceptual difference; it could be easily retrofitted
in GObject.

> I try to find a good reason qom was chosen over gobject, and I can't
> find it.

The main reasons were integration with QAPI, and the object tree.
Though everything I say here is a kind of reverse engineering of
Anthony's brain because there aren't really any design documents besides
what's in include/qom/object.h (and he overlooked some aspects, for
example "unparent" was introduced a few months later).

Overall I don't think there would be much benefit in reusing GObject.
It's about 3k lines of code, quite a few would stay (those implementing
the tree) and a bunch more would have to be rewritten.  I don't think
we'd have any use for most of the features that QOM lacks, such as
signals.  Also, QOM is quite well documented and we should include its
documentation in docs/devel instead.

That said, coccinell-ing efforts on QOM code are definitely a good idea
since it's quite mature now and we know better what we need.  And there
are practical advantages too, it's not just code cleanups---see how your
series shows the default value of the properties in -device foo,help.

Paolo

Re: [RFC] migration: Remove old compression code

[Markus Armbruster]

Cc'ing like David did, plus Eric and Mike for additional QAPI expertise.

Juan Quintela  writes:

> Hi
>
> There are several problems with the old compression code:
> * it compress one page at a time (i.e. 4KB)
>
> * it has to copy all the page to the compression thread, the
>   compression thread compress it, it copies back the compressed data
>   to the main thread and it sends it there (equivalent for decompress).
>
> * It is really slow (both the setup of it, and the compression of each page)
>
> You can see that in the previous series, I added compression to multifd, its 
> advantages are:
> * We compress "chunks" of 128 pages at a time
> * We don't copy anything, we just compress everything that is sent through 
> the channel
> * We don't send data over the main channel, so we have less overhead
> * I added zstd support, that
>   o  Compress much more
>   o  It is way faster
>
> * Using the migration-test values (best option for compressors, as we
>   only change one byte on each page):
>   * zstd is around the same latency than no compression (i.e. it takes the 
> same time)
>   * zlib needs 2500 bytes to send 128 pages compressed (remember 1
> byte changed in each page, rest are zeros)
>   * zstd needs 52 bytes for the same packets (yes, I checked several
> times, it appears that its use of dictionaries is good here)
>
> This is an RFC, you will ask why?
> There are several questions:
> * How to proceed to remove this?
>- Deprecate it into the future?
>  Has the advantage that customers can continue using it.
>  But it is not so stable to use it.
>- Remove it now?
>  Has the advantage of much less code, but 
> * Statistics:
>   Several of the statistics are already only printed when compression is
>   being used. My understanding is that we ase safe removing this ones.
> * But whan about the migration_parameters?  Should we leave them
>   alone, and just don't care about them? Should we remove them?
> * What to do with migration.json changes?  I will apprecciate comments here.
>
> Thanks, Juan.
>
> PD.  You can see from the previous series that putting compression on
>  top of multifd + splitting the code in different files + making it 
> optional:
>  around 1040 new lines of code.
>  Removing this ~ 960 lines of code.
>
> Signed-off-by: Juan Quintela 
[...]

Let's have a look at the changes to the stable external interface.

> diff --git a/qapi/migration.json b/qapi/migration.json
> index 65db85970e..5477d4d20b 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -79,27 +79,6 @@
> 'cache-miss': 'int', 'cache-miss-rate': 'number',
> 'overflow': 'int' } }
>  
> -##
> -# @CompressionStats:
> -#
> -# Detailed migration compression statistics
> -#
> -# @pages: amount of pages compressed and transferred to the target VM
> -#
> -# @busy: count of times that no free thread was available to compress data
> -#
> -# @busy-rate: rate of thread busy
> -#
> -# @compressed-size: amount of bytes after compression
> -#
> -# @compression-rate: rate of compressed size
> -#
> -# Since: 3.1
> -##
> -{ 'struct': 'CompressionStats',
> -  'data': {'pages': 'int', 'busy': 'int', 'busy-rate': 'number',
> -'compressed-size': 'int', 'compression-rate': 'number' } }
> -

Only user is MigrationInfo, which is next.

>  ##
>  # @MigrationStatus:
>  #
> @@ -200,9 +179,6 @@
   ##
   # @MigrationInfo:
[...]
>  #   only present when the postcopy-blocktime migration capability
>  #   is enabled. (Since 3.0)
>  #
> -# @compression: migration compression statistics, only returned if 
> compression
> -#   feature is on and status is 'active' or 'completed' (Since 3.1)
> -#
>  # @socket-address: Only used for tcp, to know what the real port is (Since 
> 4.0)
>  #
>  # Since: 0.14.0
> @@ -219,7 +195,6 @@
> '*error-desc': 'str',
> '*postcopy-blocktime' : 'uint32',
> '*postcopy-vcpu-blocktime': ['uint32'],
> -   '*compression': 'CompressionStats',
> '*socket-address': ['SocketAddress'] } }

MigrationInfo is returned by  query-migrate.  No other users.

Doc comment gives us wiggle room: "only returned if compression feature
is on".  Can it be on after this patch?

If no, the member can go without breaking query-migrate compatibility.

query-qmp-schema shows the change, though.  Could conceivably affect
users, but it seems unlikely.

Eric, Mike, we might want to grant ourselves explicit license to change
query-qmp-schema in such ways, by having qapi-code-gen.txt state
optional members may be removed when they can't be present anymore.
What do you think?

Alternatively, keep the member, hardcode value to whatever is returned
before the patch when compression is off, deprecate, remove after grace
period.  A bit more work, but safer.  Worthwhile?  Not for me to decide.

>  
>  ##
> @@ -364,14 +339,6 @@
   ##
   # @MigrationCapability:
[...]
>  #  to enable the