[Qemu-devel] [PATCH v2 0/2] Use SDL to create an OpenGL ES context for virglrenderer.

[Elie Tournier]

Hello everyone,

I submitted the v1 of this series before kraxel's display system rework.

Currently, virglrenderer [1] support OpenGL ES 2.0 on the guest side
and OpenGL ES 3.0 on the host side.
Thanks to this work, we are able to run QEMU on system that only support OpenGL 
ES.

The support of OpenGL ES 3.0 on the guest is limited.
We are working on it, so stay tune!
For the most curious of you, the development branch is available on
our gitlab instance [2].

In order to use this feature in QEMU, we need to create an OpenGL ES context.
This is possible thanks to the following option `-display sdl,gl=es`.

Have a nice day,
Elie

[1] https://cgit.freedesktop.org/virglrenderer
[2] https://gitlab.collabora.com/virgl-es/virglrenderer-gles/tree/hacks

Elie Tournier (2):
  qapi: Parameter gl of DisplayType now accept an enum
  sdl: Allow OpenGL ES context creation

 include/ui/sdl2.h |  1 +
 qapi/ui.json  | 21 -
 qemu-options.hx   |  2 +-
 ui/sdl2-gl.c  | 17 +++--
 ui/sdl2.c |  1 +
 vl.c  | 14 +-
 6 files changed, 47 insertions(+), 9 deletions(-)

-- 
2.17.0

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 11:55, Daniel P. Berrangé wrote:
> On Tue, Apr 10, 2018 at 11:51:31AM +0200, Gerd Hoffmann wrote:
 Hmm, I'm wondering whenever it is useful to model things this way.  It's
 not like you can actually configure things for -bios seabios.rom or
 -kernel uboot.elf.  Only pflash allows to actually configure things, and
 there are not that many useful combinations.  The code needs
 Read+Execute.  Allowing Write could be useful in theory, to allow the
 guest doing firmware updates.  But I think nobody actually does that, so
 in practice it is fixed.  The varstore can have different permissions,
 but it's only two useful combinations.  Either allow access
 unconditionally, or allow access in secure contect (aka smm) only.
>>>
>>> (I hope I understand your point right:)
>>>
>>> I'm also fine if we simply define a fixed (but extensible) set of
>>> mapping methods, basically a new enum type, that simply tells libvirtd
>>> what this firmware *is*. IOW, directly reference a mapping method we
>>> know libvirt implements, rather than give vague hints.
>>>
>>> This could repurpose SystemFirmwareType, but it should become more
>>> detailed. I'm thinking like:
>>> - ovmf: split files without requiring SMM
>>> - ovmf_smm: split files with SMM requirement
>>> - seabios: exactly that
>>> - ... other things others suggest.
>>
>> I wouldn't name them by firmware, that is misleading.  Basically we have
>> three cases:
>>
>>   (1) single firmware image (seabios, OVMF.fd, ...).
>>   (2) split firmware image (OVMF_{CODE,VARS}.fd), where vars can be
>>   writable unconditinally.
>>   (3) split firmware image, where access to vars should be restricted
>>   to smm mode.
>>
>> (2) + (3) requires pflash.  (1) works with both pflash and -bios.
> 
> A big chunk of the data in the schema looks specific to the pflash
> case, but this is not expressed except in the docs. Most of the time
> with QAPI when we have data that is only relevant in certain types,
> we use a discriminated union to describe it. It feels like a unioon
> approach could be better suited to this

I used a discriminated union specifically for pflash options in RFCv0,
which I didn't post. I felt that it wasn't flexible enough. :)

Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 11:34, Daniel P. Berrangé wrote:
> On Tue, Apr 10, 2018 at 11:16:01AM +0200, Laszlo Ersek wrote:
>> On 04/10/18 08:27, Gerd Hoffmann wrote:
>>>   Hi,
>>>
 - I considered adding wildcards (say, blacklist "all" i440fx machtypes,
 present and future, for SMM-requiring OVMF builds), but then you get
 into version sorting and similar mess. I considered fnmatch() --
 basically simple ? and * wildcards -- but that's not expressive enough.
>>>
>>> I'd suggest whitelist with wildcards.  So the smm builds would get
>>> "pc-q35-*".
>>>
>>> libvirt knows about aliases, so it should be able to handle the "q35"
>>> shortcut like "pc-q35-${latest}".
>>>
>>> Or do you see another issue?
>>
>> Well, one issue I see is version sorting; I should say "Q35 but no
>> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4".
>>
>> Anyway (also asking for Thomas's input here): if we run with your idea
>> to refer to exact mapping methods / firmware *implementation* types that
>> we know libvirt implements / supports as a "white box", do we still deem
>> machine type identification necessary? Because, libvirt already knows
>> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just
>> have to make a *reference* to that knowledge in the JSON file.
> 
> BTW, that's not quite correct - when libvirt handles the "smm" arg it
> checks if machine type == q35, and  QEMU version >= 2.4.
> 
> It is *not* checking the version of the machine type. ie it will happily
> use smm with  pc-q35-2.0, as long as QEMU version is 2.4. Perhaps this is
> not quite right,

(it's not)

> but we don't try to parse the version number out of the
> machine type, because we can't assume a specific format for the machine
> type version part. eg version can be "2.4", or it can be "rhel-7.0.0"
> or something else again on Ubuntu.

Indeed, that's exactly why I'm troubled about expressing a "minimum"
machine type version.

> 
> IMHO it would be valid to just keep life simple and only record the base
> machine type name that can use the firmware ie "pc", "q35", and ignore
> the fact that in some cases the firmware might require a specific version
> of the machine type.

Esp. with regard to SMM, there have been quite big jumps in usability /
stability across Q35 machtype versions. But, if it works for you, it
works for me.

(I double-checked Thomas's recent example about U-Boot, and he mentioned
the "ppce500" and "sam460ex" machine types, not machine type versions.)

Thanks,
Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 11:26, Thomas Huth wrote:
> On 10.04.2018 11:16, Laszlo Ersek wrote:
>> On 04/10/18 08:27, Gerd Hoffmann wrote:
>>>   Hi,
>>>
 - I considered adding wildcards (say, blacklist "all" i440fx machtypes,
 present and future, for SMM-requiring OVMF builds), but then you get
 into version sorting and similar mess. I considered fnmatch() --
 basically simple ? and * wildcards -- but that's not expressive enough.
>>>
>>> I'd suggest whitelist with wildcards.  So the smm builds would get
>>> "pc-q35-*".
>>>
>>> libvirt knows about aliases, so it should be able to handle the "q35"
>>> shortcut like "pc-q35-${latest}".
>>>
>>> Or do you see another issue?
>>
>> Well, one issue I see is version sorting; I should say "Q35 but no
>> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4".
>>
>> Anyway (also asking for Thomas's input here): if we run with your idea
>> to refer to exact mapping methods / firmware *implementation* types that
>> we know libvirt implements / supports as a "white box", do we still deem
>> machine type identification necessary? Because, libvirt already knows
>> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just
>> have to make a *reference* to that knowledge in the JSON file.
> 
> I think you really need a way to specify the machine there. Latest
> example from QEMU 2.12: We've now got two uboot binaries in the tree,
> pc-bios/u-boot.e500 and pc-bios/u-boot-sam460-20100605.bin. Both are
> uboot, both are for ppc, but u-boot.e500 only works with the "ppce500"
> machine and the other one only works with the "sam460ex" machine. How
> would you teach libvirt such a relationship without an explicit machine
> type identification field there?

My idea was to assign different "map method" enumeration constants to
them, and libvirtd would associate those with different machtype
requirements.

But, as Daniel explained, we cannot reference libvirtd features, so I
agree we have to express machine types somehow. I don't know how though.

For example, can we take it for granted that a machtype version number,
if it exists in the first place, always follows the last hyphen in the
machtype identifier? Say, "virt-2.11" / aarch64 conforms, "pc-q35-2.4"
and "pc-i440fx-2.12" conform too. But, is that a guarantee that covers
all arches and all boards?

Because, I don't think:
- machine-type-family = q35
- minimum-machine-type = 2.4

will work. Will every application that manages QEMU learn that "q35" is
short-hand for "pc-q35-XXX", and (again) that 2.12 sorts *after* 2.4?

Thanks,
Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 12:48:28PM +0100, Peter Maydell wrote:
> On 10 April 2018 at 12:34, Daniel P. Berrangé  wrote:
> > On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote:
> >
> >> Please go through the rest of the emails in this thread, and advise:
> >> - if the firmware descriptor schema may perhaps live in the libvirt tree,
> >> - accordingly, if the schema could be expressed as an XSD (and firmware
> >> packages should provide the descriptor documents as XMLs)
> >> - if you agree that the descriptor document can uniquely reference
> >> mapping methods implemented in libvirtd by simple enum constants (with
> >> necessary parameters provided).
> >
> > No to all three. This is the responsibility of QEMU to define, because
> > this information is relevant to anything managing QEMU not just libvirt.
> 
> (Please consider this as more of a grenade lobbed into the conversation
> rather than a carefully thought out proposal...)
> 
> My inclination is to say that it's not really the responsibility
> of QEMU to define either -- we provide emulated models of hardware,
> and it's up to the user or the management layer or the provider
> of the firmware to specify what guest code they want to run and how
> it needs to run on that emulated hardware...
> 
> Where the QEMU upstream itself is providing firmware blobs
> (in tarballs etc) it's probably our job to specify how they work,
> but if the firmware is compiled and provided by the distro (as eg happens
> for Arm UEFI blobs at the moment) then I don't see how upstream QEMU
> can reliably define how that firmware needs to be loaded.

QEMU should not provide the actual metadata files themselves - it just
has to the define the file format. The relevant firmware upstreams and
or distros, can provide the metadata files for the blobs they choose
to ship for use with QEMU.

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

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 11:32, Thomas Huth wrote:
> On 10.04.2018 11:22, Laszlo Ersek wrote:
>> On 04/10/18 09:33, Thomas Huth wrote:
> [...]
>>> Alternatively, what about providing some kind of "alias" or "nickname"
>>> setting here, too? So the EDK2 builds would get
>>> SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example.
>>
>> I hope I understand you right -- I think your suggestion ties in with my
>> other email I just sent in this thread. So, we could tell libvirtd,
>> "this firmware is of type 'UEFI', and you must use the 'ovmf_smm'
>> mapping method to run it, with this file or that file as varstore template".
>>
>> We could even describe the parameters for this or that mapping method
>> structurally in the schema (in a discriminated union in QAPI JSON, or in
>> an XSD choice element). For example, "ovmf" and "ovmf_smm" would both
>> take "OvmfSplitFileOptions" -- a list of single varstore template files
>> with feature enum contants attached  --, while "SeaBiosOptions" would be
>> an empty structure.
> 
> Sorry, I've got no clue about ovmf_smm and the other things you've
> mentioned here ;-)
> 
>> I feel the key question here is whether we are allowed to directly
>> reference a mapping method we know libvirt implements. If we are, that
>> makes things a lot clearer (and easier, I should hope).
> 
> Key question is maybe rather: Do you want to design / implement
> something that is libvirt-only here, or rather something generic that
> could also be used for other upper layer tools that do not use libvirt?
> (... and looks like Daniel just had the same comment in another mail in
> this thread ...)

Yeah, we can't target libvirtd as the sole consumer.

Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 01:44:13PM +0200, Laszlo Ersek wrote:
> On 04/10/18 13:34, Daniel P. Berrangé wrote:
> > On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote:
> > 
> >> Please go through the rest of the emails in this thread, and advise:
> >> - if the firmware descriptor schema may perhaps live in the libvirt tree,
> >> - accordingly, if the schema could be expressed as an XSD (and firmware
> >> packages should provide the descriptor documents as XMLs)
> >> - if you agree that the descriptor document can uniquely reference
> >> mapping methods implemented in libvirtd by simple enum constants (with
> >> necessary parameters provided).
> > 
> > No to all three. This is the responsibility of QEMU to define, because
> > this information is relevant to anything managing QEMU not just libvirt.
> 
> In that case, how do you suggest we describe the QEMU command line
> options that are (a) necessary, (b) "discoverable" to the management
> application? Should we provide verbatim command line fragments (option
> templates)? Is this feature meant to replace the cmdline generation
> logic that already exists in libvirtd?

Each part of the schema should have docs describing what CLI args it
corresponds to. eg document that when device=memory, corresponds
to -bios, that device=flash, corresponds to -drive if=pflash, etc

We've not trying to replace the cmdline generator in libvirt. We just
want to know that when we see a particular field present in the schema,
that it corresponds to a particular cli arg.

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

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Peter Maydell]

On 10 April 2018 at 12:34, Daniel P. Berrangé  wrote:
> On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote:
>
>> Please go through the rest of the emails in this thread, and advise:
>> - if the firmware descriptor schema may perhaps live in the libvirt tree,
>> - accordingly, if the schema could be expressed as an XSD (and firmware
>> packages should provide the descriptor documents as XMLs)
>> - if you agree that the descriptor document can uniquely reference
>> mapping methods implemented in libvirtd by simple enum constants (with
>> necessary parameters provided).
>
> No to all three. This is the responsibility of QEMU to define, because
> this information is relevant to anything managing QEMU not just libvirt.

(Please consider this as more of a grenade lobbed into the conversation
rather than a carefully thought out proposal...)

My inclination is to say that it's not really the responsibility
of QEMU to define either -- we provide emulated models of hardware,
and it's up to the user or the management layer or the provider
of the firmware to specify what guest code they want to run and how
it needs to run on that emulated hardware...

Where the QEMU upstream itself is providing firmware blobs
(in tarballs etc) it's probably our job to specify how they work,
but if the firmware is compiled and provided by the distro (as eg happens
for Arm UEFI blobs at the moment) then I don't see how upstream QEMU
can reliably define how that firmware needs to be loaded.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/1] linux-user: fix file offset for preadv/pwritev

[Peter Maydell]

On 10 April 2018 at 03:17, Max Filippov  wrote:
> Hi Peter,
>
> please pull the following linux-user fix for QEMU-2.12.
>
> The following changes since commit 9abfc88af3ffd3b33c7fab4471da86462ee71d95:
>
>   Merge remote-tracking branch 'remotes/xtensa/tags/20180402-xtensa' into 
> staging (2018-04-03 19:02:46 +0100)
>
> are available in the git repository at:
>
>   git://github.com/OSLL/qemu-xtensa.git tags/20180409-xtensa
>
> for you to fetch changes up to 9ac225171c9d6b2a1cba35a94ae7eeaa0106cf7d:
>
>   linux-user: fix preadv/pwritev offsets (2018-04-09 18:57:49 -0700)
>
> 
> Fix file offset for preadv/pwritev linux-user syscalls.
>
> 
> Max Filippov (1):
>   linux-user: fix preadv/pwritev offsets

Applied, thanks.

-- PMM

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 12:09, Paolo Bonzini wrote:
> On 10/04/2018 11:23, Daniel P. Berrangé wrote:
>>> And, really, this seems to reinforce my point that the schema should
>>> live in the libvirtd tree, not in the QEMU tree. In that case, perhaps
>>> it would be a better fit to work with an XSD, and firmware packages
>>> should install XML files? Personally I'm a lot more attracted to
>>> XML/XSD; I think the tooling is better too. I just don't see how QEMU is
>>> involved.
>>
>> This is defining a set of metadata that is required to use various firmware
>> files in combination with QEMU, along with defining a mapping to QEMU command
>> line arguments and/or features. Essentially, while I wish everyone used
>> libvirt, libvirt is not the only thing that manages QEMU. This information is
>> relevant to anyone managing QEMU, so it doesn't belong in libvirt's realm,
>> it is clear QEMU is best placed to declare this information.
> 
> QEMU is best placed to _standardize_ how to provide this information
> (and where in the file system to place it), but really it's up to
> firmware packages to provide it.
> 
> We can of course define the schema in QAPI terms for ease of validation
> (machine-readable specs are nice to have), but really this should just
> be a file in docs/interop/.  No code is needed in QEMU.

OK -- while we're figuring out the schema, I guess I'll keep posting
RFCs that change source code / json, but finally we can move it to
docs/interop.

Thanks!
Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 13:34, Daniel P. Berrangé wrote:
> On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote:
> 
>> Please go through the rest of the emails in this thread, and advise:
>> - if the firmware descriptor schema may perhaps live in the libvirt tree,
>> - accordingly, if the schema could be expressed as an XSD (and firmware
>> packages should provide the descriptor documents as XMLs)
>> - if you agree that the descriptor document can uniquely reference
>> mapping methods implemented in libvirtd by simple enum constants (with
>> necessary parameters provided).
> 
> No to all three. This is the responsibility of QEMU to define, because
> this information is relevant to anything managing QEMU not just libvirt.

In that case, how do you suggest we describe the QEMU command line
options that are (a) necessary, (b) "discoverable" to the management
application? Should we provide verbatim command line fragments (option
templates)? Is this feature meant to replace the cmdline generation
logic that already exists in libvirtd?

Thanks
Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 11:05, Daniel P. Berrangé wrote:
> On Mon, Apr 09, 2018 at 06:34:41PM +0200, Laszlo Ersek wrote:
>> On 04/09/18 09:26, Thomas Huth wrote:
>>>  Hi Laszlo,
>>>
>>> On 07.04.2018 02:01, Laszlo Ersek wrote:
 Add a schema that describes the properties of virtual machine
 firmware.

 Each firmware executable installed on a host system should come
 with a JSON file that conforms to this schema, and informs the
 management applications about the firmware's properties.

 In addition, a configuration directory with symlinks to the JSON
 files should exist, with the symlinks carefully named to reflect a
 priority order. Management applications can then search this
 directory in priority order for the first firmware executable that
 satisfies their search criteria. The found JSON file provides the
 management layer with domain configuration bits that are required
 to run the firmware binary.
>>> [...]
 +##
 +# @FirmwareDevice:
 +#
 +# Defines the device types that a firmware file can be mapped into.
 +#
 +# @memory: The firmware file is to be mapped into memory.
 +#
 +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
 +#  similar to @memory but may imply additional processing that is
 +#  specific to the target architecture.
 +#
 +# @flash: The firmware file is to be mapped into a pflash chip.
 +#
 +# Since: 2.13
 +##
 +{ 'enum' : 'FirmwareDevice',
 +  'data' : [ 'memory', 'kernel', 'flash' ] }
>>>
>>> This is not fully clear to me... what is this exactly good for? Is
>>> this a way to say how the firmware should be loaded, i.e. via
>>> "-bios", "-kernel" or "-pflash" parameter? If so, the term "memory"
>>> is quite misleading since files that are loaded via -bios can also
>>> end up in an emulated ROM chip.
>>
>> I threw in "-kernel" because, although it also (usually?) means
>> "memory", I expected people would want it separate.
>
> What platform / scenario actually uses -kernel to load firmware. If
> you have loaded firmware using -kernel, how do you then load the
> actual kernel ?

AAVMF has a build called "ArmVirtQemuKernel" where the firmware is
loaded with the -kernel switch.

commit 8de84d4242215252af9d2afecd45e2419689ee5f
Author: Ard Biesheuvel 
Date:   Fri Feb 5 14:57:57 2016 +0100

ArmVirtPkg: implement ArmVirtQemuKernel

This implements a version of ArmVirtQemu that does not execute in place from
emulated NOR flash, but implements the Linux kernel boot protocol, and 
executes
from DRAM instead. This allows UEFI to be loaded as a payload by a previous
bootloader stage such as ARM Trusted Firmware/OP-TEE.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
Acked-by: Laszlo Ersek 

My understanding is that in this scenario you cannot use -kernel for
loading a Linux kernel; instead you have to boot the Linux OS off of
some other media (CD-ROM, disk, network...) Personally I never use this
AAVMF build, but I know it exists and Ard uses it at least occasionally.

Thanks,
Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Gerd Hoffmann]

  Hi,

> It occurs to me that we are actually over-thinking things, by making it
> possible to list a choice of vars files per firmware. We could remove this
> special case by just having separate tpo level firmware entries and a main
> feature flag to say if it is enrolled or not - see below example

That would also make it easier to implement something like ...

qemu -firmware json=/path/to/firmware/spec.json

... because you simply have two files for the enrolled/non-enrolled
variants.

cheers,
  Gerd

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 01:27:18PM +0200, Laszlo Ersek wrote:

> Please go through the rest of the emails in this thread, and advise:
> - if the firmware descriptor schema may perhaps live in the libvirt tree,
> - accordingly, if the schema could be expressed as an XSD (and firmware
> packages should provide the descriptor documents as XMLs)
> - if you agree that the descriptor document can uniquely reference
> mapping methods implemented in libvirtd by simple enum constants (with
> necessary parameters provided).

No to all three. This is the responsibility of QEMU to define, because
this information is relevant to anything managing QEMU not just libvirt.


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

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 11:18, Daniel P. Berrangé wrote:
> On Mon, Apr 09, 2018 at 07:57:54PM +0200, Laszlo Ersek wrote:
>> On 04/09/18 10:49, Daniel P. Berrangé wrote:
>>> On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote:
 Add a schema that describes the properties of virtual machine firmware.

 Each firmware executable installed on a host system should come with a
 JSON file that conforms to this schema, and informs the management
 applications about the firmware's properties.

 In addition, a configuration directory with symlinks to the JSON files
 should exist, with the symlinks carefully named to reflect a priority
 order. Management applications can then search this directory in priority
 order for the first firmware executable that satisfies their search
 criteria. The found JSON file provides the management layer with domain
 configuration bits that are required to run the firmware binary.

 Cc: "Daniel P. Berrange" 
 Cc: Alexander Graf 
 Cc: Ard Biesheuvel 
 Cc: David Gibson 
 Cc: Eric Blake 
 Cc: Gary Ching-Pang Lin 
 Cc: Gerd Hoffmann 
 Cc: Kashyap Chamarthy 
 Cc: Markus Armbruster 
 Cc: Michael Roth 
 Cc: Michal Privoznik 
 Cc: Peter Krempa 
 Cc: Peter Maydell 
 Cc: Thomas Huth 
 Signed-off-by: Laszlo Ersek 
 ---

 Notes:
 Folks on the CC list, please try to see if the suggested schema is
 flexible enough to describe the virtual firmware(s) that you are
 familiar with. Thanks!

  Makefile  |   9 ++
  Makefile.objs |   4 +
  qapi/firmware.json| 343 
 ++
  qapi/qapi-schema.json |   1 +
  qmp.c |   5 +
  .gitignore|   4 +
  6 files changed, 366 insertions(+)
  create mode 100644 qapi/firmware.json

>>>
 diff --git a/qapi/firmware.json b/qapi/firmware.json
 new file mode 100644
 index ..f267240f44dd
 --- /dev/null
 +++ b/qapi/firmware.json
 @@ -0,0 +1,343 @@
 +# -*- Mode: Python -*-
 +
 +##
 +# = Firmware
 +##
 +
 +##
 +# @FirmwareDevice:
 +#
 +# Defines the device types that a firmware file can be mapped into.
 +#
 +# @memory: The firmware file is to be mapped into memory.
 +#
 +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
 +#  similar to @memory but may imply additional processing that is
 +#  specific to the target architecture.
 +#
 +# @flash: The firmware file is to be mapped into a pflash chip.
 +#
 +# Since: 2.13
 +##
 +{ 'enum' : 'FirmwareDevice',
 +  'data' : [ 'memory', 'kernel', 'flash' ] }
 +
 +##
 +# @FirmwareAccess:
 +#
 +# Defines the possible permissions for a given access mode to a device 
 that
 +# maps a firmware file.
 +#
 +# @denied: The access is denied.
 +#
 +# @permitted: The access is permitted.
 +#
 +# @restricted-to-secure-context: The access is permitted for guest code 
 that
 +#runs in a secure context; otherwise the 
 access
 +#is denied. The definition of "secure 
 context"
 +#is specific to the target architecture.
 +#
 +# Since: 2.13
 +##
 +{ 'enum' : 'FirmwareAccess',
 +  'data' : [ 'denied', 'permitted', 'restricted-to-secure-context' ] }
>>>
>>> I'm not really understanding the purpose of this - what does it map to
>>> on the command line ?
>>
>> That's difficult to answer generally, because -bios and -kernel have
>> different meanings per board type. So I didn't aim at command line
>> switches here; instead I tried to capture where and how the firmware
>> wants to "end up" in the virtual hardware. How that maps to a particular
>> board is a separate question.
> 
> I tend to think that defining a mapping to command line arguments is a key
> feature that this should cover. Even if there variations across boards, QEMU
> still has a small finite set of approaches to configure firmware, so it does
> not feel unreasonable to define what they are and how they map to thes 
> firmware
> files.

I agree, now that I've read about Gerd's similar argument.

There I made the suggestion that the schema could define enum constants
(mapping identifiers) that directly refer to libvirtd's existing logic
to map various firmware types.

> Your FirmwareDevice enum above with "memory", "kernel" and "flash" has
> pretty much suggested the -bios, -kernel or -drive if=flash args anway
> 
>> So, the schema intends to describe the mapping that the firmware expects
>> from the board. How that is implemented on the QEMU command line is left
>> as an exercise to ... libvirtd. :)
> 
> I think this is pretty unhelpful. Essentially that is saying here

Re: [Qemu-devel] [PATCH] configure: don't warn SDL abi if disabled

[Peter Xu]

On Tue, Apr 10, 2018 at 04:49:50PM +0800, Fam Zheng wrote:

[...]

> We have had
> 
> function_name()
> {
> ...
> 
> and
> 
> function_name() {
> ...
> 
> and
> 
> function_name () {
> ...
> 
> finally you invent the last in the family:
> 
> function_name ()
> {
> 
> :)

I was careful about whether we were using the "function" keyword, but
still that seems to be not enough... :-)

[...]

> Reviewed-by: Fam Zheng 

Thanks!

-- 
Peter Xu

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 11:20:33AM +0100, Daniel P. Berrangé wrote:
> On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote:
> > Add a schema that describes the properties of virtual machine firmware.
> > 
> > Each firmware executable installed on a host system should come with a
> > JSON file that conforms to this schema, and informs the management
> > applications about the firmware's properties.
> > 
> > In addition, a configuration directory with symlinks to the JSON files
> > should exist, with the symlinks carefully named to reflect a priority
> > order. Management applications can then search this directory in priority
> > order for the first firmware executable that satisfies their search
> > criteria. The found JSON file provides the management layer with domain
> > configuration bits that are required to run the firmware binary.
> > 
> 
> > diff --git a/qapi/firmware.json b/qapi/firmware.json
> > new file mode 100644
> > index ..f267240f44dd
> > --- /dev/null
> > +++ b/qapi/firmware.json
> 
> [snip]
> 
> > +{ 'struct' : 'SystemFirmware',
> > +  'data'   : { 'executable' : 'FirmwareFile',
> > +   'type'   : 'SystemFirmwareType',
> > +   'targets': [ 'str' ],
> > +   'sysfw-map'  : 'FirmwareMapping',
> > +   '*nvram-slots'   : [ 'NVRAMSlot' ],
> > +   '*supports-uefi-secure-boot' : 'bool',
> > +   '*supports-amd-sev'  : 'bool',
> > +   '*supports-acpi-s3'  : 'bool',
> > +   '*supports-acpi-s4'  : 'bool' } }
> 
> Elsewhere in the thread I mentioned that I think we should try to use a
> union approach to isolate which information is relevant to "flash" loader
> format and which is relevant to "memory" and "kernel". To try to illustrate
> what I mean by that I've knocked up an alternative structure. I also
> incorporated the points about features & target/machine types.  I've left
> out the read/write/etc fields, but they could be put back in at the
> relevant position
> 
> 
> { 'enum' : 'SystemFirmwareType',
>   'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
> 
> { 'enum' : 'SystemFirmwareDevice',
>   'data' : [ 'memory', 'kernel', 'flash' ] }
> 
> { 'enum' : 'SystemFirmwareArchitecture',
>   'data':  ['x86_64', 'i386', ..etc.. ] }
>   
> { 'enum' : 'SystemFirmwareFeature',
>   'data': ['acpi-s3', 'acpi-s5', 'secure-boot', 'amd-sev' ]}
> 
> 
> ## Struct(s) for device==memory
> 
> { 'struct': 'SystemFirmwareBinaryMemory',
>   'data': { 'pathname': 'str' } }
> 
> 
> ## Struct(s) for device==kernel
> 
> { 'struct': 'SystemFirmwareBinaryKernel',
>   'data': { 'pathname': 'str' } }
> 
> 
> ## Struct(s) for device==flash
> 
> { 'struct': 'SystemFirmwareBinaryFlashFile',
>   'data':  { 'filename': 'str',
>  'format': 'BlockdevDriver' } }
> 
> { 'struct': 'SystemFirmwareBinaryFlashCode',
>   'base': 'SystemFirmwareBinaryFlashFile' }
> 
> { 'struct': 'SystemFirmwareBinaryFlashVars',
>   'base': 'SystemFirmwareBinaryFlashFile',
>   'data': { 'secure-boot-key-enroll': 'bool' } }
> 
> { 'struct': 'SystemFirmwareBinaryFlash',
>   'data': { 'code': 'SystemFirmwareBinaryFlashCode',
> 'vars': ['SystemFirmwareBinaryFlashVars' ] } }
> 
> 
> ## Discriminated struct for different loading approaches
> 
> { 'union': 'SystemFirmwareBinary',
>   'base': { 'device': 'SystemFirmwareDevice' },
>   'discriminator': 'device',
>   'data': { 'memory': 'SystemFirmwareBinaryMemory',
> 'kernel': 'SystemFirmwareBinaryKernel',
> 'flash': 'SystemFirmwareBinaryFlash' } }
> 
> 
> 
> { 'struct' : 'SystemFirmwareTarget',
>   'data': { 'architecture': 'SystemFirmwareArchitecture',
> 'machines': [ 'str' ] } }
> 
> 
> { 'struct' : 'SystemFirmware',
>   'data'   : {
>   'description'  : 'str',
>   'type' : 'SystemFirmwareType',
>   'binary'   : 'SystemFirmwareBinary',
>   'targets'  : [ 'SystemFirmwareTarget' ],
>   'features' : ['SystemFirmwareFeature'] } } 
> 
> 
> 
> # Examples:
> #
> # {
> #'description': 'SeaBIOS 256k',
> #'type': 'bios',
> #'binary': {
> #'type': 'memory',
> #'filename': '/path/to/seabios/rom-256k',
> #}
> #'targets':  {
> #'x86_64': [ "pc", "q35"],
> #'i386': [ "pc", "q35"],
> #}
> #'features': ['acpi-s3', 'acpi-s5'],
> # }
> # {
> #'description': 'SeaBIOS 128k',
> #'type': 'bios',
> #'binary': {
> #'type': 'memory',
> #'filename': '/path/to/seabios/rom-128k',
> #}
> #'targets':  {
> #'x86_64': [ "isapc"],
> #'i386': [ "isapc"],
> #}
> #'features': [],
> # }
> # {
> #'description': 'OVMF',
> #'type': 'uefi'
> #'binary': {
> #'type': 'flash',
> #'code': {
> #  'filename': '/usr/share/OVMF/OVMF_CODE.secboot.fd',
> #  'format': 'raw',
> #

Re: [Qemu-devel] [PULL 0/2] linux-user fixes for -rc3

[no-reply]

Hi,

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

Type: series
Message-id: 20180410105606.16413-1-laur...@vivier.eu
Subject: [Qemu-devel] [PULL 0/2] linux-user fixes for -rc3

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

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

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

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

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]   patchew/20180410105606.16413-1-laur...@vivier.eu -> 
patchew/20180410105606.16413-1-laur...@vivier.eu
Switched to a new branch 'test'
912217b576 linux-user: add microblaze/microblazeel magic numbers in 
qemu-binfmt-conf.sh
88b131a658 linux-user: fix microblaze get_sp_from_cpustate()

=== OUTPUT BEGIN ===
Checking PATCH 1/2: linux-user: fix microblaze get_sp_from_cpustate()...
Checking PATCH 2/2: linux-user: add microblaze/microblazeel magic numbers in 
qemu-binfmt-conf.sh...
ERROR: line over 90 characters
#24: FILE: scripts/qemu-binfmt-conf.sh:7:
+sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb 
microblaze microblazeel"

WARNING: line over 80 characters
#32: FILE: scripts/qemu-binfmt-conf.sh:119:
+microblaze_magic='\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xba\xab'

ERROR: line over 90 characters
#33: FILE: scripts/qemu-binfmt-conf.sh:120:
+microblaze_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'

ERROR: line over 90 characters
#36: FILE: scripts/qemu-binfmt-conf.sh:123:
+microblazeel_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xab\xba'

ERROR: line over 90 characters
#37: FILE: scripts/qemu-binfmt-conf.sh:124:
+microblazeel_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'

total: 4 errors, 1 warnings, 26 lines checked

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

=== OUTPUT END ===

Test command exited with code: 1


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

[Qemu-devel] [PULL 2/2] linux-user: add microblaze/microblazeel magic numbers in qemu-binfmt-conf.sh

[Laurent Vivier]

Signed-off-by: Laurent Vivier 
Message-Id: <20180409115212.875-2-laur...@vivier.eu>
---
 scripts/qemu-binfmt-conf.sh | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/scripts/qemu-binfmt-conf.sh b/scripts/qemu-binfmt-conf.sh
index f39ad344fc..7ab7435fbd 100755
--- a/scripts/qemu-binfmt-conf.sh
+++ b/scripts/qemu-binfmt-conf.sh
@@ -1,10 +1,10 @@
 #!/bin/sh
-# enable automatic i386/ARM/M68K/MIPS/SPARC/PPC/s390/HPPA/Xtensa
+# enable automatic i386/ARM/M68K/MIPS/SPARC/PPC/s390/HPPA/Xtensa/microblaze
 # program execution by the kernel
 
 qemu_target_list="i386 i486 alpha arm armeb sparc32plus ppc ppc64 ppc64le m68k 
\
 mips mipsel mipsn32 mipsn32el mips64 mips64el \
-sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb"
+sh4 sh4eb s390x aarch64 aarch64_be hppa riscv32 riscv64 xtensa xtensaeb 
microblaze microblazeel"
 
 
i386_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\x03\x00'
 
i386_mask='\xff\xff\xff\xff\xff\xfe\xfe\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
@@ -116,6 +116,14 @@ 
xtensaeb_magic='\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\
 
xtensaeb_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff'
 xtensaeb_family=xtensaeb
 
+microblaze_magic='\x7fELF\x01\x02\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xba\xab'
+microblaze_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
+microblaze_family=microblaze
+
+microblazeel_magic='\x7fELF\x01\x01\x01\x00\x00\x00\x00\x00\x00\x00\x00\x00\x02\x00\xab\xba'
+microblazeel_mask='\xff\xff\xff\xff\xff\xff\xff\x00\xff\xff\xff\xff\xff\xff\xff\xff\xfe\xff\xff\xff'
+microblazeel_family=microblazeel
+
 qemu_get_family() {
 cpu=${HOST_ARCH:-$(uname -m)}
 case "$cpu" in
-- 
2.14.3

Re: [Qemu-devel] [PATCH for-2.12] fpu: Fix rounding mode for floatN_to_uintM_round_to_zero

[Peter Maydell]

On 10 April 2018 at 06:59, Richard Henderson
 wrote:
> We incorrectly passed in the current rounding mode
> instead of float_round_to_zero.
>
> Signed-off-by: Richard Henderson 
> ---
>
> Found while runnning SVE RISU tests; it should be visible with the
> right set of inputs to AdvSIMD RISU tests.



Applied to target-arm.next, thanks.

-- PMM

[Qemu-devel] [PULL 0/2] linux-user fixes for -rc3

[Laurent Vivier]

The following changes since commit 1e7e92e2ef874aa3a235d59b2be1da7a29b6fd29:

  Merge remote-tracking branch 'remotes/cohuck/tags/s390x-20180409' into 
staging (2018-04-09 18:21:23 +0100)

are available in the Git repository at:

  git://github.com/vivier/qemu.git tags/linux-user-for-2.12-pull-request

for you to fetch changes up to f5d29f9074c4cd62dfb7b229b9ad208639217a35:

  linux-user: add microblaze/microblazeel magic numbers in qemu-binfmt-conf.sh 
(2018-04-10 12:18:40 +0200)


Trivial fixes for microblaze


Laurent Vivier (2):
  linux-user: fix microblaze get_sp_from_cpustate()
  linux-user: add microblaze/microblazeel magic numbers in
qemu-binfmt-conf.sh

 linux-user/microblaze/target_signal.h |  2 +-
 scripts/qemu-binfmt-conf.sh   | 12 ++--
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.14.3

[Qemu-devel] [PULL 1/2] linux-user: fix microblaze get_sp_from_cpustate()

[Laurent Vivier]

get_sigframe() uses regs[1] and this is actual SP.

Signed-off-by: Laurent Vivier 
Message-Id: <20180409115212.875-1-laur...@vivier.eu>
---
 linux-user/microblaze/target_signal.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/microblaze/target_signal.h 
b/linux-user/microblaze/target_signal.h
index de2b0f49d5..642865f12e 100644
--- a/linux-user/microblaze/target_signal.h
+++ b/linux-user/microblaze/target_signal.h
@@ -23,7 +23,7 @@ typedef struct target_sigaltstack {
 
 static inline abi_ulong get_sp_from_cpustate(CPUMBState *state)
 {
-return state->regs[14];
+return state->regs[1];
 }
 
 
-- 
2.14.3

[Qemu-devel] [Bug 1762707] [NEW] VFIO device gets DMA failures when virtio-balloon leak from highmem to lowmem

[Pixel, Ding]

Public bug reported:

Is there any known conflict between VFIO passthrough device and virtio-
balloon?

The VM has:
1. 4GB system memory
2. one VFIO passthrough device which supports high address memory DMA and uses 
GFP_HIGHUSER pages.
3. Memory balloon device with 4GB target.

When setting the memory balloon target to 1GB and 4GB in loop during
runtime (I used the command "virsh qemu-monitor-command debian --hmp
--cmd balloon 1024"), the VFIO device DMA randomly gets failure.

More clues:
1. configure 2GB system memory (no highmem) VM, no issue with similar operations
2. setting the memory balloon to higher like 8GB, no issue with similar 
operations

I'm also trying to narrow down this issue. It's appreciated for that you
guys may share some thoughts.

** Affects: qemu
 Importance: Undecided
 Status: New

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

Title:
  VFIO device gets DMA failures when virtio-balloon leak from highmem to
  lowmem

Status in QEMU:
  New

Bug description:
  Is there any known conflict between VFIO passthrough device and
  virtio-balloon?

  The VM has:
  1. 4GB system memory
  2. one VFIO passthrough device which supports high address memory DMA and 
uses GFP_HIGHUSER pages.
  3. Memory balloon device with 4GB target.

  When setting the memory balloon target to 1GB and 4GB in loop during
  runtime (I used the command "virsh qemu-monitor-command debian --hmp
  --cmd balloon 1024"), the VFIO device DMA randomly gets failure.

  More clues:
  1. configure 2GB system memory (no highmem) VM, no issue with similar 
operations
  2. setting the memory balloon to higher like 8GB, no issue with similar 
operations

  I'm also trying to narrow down this issue. It's appreciated for that
  you guys may share some thoughts.

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

Re: [Qemu-devel] [PATCH] migration: calculate expected_downtime with ram_bytes_remaining()

[Balamuruhan S]

On 2018-04-10 15:22, Balamuruhan S wrote:

On Wed, Apr 04, 2018 at 11:04:59AM +0200, Juan Quintela wrote:

Balamuruhan S  wrote:
> expected_downtime value is not accurate with dirty_pages_rate * page_size,
> using ram_bytes_remaining would yeild it correct.
>
> Signed-off-by: Balamuruhan S 

Reviewed-by: Juan Quintela 

See my other mail on the thread, my understanding is that your change 
is

corret (TM).


Juan, Please help to merge it.


Sorry for asking it as during discussion going on, but the reason is 
currently
postcopy migration for HP backed P8 guest from P8 -> P9 is broken and to 
use
precopy with appropriate downtime value we need this patch to be 
backported

to distros that is to be released soon.



Regards,
Bala



Thanks, Juan.

> ---
>  migration/migration.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 58bd382730..4e43dc4f92 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2245,8 +2245,7 @@ static void migration_update_counters(MigrationState *s,
>   * recalculate. 1 is a small enough number for our purposes
>   */
>  if (ram_counters.dirty_pages_rate && transferred > 1) {
> -s->expected_downtime = ram_counters.dirty_pages_rate *
> -qemu_target_page_size() / bandwidth;
> +s->expected_downtime = ram_bytes_remaining() / bandwidth;
>  }
>
>  qemu_file_reset_rate_limit(s->to_dst_file);

Re: [Qemu-devel] [PATCH for-2.12] tcg: Introduce tcg_set_insn_start_param

[Peter Maydell]

On 10 April 2018 at 01:35, Richard Henderson
 wrote:
> The parameters for tcg_gen_insn_start are target_ulong, which may be split
> into two TCGArg parameters for storage in the opcode on 32-bit hosts.
>
> Fixes the ARM target and its direct use of tcg_set_insn_param, which would
> set the wrong argument in the 64-on-32 case.
>
> Cc: qemu-sta...@nongnu.org
> Reported-by: alar...@ddci.com
> Signed-off-by: Richard Henderson 
> ---
>
> Peter, I'm not sure what the reproducer is for this reported problem.

It's a standalone binary, which was attached to this email:
https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg05995.html

I've checked and this patch does indeed fix it.

Applied to target-arm.next, thanks.

-- PMM

Re: [Qemu-devel] [PATCH v2 for-2.13 1/3] net: Remove the deprecated "vlan" parameter

[Thomas Huth]

On 06.04.2018 16:17, Paolo Bonzini wrote:
> On 06/04/2018 15:37, Thomas Huth wrote:
>> It's been marked as deprecated since QEMU v2.9.0, so that should have
>> been enough time for everybody to switch to the modern -device + -netdev
>> syntax for connecting guest NICs with host network backends, or to switch
>> to the "hubport" netdev in case hubs are really wanted instead.
> 
> ... or to just drop unnecessary "vlan=0", which is the really common case.
> 
>>
>>  # @NetdevL2TPv3Options:
>>  #
>> -# Connect the VLAN to Ethernet over L2TPv3 Static tunnel
>> +# Configure an Ethernet over L2TPv3 Static tunnel.
>>  #
> 
> Since you are at it, probably remove "Static"?  I don't see the point
> really.

Sure, will do.

>> or the netdev 'nd' (for pluggable\n"
>> "NICs please use '-device devtype,netdev=nd' instead)\n"
> 
> Maybe remove this if -net is prominently marked deprecated?  And mention
> using -nic instead of "-net nic,netdev=...".

Yes, "-net nic,netdev=..." does indeed not make much sense anymore now
that we've got "-nic". I'll remove it from the documentation in the next
version of the patch.

 Thomas

Re: [Qemu-devel] [PATCH] migration: Don't activate block devices if using -S

[Dr. David Alan Gilbert]

* Kevin Wolf (kw...@redhat.com) wrote:
> Am 10.04.2018 um 10:45 hat Dr. David Alan Gilbert geschrieben:
> > * Kevin Wolf (kw...@redhat.com) wrote:
> > > Am 10.04.2018 um 09:36 hat Jiri Denemark geschrieben:
> > > > On Mon, Apr 09, 2018 at 15:40:03 +0200, Kevin Wolf wrote:
> > > > > Am 09.04.2018 um 12:27 hat Dr. David Alan Gilbert geschrieben:
> > > > > > It's a fairly hairy failure case they had; if I remember correctly 
> > > > > > it's:
> > > > > >   a) Start migration
> > > > > >   b) Migration gets to completion point
> > > > > >   c) Destination is still paused
> > > > > >   d) Libvirt is restarted on the source
> > > > > >   e) Since libvirt was restarted it fails the migration (and hence 
> > > > > > knows
> > > > > >  the destination won't be started)
> > > > > >   f) It now tries to resume the qemu on the source
> > > > > > 
> > > > > > (f) fails because (b) caused the locks to be taken on the 
> > > > > > destination;
> > > > > > hence this patch stops doing that.  It's a case we don't really 
> > > > > > think
> > > > > > about - i.e. that the migration has actually completed and all the 
> > > > > > data
> > > > > > is on the destination, but libvirt decides for some other reason to
> > > > > > abandon migration.
> > > > > 
> > > > > If you do remember correctly, that scenario doesn't feel tricky at 
> > > > > all.
> > > > > libvirt needs to quit the destination qemu, which will inactivate the
> > > > > images on the destination and release the lock, and then it can 
> > > > > continue
> > > > > the source.
> > > > > 
> > > > > In fact, this is so straightforward that I wonder what else libvirt is
> > > > > doing. Is the destination qemu only shut down after trying to continue
> > > > > the source? That would be libvirt using the wrong order of steps.
> > > > 
> > > > There's no connection between the two libvirt daemons in the case we're
> > > > talking about so they can't really synchronize the actions. The
> > > > destination daemon will kill the new QEMU process and the source will
> > > > resume the old one, but the order is completely random.
> > > 
> > > Hm, okay...
> > > 
> > > > > > Yes it was a 'block-activate' that I'd wondered about.  One 
> > > > > > complication
> > > > > > is that if this now under the control of the management layer then 
> > > > > > we
> > > > > > should stop asserting when the block devices aren't in the expected
> > > > > > state and just cleanly fail the command instead.
> > > > > 
> > > > > Requiring an explicit 'block-activate' on the destination would be an
> > > > > incompatible change, so you would have to introduce a new option for
> > > > > that. 'block-inactivate' on the source feels a bit simpler.
> > > > 
> > > > As I said in another email, the explicit block-activate command could
> > > > depend on a migration capability similarly to how pre-switchover state
> > > > works.
> > > 
> > > Yeah, that's exactly the thing that we wouldn't need if we could use
> > > 'block-inactivate' on the source instead. It feels a bit wrong to
> > > design a more involved QEMU interface around the libvirt internals,
> > 
> > It's not necessarily 'libvirt internals' - it's a case of them having to
> > cope with recovering from failures that happen around migration; it's
> > not an easy problem, and if they've got a way to stop both sides running
> > at the same time that's pretty important.
> 
> The 'libvirt internals' isn't that it needs an additional state where
> neither source nor destination QEMU own the images, but that it has to
> be between migration completion and image activation on the destination
> rather than between image inactivation on the source and migration
> completion. The latter would be much easier for qemu, but apparently it
> doesn't work for libvirt because of how it works internally.

I suspect this is actually a fundamental requirement to ensuring that we
don't end up with a QEMU running on both sides rather than how libvirt is
structured.

> But as I said, I'd just implement both for symmetry and then management
> tools can pick whatever makes their life easier.
> 
> > > but
> > > as long as we implement both sides for symmetry and libvirt just happens
> > > to pick the destination side for now, I think it's okay.
> > > 
> > > By the way, are block devices the only thing that need to be explicitly
> > > activated? For example, what about qemu_announce_self() for network
> > > cards, do we need to delay that, too?
> > > 
> > > In any case, I think this patch needs to be reverted for 2.12 because
> > > it's wrong, and then we can create the proper solution in the 2.13
> > > timefrage.
> > 
> > what case does this break?
> > I'm a bit wary of reverting this, which fixes a known problem, on the
> > basis that it causes a theoretical problem.
> 
> It breaks the API. And the final design we're having in mind now is
> compatible with the old API, not with the new one exposed by this patch,
> so that switch would break the API again to get ba

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote:
> Add a schema that describes the properties of virtual machine firmware.
> 
> Each firmware executable installed on a host system should come with a
> JSON file that conforms to this schema, and informs the management
> applications about the firmware's properties.
> 
> In addition, a configuration directory with symlinks to the JSON files
> should exist, with the symlinks carefully named to reflect a priority
> order. Management applications can then search this directory in priority
> order for the first firmware executable that satisfies their search
> criteria. The found JSON file provides the management layer with domain
> configuration bits that are required to run the firmware binary.
> 

> diff --git a/qapi/firmware.json b/qapi/firmware.json
> new file mode 100644
> index ..f267240f44dd
> --- /dev/null
> +++ b/qapi/firmware.json

[snip]

> +{ 'struct' : 'SystemFirmware',
> +  'data'   : { 'executable' : 'FirmwareFile',
> +   'type'   : 'SystemFirmwareType',
> +   'targets': [ 'str' ],
> +   'sysfw-map'  : 'FirmwareMapping',
> +   '*nvram-slots'   : [ 'NVRAMSlot' ],
> +   '*supports-uefi-secure-boot' : 'bool',
> +   '*supports-amd-sev'  : 'bool',
> +   '*supports-acpi-s3'  : 'bool',
> +   '*supports-acpi-s4'  : 'bool' } }

Elsewhere in the thread I mentioned that I think we should try to use a
union approach to isolate which information is relevant to "flash" loader
format and which is relevant to "memory" and "kernel". To try to illustrate
what I mean by that I've knocked up an alternative structure. I also
incorporated the points about features & target/machine types.  I've left
out the read/write/etc fields, but they could be put back in at the
relevant position


{ 'enum' : 'SystemFirmwareType',
  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

{ 'enum' : 'SystemFirmwareDevice',
  'data' : [ 'memory', 'kernel', 'flash' ] }

{ 'enum' : 'SystemFirmwareArchitecture',
  'data':  ['x86_64', 'i386', ..etc.. ] }
  
{ 'enum' : 'SystemFirmwareFeature',
  'data': ['acpi-s3', 'acpi-s5', 'secure-boot', 'amd-sev' ]}


## Struct(s) for device==memory

{ 'struct': 'SystemFirmwareBinaryMemory',
  'data': { 'pathname': 'str' } }


## Struct(s) for device==kernel

{ 'struct': 'SystemFirmwareBinaryKernel',
  'data': { 'pathname': 'str' } }


## Struct(s) for device==flash

{ 'struct': 'SystemFirmwareBinaryFlashFile',
  'data':  { 'filename': 'str',
 'format': 'BlockdevDriver' } }

{ 'struct': 'SystemFirmwareBinaryFlashCode',
  'base': 'SystemFirmwareBinaryFlashFile' }

{ 'struct': 'SystemFirmwareBinaryFlashVars',
  'base': 'SystemFirmwareBinaryFlashFile',
  'data': { 'secure-boot-key-enroll': 'bool' } }

{ 'struct': 'SystemFirmwareBinaryFlash',
  'data': { 'code': 'SystemFirmwareBinaryFlashCode',
'vars': ['SystemFirmwareBinaryFlashVars' ] } }


## Discriminated struct for different loading approaches

{ 'union': 'SystemFirmwareBinary',
  'base': { 'device': 'SystemFirmwareDevice' },
  'discriminator': 'device',
  'data': { 'memory': 'SystemFirmwareBinaryMemory',
'kernel': 'SystemFirmwareBinaryKernel',
'flash': 'SystemFirmwareBinaryFlash' } }



{ 'struct' : 'SystemFirmwareTarget',
  'data': { 'architecture': 'SystemFirmwareArchitecture',
'machines': [ 'str' ] } }


{ 'struct' : 'SystemFirmware',
  'data'   : {
  'description'  : 'str',
  'type' : 'SystemFirmwareType',
  'binary'   : 'SystemFirmwareBinary',
  'targets'  : [ 'SystemFirmwareTarget' ],
  'features' : ['SystemFirmwareFeature'] } } 



# Examples:
#
# {
#'description': 'SeaBIOS 256k',
#'type': 'bios',
#'binary': {
#'type': 'memory',
#'filename': '/path/to/seabios/rom-256k',
#}
#'targets':  {
#'x86_64': [ "pc", "q35"],
#'i386': [ "pc", "q35"],
#}
#'features': ['acpi-s3', 'acpi-s5'],
# }
# {
#'description': 'SeaBIOS 128k',
#'type': 'bios',
#'binary': {
#'type': 'memory',
#'filename': '/path/to/seabios/rom-128k',
#}
#'targets':  {
#'x86_64': [ "isapc"],
#'i386': [ "isapc"],
#}
#'features': [],
# }
# {
#'description': 'OVMF',
#'type': 'uefi'
#'binary': {
#'type': 'flash',
#'code': {
#  'filename': '/usr/share/OVMF/OVMF_CODE.secboot.fd',
#  'format': 'raw',
#},
#'vars': [
#   {
#  'filename': '/usr/share/OVMF/OVMF_VARS.fd',
#  'format': 'raw',
#  'secure=boot-key-enroll': false,
#   },
#   {
#  'filename': '/usr/share/OVMF/OVMF_VARS.secboot.fd',
#  'format': 'raw',
#  'secure=boot-key-enroll': true,
#   }
# 

Re: [Qemu-devel] [PATCH v2 for-2.12] icount: fix cpu_restore_state_from_tb for non-tb-exit cases

[Paolo Bonzini]

On 10/04/2018 12:13, Richard Henderson wrote:
> In most cases new parameter is true (icount should be recalculated).
> But there are two cases in i386 and openrisc when the CPU state is only
> queried without the need to break the TB.  This patch fixes both of
> these cases.
> 
> Signed-off-by: Pavel Dovgalyuk 
> Message-Id: <20180409091320.12504.35329.stgit@pasha-VirtualBox>
> [rth: Make can_do_io setting unconditional; move from cpu_exec;
> make cpu_loop_exit_{noexc,restore} call cpu_loop_exit.]
> Signed-off-by: Richard Henderson 

Looks good, thanks!

Paolo

Re: [Qemu-devel] [Qemu-arm] Crash when running hello-world unikernel for ARM

[Ajay Garg]

Thanks Peter .. my sincere gratitude.
You pin-pointed the real issue ..



On Tue, Apr 10, 2018 at 2:50 PM, Peter Maydell  wrote:
> On 10 April 2018 at 09:16, Ajay Garg  wrote:
>> On Tue, Apr 10, 2018 at 1:08 PM, Peter Maydell  
>> wrote:
>>> What hardware (what CPU, board, etc) is this "rumprun" software
>>> expecting to run on?
>>
>> Yep, just to ensure that there are no cross-compiling issues, I am
>> building rumprun on the pseudo-real hardware itself.
>> In our case, the pseudo-real hardware are :
>>
>> a)
>> An ARM32 "virt" hardware/machine in a qemu environment
>> (https://translatedcode.wordpress.com/2016/11/03/installing-debian-on-qemus-32-bit-arm-virt-board/)
>>
>> Once I start  this machine, all environment is arm32, and I compile
>> rumprun within this environemnt without any cross-compiling.
>>
>> b)
>> A beaglebone-green-wireless.
>> This is a arm32 machine bottoms-up, so no question of cross-compiling
>> whatsoever here :)
>>
>> In both cases, I then use qemu-system-arm (on the "virt" machine, and
>> beaglebone-green-wireless itself).
>
> That's telling me what setups you're trying to compile in,
> which doesn't correspond necessarily to what the guest
> OS is built to run on.
>
>> One query : It is apparent that there is nested qemu-virtualization in
>> step a), could that be an issue?
>
> Why are you running this in a nested setup? I don't understand
> the purpose of doing that. It would be simpler and faster to
> just run the guest on a QEMU running in your native host system.
>
> Assuming this is the source for the guest you're trying to run:
>
> https://github.com/rumpkernel/rumprun/tree/master/platform/hw/arch
>
> that suggests that the only Arm board it supports is "integrator"
> (which is an absolutely ancient devboard with very little memory,
> no PCI and no virtio support). You need to confirm what Arm hardware
> this 'rumpkernel' is actually intended to run on, and then give QEMU
> the right command line arguments to emulate that hardware. I can't
> really help any further, I'm afraid -- you need somebody who knows
> about this guest OS.
>
> thanks
> -- PMM



-- 
Regards,
Ajay

Re: [Qemu-devel] [PATCH for-2.12] fpu: Fix rounding mode for floatN_to_uintM_round_to_zero

[Richard Henderson]

On 04/10/2018 05:40 PM, Peter Maydell wrote:
> Would this be likely the fix for
> https://bugs.launchpad.net/qemu/+bug/1761401
> ?

Yes indeed.


r~

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Paolo Bonzini]

On 10/04/2018 11:23, Daniel P. Berrangé wrote:
>> And, really, this seems to reinforce my point that the schema should
>> live in the libvirtd tree, not in the QEMU tree. In that case, perhaps
>> it would be a better fit to work with an XSD, and firmware packages
>> should install XML files? Personally I'm a lot more attracted to
>> XML/XSD; I think the tooling is better too. I just don't see how QEMU is
>> involved.
>
> This is defining a set of metadata that is required to use various firmware
> files in combination with QEMU, along with defining a mapping to QEMU command
> line arguments and/or features. Essentially, while I wish everyone used
> libvirt, libvirt is not the only thing that manages QEMU. This information is
> relevant to anyone managing QEMU, so it doesn't belong in libvirt's realm,
> it is clear QEMU is best placed to declare this information.

QEMU is best placed to _standardize_ how to provide this information
(and where in the file system to place it), but really it's up to
firmware packages to provide it.

We can of course define the schema in QAPI terms for ease of validation
(machine-readable specs are nice to have), but really this should just
be a file in docs/interop/.  No code is needed in QEMU.

Paolo

[Qemu-devel] [PATCH v2 for-2.12] icount: fix cpu_restore_state_from_tb for non-tb-exit cases

[Richard Henderson]

From: Pavel Dovgalyuk 

In icount mode, instructions that access io memory spaces in the middle
of the translation block invoke TB recompilation.  After recompilation,
such instructions become last in the TB and are allowed to access io
memory spaces.

When the code includes instruction like i386 'xchg eax, 0xd080'
which accesses APIC, QEMU goes into an infinite loop of the recompilation.

This instruction includes two memory accesses - one read and one write.
After the first access, APIC calls cpu_report_tpr_access, which restores
the CPU state to get the current eip.  But cpu_restore_state_from_tb
resets the cpu->can_do_io flag which makes the second memory access invalid.
Therefore the second memory access causes a recompilation of the block.
Then these operations repeat again and again.

This patch moves resetting cpu->can_do_io flag from
cpu_restore_state_from_tb to cpu_loop_exit* functions.

It also adds a parameter for cpu_restore_state which controls restoring
icount.  There is no need to restore icount when we only query CPU state
without breaking the TB.  Restoring it in such cases leads to the
incorrect flow of the virtual time.

In most cases new parameter is true (icount should be recalculated).
But there are two cases in i386 and openrisc when the CPU state is only
queried without the need to break the TB.  This patch fixes both of
these cases.

Signed-off-by: Pavel Dovgalyuk 
Message-Id: <20180409091320.12504.35329.stgit@pasha-VirtualBox>
[rth: Make can_do_io setting unconditional; move from cpu_exec;
make cpu_loop_exit_{noexc,restore} call cpu_loop_exit.]
Signed-off-by: Richard Henderson 
---
 include/exec/exec-all.h  |  5 -
 accel/tcg/cpu-exec-common.c  | 10 +-
 accel/tcg/cpu-exec.c |  1 -
 accel/tcg/translate-all.c| 27 ++-
 accel/tcg/user-exec.c|  2 +-
 hw/misc/mips_itu.c   |  3 +--
 target/alpha/helper.c|  2 +-
 target/alpha/mem_helper.c|  6 ++
 target/arm/op_helper.c   |  6 +++---
 target/cris/op_helper.c  |  4 ++--
 target/i386/helper.c |  2 +-
 target/i386/svm_helper.c |  2 +-
 target/m68k/op_helper.c  |  4 ++--
 target/moxie/helper.c|  2 +-
 target/openrisc/sys_helper.c |  8 
 target/tricore/op_helper.c   |  2 +-
 target/xtensa/op_helper.c|  4 ++--
 17 files changed, 45 insertions(+), 45 deletions(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e5afd2e6d3..bd68328ed9 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -50,13 +50,16 @@ void cpu_gen_init(void);
  * cpu_restore_state:
  * @cpu: the vCPU state is to be restore to
  * @searched_pc: the host PC the fault occurred at
+ * @will_exit: true if the TB executed will be interrupted after some
+   cpu adjustments. Required for maintaining the correct
+   icount valus
  * @return: true if state was restored, false otherwise
  *
  * Attempt to restore the state for a fault occurring in translated
  * code. If the searched_pc is not in translated code no state is
  * restored and the function returns false.
  */
-bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc);
+bool cpu_restore_state(CPUState *cpu, uintptr_t searched_pc, bool will_exit);
 
 void QEMU_NORETURN cpu_loop_exit_noexc(CPUState *cpu);
 void QEMU_NORETURN cpu_io_recompile(CPUState *cpu, uintptr_t retaddr);
diff --git a/accel/tcg/cpu-exec-common.c b/accel/tcg/cpu-exec-common.c
index dac5aac477..2988fde650 100644
--- a/accel/tcg/cpu-exec-common.c
+++ b/accel/tcg/cpu-exec-common.c
@@ -27,10 +27,8 @@ bool tcg_allowed;
 /* exit the current TB, but without causing any exception to be raised */
 void cpu_loop_exit_noexc(CPUState *cpu)
 {
-/* XXX: restore cpu registers saved in host registers */
-
 cpu->exception_index = -1;
-siglongjmp(cpu->jmp_env, 1);
+cpu_loop_exit(cpu);
 }
 
 #if defined(CONFIG_SOFTMMU)
@@ -65,15 +63,17 @@ void cpu_reloading_memory_map(void)
 
 void cpu_loop_exit(CPUState *cpu)
 {
+/* Undo the setting in cpu_tb_exec.  */
+cpu->can_do_io = 1;
 siglongjmp(cpu->jmp_env, 1);
 }
 
 void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
 {
 if (pc) {
-cpu_restore_state(cpu, pc);
+cpu_restore_state(cpu, pc, true);
 }
-siglongjmp(cpu->jmp_env, 1);
+cpu_loop_exit(cpu);
 }
 
 void cpu_loop_exit_atomic(CPUState *cpu, uintptr_t pc)
diff --git a/accel/tcg/cpu-exec.c b/accel/tcg/cpu-exec.c
index 9cc697205c..81153e7a13 100644
--- a/accel/tcg/cpu-exec.c
+++ b/accel/tcg/cpu-exec.c
@@ -704,7 +704,6 @@ int cpu_exec(CPUState *cpu)
 g_assert(cpu == current_cpu);
 g_assert(cc == CPU_GET_CLASS(cpu));
 #endif /* buggy compiler */
-cpu->can_do_io = 1;
 tb_lock_reset();
 if (qemu_mutex_iothread_locked()) {
 qemu_mutex_unlock_iothread();
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index d4190602d1..f409d42d54 100644
--- a/accel/tcg/translate-all

Re: [Qemu-devel] [PATCH] migration: calculate expected_downtime with ram_bytes_remaining()

[Dr. David Alan Gilbert]

* David Gibson (dgib...@redhat.com) wrote:
> On Mon, 9 Apr 2018 19:57:47 +0100
> "Dr. David Alan Gilbert"  wrote:
> 
> > * Balamuruhan S (bal...@linux.vnet.ibm.com) wrote:
> > > On 2018-04-04 13:36, Peter Xu wrote:  
> > > > On Wed, Apr 04, 2018 at 11:55:14AM +0530, Balamuruhan S wrote:
> [snip]
> > > > > > - postcopy: that'll let you start the destination VM even without
> > > > > >   transferring all the RAMs before hand  
> > > > > 
> > > > > I am seeing issue in postcopy migration between POWER8(16M) ->
> > > > > POWER9(1G)
> > > > > where the hugepage size is different. I am trying to enable it but
> > > > > host
> > > > > start
> > > > > address have to be aligned with 1G page size in
> > > > > ram_block_discard_range(),
> > > > > which I am debugging further to fix it.  
> > > > 
> > > > I thought the huge page size needs to be matched on both side
> > > > currently for postcopy but I'm not sure.  
> > > 
> > > you are right! it should be matched, but we need to support
> > > POWER8(16M) -> POWER9(1G)
> > >   
> > > > CC Dave (though I think Dave's still on PTO).  
> > 
> > There's two problems there:
> >   a) Postcopy with really big huge pages is a problem, because it takes
> >  a long time to send the whole 1G page over the network and the vCPU
> >  is paused during that time;  for example on a 10Gbps link, it takes
> >  about 1 second to send a 1G page, so that's a silly time to keep
> >  the vCPU paused.
> > 
> >   b) Mismatched pagesizes are a problem on postcopy; we require that the
> >  whole of a hostpage is sent continuously, so that it can be
> >  atomically placed in memory, the source knows to do this based on
> >  the page sizes that it sees.  There are some other cases as well 
> >  (e.g. discards have to be page aligned.)
> 
> I'm not entirely clear on what mismatched means here.  Mismatched
> between where and where?  I *think* the relevant thing is a mismatch
> between host backing page size on source and destination, but I'm not
> certain.

Right.  As I understand it, we make no requirements on (an x86) guest
as to what page sizes it uses given any particular host page sizes.

> > Both of the problems are theoretically fixable; but neither case is
> > easy.
> > (b) could be fixed by sending the hugepage size back to the source,
> > so that it knows to perform alignments on a larger boundary to it's
> > own RAM blocks.
> 
> Sounds feasible, but like something that will take some thought and
> time upstream.

Yes; it's not too bad.

> > (a) is a much much harder problem; one *idea* would be a major
> > reorganisation of the kernels hugepage + userfault code to somehow
> > allow them to temporarily present as normal pages rather than a
> > hugepage.
> 
> Yeah... for Power specifically, I think doing that would be really
> hard, verging on impossible, because of the way the MMU is
> virtualized.  Well.. it's probably not too bad for a native POWER9
> guest (using the radix MMU), but the issue here is for POWER8 compat
> guests which use the hash MMU.

My idea was to fill the pagetables for that hugepage using small page
entries but using the physical hugepages memory; so that once we're
done we'd flip it back to being a single hugepage entry.
(But my understanding is that doesn't fit at all into the way the kernel
hugepage code works).

> > Does P9 really not have a hugepage that's smaller than 1G?
> 
> It does (2M), but we can't use it in this situation.  As hinted above,
> POWER9 has two very different MMU modes, hash and radix.  In hash mode
> (which is similar to POWER8 and earlier CPUs) the hugepage sizes are
> 16M and 16G, in radix mode (more like x86) they are 2M and 1G.
> 
> POWER9 hosts always run in radix mode.  Or at least, we only support
> running them in radix mode.  We support both radix mode and hash mode
> guests, the latter including all POWER8 compat mode guests.
> 
> The next complication is because the way the hash virtualization works,
> any page used by the guest must be HPA-contiguous, not just
> GPA-contiguous.  Which means that any pagesize used by the guest must
> be smaller or equal than the host pagesizes used to back the guest.
> We (sort of) cope with that by only advertising the 16M pagesize to the
> guest if all guest RAM is backed by >= 16M pages.
> 
> But that advertisement only happens at guest boot.  So if we migrate a
> guest from POWER8, backed by 16M pages to POWER9 backed by 2M pages,
> the guest still thinks it can use 16M pages and jams up.  (I'm in the
> middle of upstream work to make the failure mode less horrible).
> 
> So, the only way to run a POWER8 compat mode guest with access to 16M
> pages on a POWER9 radix mode host is using 1G hugepages on the host
> side.

Ah ok;  I'm not seeing an easy answer here.
The only vague thing I can think of is if you gave P9 a fake 16M
hugepage mode, that did all HPA and mappings in 16M chunks (using 8 x 2M
page entries).

Dave

> -- 
> David Gibson 
> Principal Softwar

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 11:51:31AM +0200, Gerd Hoffmann wrote:
> > > Hmm, I'm wondering whenever it is useful to model things this way.  It's
> > > not like you can actually configure things for -bios seabios.rom or
> > > -kernel uboot.elf.  Only pflash allows to actually configure things, and
> > > there are not that many useful combinations.  The code needs
> > > Read+Execute.  Allowing Write could be useful in theory, to allow the
> > > guest doing firmware updates.  But I think nobody actually does that, so
> > > in practice it is fixed.  The varstore can have different permissions,
> > > but it's only two useful combinations.  Either allow access
> > > unconditionally, or allow access in secure contect (aka smm) only.
> > 
> > (I hope I understand your point right:)
> > 
> > I'm also fine if we simply define a fixed (but extensible) set of
> > mapping methods, basically a new enum type, that simply tells libvirtd
> > what this firmware *is*. IOW, directly reference a mapping method we
> > know libvirt implements, rather than give vague hints.
> > 
> > This could repurpose SystemFirmwareType, but it should become more
> > detailed. I'm thinking like:
> > - ovmf: split files without requiring SMM
> > - ovmf_smm: split files with SMM requirement
> > - seabios: exactly that
> > - ... other things others suggest.
> 
> I wouldn't name them by firmware, that is misleading.  Basically we have
> three cases:
> 
>   (1) single firmware image (seabios, OVMF.fd, ...).
>   (2) split firmware image (OVMF_{CODE,VARS}.fd), where vars can be
>   writable unconditinally.
>   (3) split firmware image, where access to vars should be restricted
>   to smm mode.
> 
> (2) + (3) requires pflash.  (1) works with both pflash and -bios.

A big chunk of the data in the schema looks specific to the pflash
case, but this is not expressed except in the docs. Most of the time
with QAPI when we have data that is only relevant in certain types,
we use a discriminated union to describe it. It feels like a unioon
approach could be better suited to this

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

Re: [Qemu-devel] [PATCH] migration: calculate expected_downtime with ram_bytes_remaining()

[Balamuruhan S]

On Wed, Apr 04, 2018 at 11:04:59AM +0200, Juan Quintela wrote:
> Balamuruhan S  wrote:
> > expected_downtime value is not accurate with dirty_pages_rate * page_size,
> > using ram_bytes_remaining would yeild it correct.
> >
> > Signed-off-by: Balamuruhan S 
> 
> Reviewed-by: Juan Quintela 
> 
> See my other mail on the thread, my understanding is that your change is
> corret (TM).

Juan, Please help to merge it.

Regards,
Bala

> 
> Thanks, Juan.
> 
> > ---
> >  migration/migration.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 58bd382730..4e43dc4f92 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2245,8 +2245,7 @@ static void migration_update_counters(MigrationState 
> > *s,
> >   * recalculate. 1 is a small enough number for our purposes
> >   */
> >  if (ram_counters.dirty_pages_rate && transferred > 1) {
> > -s->expected_downtime = ram_counters.dirty_pages_rate *
> > -qemu_target_page_size() / bandwidth;
> > +s->expected_downtime = ram_bytes_remaining() / bandwidth;
> >  }
> >  
> >  qemu_file_reset_rate_limit(s->to_dst_file);
> 

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Gerd Hoffmann]

> > Hmm, I'm wondering whenever it is useful to model things this way.  It's
> > not like you can actually configure things for -bios seabios.rom or
> > -kernel uboot.elf.  Only pflash allows to actually configure things, and
> > there are not that many useful combinations.  The code needs
> > Read+Execute.  Allowing Write could be useful in theory, to allow the
> > guest doing firmware updates.  But I think nobody actually does that, so
> > in practice it is fixed.  The varstore can have different permissions,
> > but it's only two useful combinations.  Either allow access
> > unconditionally, or allow access in secure contect (aka smm) only.
> 
> (I hope I understand your point right:)
> 
> I'm also fine if we simply define a fixed (but extensible) set of
> mapping methods, basically a new enum type, that simply tells libvirtd
> what this firmware *is*. IOW, directly reference a mapping method we
> know libvirt implements, rather than give vague hints.
> 
> This could repurpose SystemFirmwareType, but it should become more
> detailed. I'm thinking like:
> - ovmf: split files without requiring SMM
> - ovmf_smm: split files with SMM requirement
> - seabios: exactly that
> - ... other things others suggest.

I wouldn't name them by firmware, that is misleading.  Basically we have
three cases:

  (1) single firmware image (seabios, OVMF.fd, ...).
  (2) split firmware image (OVMF_{CODE,VARS}.fd), where vars can be
  writable unconditinally.
  (3) split firmware image, where access to vars should be restricted
  to smm mode.

(2) + (3) requires pflash.  (1) works with both pflash and -bios.

There also is (4) elf binary loadable with -kernel.  Not sure we should
include that case.  u-boot can be loaded that way.  The elf binary seems
to be more a side product of the build proccess, I always have both
u-boot (elf binary) and u-boot.bin (binary blob loadable with -bios).
So maybe we should put aside -kernel for now, and maybe reconsider once
a real need for it shows up.

So maybe Firmware{Device,Access,Mapping} should be replaced with a
FirmwareImageType [ 'single', 'code+vars', 'code+protectedvars' ] ?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] i386/kvm: add support for KVM_CAP_X86_DISABLE_EXITS

[Wanpeng Li]

Hi Paolo,
2018-03-27 3:43 GMT+08:00 Eduardo Habkost :
> On Sun, Mar 25, 2018 at 11:33:01AM +0800, Wanpeng Li wrote:
>> 2018-03-24 4:18 GMT+08:00 Eduardo Habkost :
>> > On Fri, Mar 16, 2018 at 07:36:42AM -0700, Wanpeng Li wrote:
>> >> From: Wanpeng Li 
>> >>
>> >> This patch adds support for KVM_CAP_X86_DISABLE_EXITS. Provides userspace 
>> >> with
>> >> per-VM capability(KVM_CAP_X86_DISABLE_EXITS) to not intercept 
>> >> MWAIT/HLT/PAUSE
>> >> in order that to improve latency in some workloads.
>> >>
>> >> Cc: Paolo Bonzini 
>> >> Cc: Radim Krčmář 
>> >> Cc: Eduardo Habkost 
>> >> Signed-off-by: Wanpeng Li 
>> >
>> >
>> > Thanks.
>> >
>> > Patch looks good (except for comment below), but I would like to
>> > see QEMU documentation mentioning what exactly are the practical
>> > consequences of setting "+kvm-hint-dedicated" (especially what
>> > could happen if people enable the flag without properly
>> > configuring vCPU pinning).
>> >
>> >
>> > [...]
>> >> +if (env->features[FEAT_KVM_HINTS] & KVM_HINTS_DEDICATED) {
>> >> +int disable_exits = kvm_check_extension(cs->kvm_state, 
>> >> KVM_CAP_X86_DISABLE_EXITS);
>> >> +if (disable_exits) {
>> >> +disable_exits &= (KVM_X86_DISABLE_EXITS_MWAIT |
>> >> +  KVM_X86_DISABLE_EXITS_HLT |
>> >> +  KVM_X86_DISABLE_EXITS_PAUSE);
>> >> +}
>> >
>> > Documentation/virtual/kvm/api.txt says that KVM_FEATURE_PV_UNHALT
>> > shouldn't be enabled if disabling HLT exits.  This needs to be
>> > handled by QEMU.
>>
>> This is handled by KVM(in kvm_update_cpuid()) currently to avoid kvm
>> userspace doing something crazy.
>> https://git.kernel.org/pub/scm/virt/kvm/kvm.git/commit/?h=queue&id=caa057a2cad647fb368a12c8e6c410ac4c28e063
>
> This seems to disable kvm-pv-unhalt silently if
> KVM_X86_DISABLE_EXITS_HLT is enabled.  We shouldn't do that if
> the user explicitly requested +kvm-pv-unhalt in the command-line.
>
>>
>> >
>> > Probably the simplest solution is to not allow kvm-hint-dedicated
>> > to be enabled if kvm-pv-unhalt is.  This should be mentioned in
>> > QEMU documentation, also, especially considering that we might
>> > enable kvm-pv-unhalt by default in future QEMU versions.
>>
>> As Locking guy Waiman mentioned before:
>> > Generally speaking, unfair lock performs well for VMs with a small number 
>> > of vCPUs. Native qspinlock may perform better than pvqspinlock if there is 
>> > vCPU pinning and there is no vCPU over-commitment.
>> I think +kvm-hint-dedicated, -kvm-pv-unhalt is more suitable for vCPU
>> pinning and there is no vCPU over-commitment, on the contrary,
>> -kvm-hint-dedicated, +kvm-pv-unhalt is more prefer.
>
> Disabling kvm-pv-unhalt by default if only "-cpu
> ...,+kvm-hint-dedicated" is used makes sense.  But we still need
> the system to not silently ignore options if
> "-cpu ...,+kvm-pv-unhalt,+kvm-hint-dedicated" is specified.

What's your opinion for these two comments from Eduardo?

Regards,
Wanpeng Li

Re: [Qemu-devel] [PATCH] icount: fix cpu_restore_state_from_tb for non-tb-exit cases

[Richard Henderson]

On 04/10/2018 05:35 PM, Paolo Bonzini wrote:
> This is incorrect, "cpu->can_do_io" is 1 when not in tcg_qemu_tb_exec.
> In fact, in cpu_exec we have "cpu->can_do_io = 1;" immediately after
> siglongjmp, so I propose adding the same "cpu->can_do_io = 1;"
> assignment to cpu_exec_step_atomic.

Ooo, good catch.  I agree.

> In any case, please change the two siglongjmp of
> cpu_loop_exit_{noexc,restore} to cpu_loop_exit, instead of duplicating
> that cpu->can_do_io assignment.

I've made that change too.  I'll post a v2 shortly.


r~

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 11:16:01AM +0200, Laszlo Ersek wrote:
> On 04/10/18 08:27, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >> - I considered adding wildcards (say, blacklist "all" i440fx machtypes,
> >> present and future, for SMM-requiring OVMF builds), but then you get
> >> into version sorting and similar mess. I considered fnmatch() --
> >> basically simple ? and * wildcards -- but that's not expressive enough.
> > 
> > I'd suggest whitelist with wildcards.  So the smm builds would get
> > "pc-q35-*".
> > 
> > libvirt knows about aliases, so it should be able to handle the "q35"
> > shortcut like "pc-q35-${latest}".
> > 
> > Or do you see another issue?
> 
> Well, one issue I see is version sorting; I should say "Q35 but no
> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4".
> 
> Anyway (also asking for Thomas's input here): if we run with your idea
> to refer to exact mapping methods / firmware *implementation* types that
> we know libvirt implements / supports as a "white box", do we still deem
> machine type identification necessary? Because, libvirt already knows
> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just
> have to make a *reference* to that knowledge in the JSON file.

BTW, that's not quite correct - when libvirt handles the "smm" arg it
checks if machine type == q35, and  QEMU version >= 2.4.

It is *not* checking the version of the machine type. ie it will happily
use smm with  pc-q35-2.0, as long as QEMU version is 2.4. Perhaps this is
not quite right, but we don't try to parse the version number out of the
machine type, because we can't assume a specific format for the machine
type version part. eg version can be "2.4", or it can be "rhel-7.0.0"
or something else again on Ubuntu.

IMHO it would be valid to just keep life simple and only record the base
machine type name that can use the firmware ie "pc", "q35", and ignore
the fact that in some cases the firmware might require a specific version
of the machine type.

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

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Thomas Huth]

On 10.04.2018 11:22, Laszlo Ersek wrote:
> On 04/10/18 09:33, Thomas Huth wrote:
[...]
>> Alternatively, what about providing some kind of "alias" or "nickname"
>> setting here, too? So the EDK2 builds would get
>> SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example.
> 
> I hope I understand you right -- I think your suggestion ties in with my
> other email I just sent in this thread. So, we could tell libvirtd,
> "this firmware is of type 'UEFI', and you must use the 'ovmf_smm'
> mapping method to run it, with this file or that file as varstore template".
> 
> We could even describe the parameters for this or that mapping method
> structurally in the schema (in a discriminated union in QAPI JSON, or in
> an XSD choice element). For example, "ovmf" and "ovmf_smm" would both
> take "OvmfSplitFileOptions" -- a list of single varstore template files
> with feature enum contants attached  --, while "SeaBiosOptions" would be
> an empty structure.

Sorry, I've got no clue about ovmf_smm and the other things you've
mentioned here ;-)

> I feel the key question here is whether we are allowed to directly
> reference a mapping method we know libvirt implements. If we are, that
> makes things a lot clearer (and easier, I should hope).

Key question is maybe rather: Do you want to design / implement
something that is libvirt-only here, or rather something generic that
could also be used for other upper layer tools that do not use libvirt?
(... and looks like Daniel just had the same comment in another mail in
this thread ...)

 Thomas

[Qemu-devel] [PULL 2/4] gtk: drop pointless code from gd_window_close

[Gerd Hoffmann]

Unregistering the display change listener looks like a pointless
excercise given we'll exit in a moment.  When exiting qemu via
menu/file/quit this will not happen either.  Just drop the code.

Also return TRUE unconditionally.  This will tell gtk to ignore the
close request, so gtk will not start destroying widgets and causing
warnings due to UI code trying to talk to widgets which are gone.
Just depend on qmp_quit() doing it's job instead.

Reported-by: Mark Cave-Ayland 
Signed-off-by: Gerd Hoffmann 
Message-Id: <20180314080439.4229-1-kra...@redhat.com>
---
 ui/gtk.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index e98ac4d2fc..bb3214cffb 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -786,21 +786,13 @@ static gboolean gd_window_close(GtkWidget *widget, 
GdkEvent *event,
 {
 GtkDisplayState *s = opaque;
 bool allow_close = true;
-int i;
 
 if (s->opts->has_window_close && !s->opts->window_close) {
 allow_close = false;
 }
 
 if (allow_close) {
-for (i = 0; i < s->nb_vcs; i++) {
-if (s->vc[i].type != GD_VC_GFX) {
-continue;
-}
-unregister_displaychangelistener(&s->vc[i].gfx.dcl);
-}
 qmp_quit(NULL);
-return FALSE;
 }
 
 return TRUE;
-- 
2.9.3

[Qemu-devel] [PULL 1/4] ui: fix keymap detection under Xwayland

[Gerd Hoffmann]

From: Daniel P. Berrangé 

The X11 code currently detects the keymap by looking for the keycode
name property. Unfortunately due to the way Xwayland handles keyboards,
this property gets unset almost immediately after the first application
starts using Xwayland resulting in

  ** (qemu-system-x86_64:19644): WARNING **: Unknown X11 keycode mapping 
'(unnamed)'.
  Please report to qemu-devel@nongnu.org
  including the following information:

- Operating system
- X11 Server
- xprop -root
- xdpyinfo

Fortunately people will only see this problem if they built QEMU with
GTK2, or have told GTK3 to prefer X11 by setting the GDK_BACKEND=x11
env variable.

To workaround the problem, we add a heuristic that looks at what
scancode the XK_Page_Up keysymbol maps to, to determine if we've
likely got the X11 kbd or evdev driver.

Signed-off-by: Daniel P. Berrangé 
Message-Id: <20180313104235.20725-1-berra...@redhat.com>
Signed-off-by: Gerd Hoffmann 
---
 ui/x_keymap.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/ui/x_keymap.c b/ui/x_keymap.c
index 22e0e77c4d..2bc01432e5 100644
--- a/ui/x_keymap.c
+++ b/ui/x_keymap.c
@@ -17,6 +17,7 @@
 #include "ui/input.h"
 
 #include 
+#include 
 
 static gboolean check_for_xwin(Display *dpy)
 {
@@ -87,11 +88,13 @@ const guint16 *qemu_xkeymap_mapping_table(Display *dpy, 
size_t *maplen)
 trace_xkeymap_keymap("xquartz");
 *maplen = qemu_input_map_xorgxquartz_to_qcode_len;
 return qemu_input_map_xorgxquartz_to_qcode;
-} else if (keycodes && g_str_has_prefix(keycodes, "evdev")) {
+} else if ((keycodes && g_str_has_prefix(keycodes, "evdev")) ||
+   (XKeysymToKeycode(dpy, XK_Page_Up) == 0x70)) {
 trace_xkeymap_keymap("evdev");
 *maplen = qemu_input_map_xorgevdev_to_qcode_len;
 return qemu_input_map_xorgevdev_to_qcode;
-} else if (keycodes && g_str_has_prefix(keycodes, "xfree86")) {
+} else if ((keycodes && g_str_has_prefix(keycodes, "xfree86")) ||
+   (XKeysymToKeycode(dpy, XK_Page_Up) == 0x63)) {
 trace_xkeymap_keymap("kbd");
 *maplen = qemu_input_map_xorgkbd_to_qcode_len;
 return qemu_input_map_xorgkbd_to_qcode;
-- 
2.9.3

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Thomas Huth]

On 10.04.2018 11:16, Laszlo Ersek wrote:
> On 04/10/18 08:27, Gerd Hoffmann wrote:
>>   Hi,
>>
>>> - I considered adding wildcards (say, blacklist "all" i440fx machtypes,
>>> present and future, for SMM-requiring OVMF builds), but then you get
>>> into version sorting and similar mess. I considered fnmatch() --
>>> basically simple ? and * wildcards -- but that's not expressive enough.
>>
>> I'd suggest whitelist with wildcards.  So the smm builds would get
>> "pc-q35-*".
>>
>> libvirt knows about aliases, so it should be able to handle the "q35"
>> shortcut like "pc-q35-${latest}".
>>
>> Or do you see another issue?
> 
> Well, one issue I see is version sorting; I should say "Q35 but no
> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4".
> 
> Anyway (also asking for Thomas's input here): if we run with your idea
> to refer to exact mapping methods / firmware *implementation* types that
> we know libvirt implements / supports as a "white box", do we still deem
> machine type identification necessary? Because, libvirt already knows
> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just
> have to make a *reference* to that knowledge in the JSON file.

I think you really need a way to specify the machine there. Latest
example from QEMU 2.12: We've now got two uboot binaries in the tree,
pc-bios/u-boot.e500 and pc-bios/u-boot-sam460-20100605.bin. Both are
uboot, both are for ppc, but u-boot.e500 only works with the "ppce500"
machine and the other one only works with the "sam460ex" machine. How
would you teach libvirt such a relationship without an explicit machine
type identification field there?

 Thomas

[Qemu-devel] [PULL 0/4] Ui 20180410 patches

[Gerd Hoffmann]

The following changes since commit 915d34c5f99b0ab91517c69f54272bfdb6ca2b32:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream' into staging 
(2018-04-09 17:29:10 +0100)

are available in the git repository at:

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

for you to fetch changes up to c6093a05d6a84d2144bb6462cf20e907eddf8aeb:

  configure: don't warn SDL abi if disabled (2018-04-10 11:22:03 +0200)


configure: don't warn on old sdl/gtk versions if disabled.
keymap + gtk fixes.



Daniel P. Berrangé (1):
  ui: fix keymap detection under Xwayland

Gerd Hoffmann (1):
  gtk: drop pointless code from gd_window_close

Peter Xu (2):
  configure: don't warn GTK if disabled
  configure: don't warn SDL abi if disabled

 configure | 110 +++---
 ui/gtk.c  |   8 -
 ui/x_keymap.c |   7 ++--
 3 files changed, 63 insertions(+), 62 deletions(-)

-- 
2.9.3

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 11:16:01AM +0200, Laszlo Ersek wrote:
> On 04/10/18 08:27, Gerd Hoffmann wrote:
> >   Hi,
> > 
> >> - I considered adding wildcards (say, blacklist "all" i440fx machtypes,
> >> present and future, for SMM-requiring OVMF builds), but then you get
> >> into version sorting and similar mess. I considered fnmatch() --
> >> basically simple ? and * wildcards -- but that's not expressive enough.
> > 
> > I'd suggest whitelist with wildcards.  So the smm builds would get
> > "pc-q35-*".
> > 
> > libvirt knows about aliases, so it should be able to handle the "q35"
> > shortcut like "pc-q35-${latest}".
> > 
> > Or do you see another issue?
> 
> Well, one issue I see is version sorting; I should say "Q35 but no
> earlier than 2.4", and lexicographically, "2.11" sorts before "2.4".
> 
> Anyway (also asking for Thomas's input here): if we run with your idea
> to refer to exact mapping methods / firmware *implementation* types that
> we know libvirt implements / supports as a "white box", do we still deem
> machine type identification necessary? Because, libvirt already knows
> (for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just
> have to make a *reference* to that knowledge in the JSON file.
> 
> And, really, this seems to reinforce my point that the schema should
> live in the libvirtd tree, not in the QEMU tree. In that case, perhaps
> it would be a better fit to work with an XSD, and firmware packages
> should install XML files? Personally I'm a lot more attracted to
> XML/XSD; I think the tooling is better too. I just don't see how QEMU is
> involved.

This is defining a set of metadata that is required to use various firmware
files in combination with QEMU, along with defining a mapping to QEMU command
line arguments and/or features. Essentially, while I wish everyone used
libvirt, libvirt is not the only thing that manages QEMU. This information is
relevant to anyone managing QEMU, so it doesn't belong in libvirt's realm,
it is clear QEMU is best placed to declare this information.


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

[Qemu-devel] [PULL 3/4] configure: don't warn GTK if disabled

[Gerd Hoffmann]

From: Peter Xu 

We don't need to detect GTK ABI if GTK is disabled in general.
Otherwise we could get this warning (when host is installed with GTK ABI
version 2) even when configure with "--disable-gtk":

WARNING: Use of GTK 2.0 is deprecated and will be removed in
WARNING: future releases. Please switch to using GTK 3.0

CC: Paolo Bonzini 
CC: Gerd Hoffmann 
CC: Peter Maydell 
CC: Fam Zheng 
CC: "Philippe Mathieu-Daudé" 
Signed-off-by: Peter Xu 
Reviewed-by: Daniel P. Berrangé 
Message-id: 20180409082323.29575-1-pet...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 configure | 23 +++
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/configure b/configure
index 752dd9ef32..931d2a07cc 100755
--- a/configure
+++ b/configure
@@ -2540,19 +2540,18 @@ fi
 ##
 # GTK probe
 
-if test "$gtkabi" = ""; then
-# The GTK ABI was not specified explicitly, so try whether 3.0 is 
available.
-# Use 2.0 as a fallback if that is available.
-if $pkg_config --exists "gtk+-3.0 >= 3.0.0"; then
-gtkabi=3.0
-elif $pkg_config --exists "gtk+-2.0 >= 2.18.0"; then
-gtkabi=2.0
-else
-gtkabi=3.0
-fi
-fi
-
 if test "$gtk" != "no"; then
+if test "$gtkabi" = ""; then
+# The GTK ABI was not specified explicitly, so try whether 3.0 is 
available.
+# Use 2.0 as a fallback if that is available.
+if $pkg_config --exists "gtk+-3.0 >= 3.0.0"; then
+gtkabi=3.0
+elif $pkg_config --exists "gtk+-2.0 >= 2.18.0"; then
+gtkabi=2.0
+else
+gtkabi=3.0
+fi
+fi
 gtkpackage="gtk+-$gtkabi"
 gtkx11package="gtk+-x11-$gtkabi"
 if test "$gtkabi" = "3.0" ; then
-- 
2.9.3

[Qemu-devel] [PULL 4/4] configure: don't warn SDL abi if disabled

[Gerd Hoffmann]

From: Peter Xu 

SDL has the same problem as GTK that we might get warnings on SDL ABI
version even if SDL is disabled.  Fix that by only probing SDL if SDL is
enabled.  Also this should let configure be a little bit faster since we
don't really need to probe SDL stuff when it's off.

CC: Paolo Bonzini 
CC: Gerd Hoffmann 
CC: Peter Maydell 
CC: Daniel P. Berrange 
CC: Fam Zheng 
CC: "Philippe Mathieu-Daudé" 
Signed-off-by: Peter Xu 
Reviewed-by: Daniel P. Berrangé 
Reviewed-by: Fam Zheng 
Message-id: 20180410054034.20479-1-pet...@redhat.com
Signed-off-by: Gerd Hoffmann 
---
 configure | 87 ++-
 1 file changed, 47 insertions(+), 40 deletions(-)

diff --git a/configure b/configure
index 931d2a07cc..0a19b033bc 100755
--- a/configure
+++ b/configure
@@ -2835,49 +2835,52 @@ fi
 # Look for sdl configuration program (pkg-config or sdl-config).  Try
 # sdl-config even without cross prefix, and favour pkg-config over sdl-config.
 
-if test "$sdlabi" = ""; then
-if $pkg_config --exists "sdl2"; then
-sdlabi=2.0
-elif $pkg_config --exists "sdl"; then
-sdlabi=1.2
-else
-sdlabi=2.0
+sdl_probe ()
+{
+  sdl_too_old=no
+  if test "$sdlabi" = ""; then
+  if $pkg_config --exists "sdl2"; then
+  sdlabi=2.0
+  elif $pkg_config --exists "sdl"; then
+  sdlabi=1.2
+  else
+  sdlabi=2.0
+  fi
+  fi
+
+  if test $sdlabi = "2.0"; then
+  sdl_config=$sdl2_config
+  sdlname=sdl2
+  sdlconfigname=sdl2_config
+  elif test $sdlabi = "1.2"; then
+  sdlname=sdl
+  sdlconfigname=sdl_config
+  else
+  error_exit "Unknown sdlabi $sdlabi, must be 1.2 or 2.0"
+  fi
+
+  if test "$(basename $sdl_config)" != $sdlconfigname && ! has ${sdl_config}; 
then
+sdl_config=$sdlconfigname
+  fi
+
+  if $pkg_config $sdlname --exists; then
+sdlconfig="$pkg_config $sdlname"
+sdlversion=$($sdlconfig --modversion 2>/dev/null)
+  elif has ${sdl_config}; then
+sdlconfig="$sdl_config"
+sdlversion=$($sdlconfig --version)
+  else
+if test "$sdl" = "yes" ; then
+  feature_not_found "sdl" "Install SDL2-devel"
 fi
-fi
-
-if test $sdlabi = "2.0"; then
-sdl_config=$sdl2_config
-sdlname=sdl2
-sdlconfigname=sdl2_config
-elif test $sdlabi = "1.2"; then
-sdlname=sdl
-sdlconfigname=sdl_config
-else
-error_exit "Unknown sdlabi $sdlabi, must be 1.2 or 2.0"
-fi
-
-if test "$(basename $sdl_config)" != $sdlconfigname && ! has ${sdl_config}; 
then
-  sdl_config=$sdlconfigname
-fi
-
-if $pkg_config $sdlname --exists; then
-  sdlconfig="$pkg_config $sdlname"
-  sdlversion=$($sdlconfig --modversion 2>/dev/null)
-elif has ${sdl_config}; then
-  sdlconfig="$sdl_config"
-  sdlversion=$($sdlconfig --version)
-else
-  if test "$sdl" = "yes" ; then
-feature_not_found "sdl" "Install SDL2-devel"
+sdl=no
+# no need to do the rest
+return
+  fi
+  if test -n "$cross_prefix" && test "$(basename "$sdlconfig")" = sdl-config; 
then
+echo warning: using "\"$sdlconfig\"" to detect cross-compiled sdl >&2
   fi
-  sdl=no
-fi
-if test -n "$cross_prefix" && test "$(basename "$sdlconfig")" = sdl-config; 
then
-  echo warning: using "\"$sdlconfig\"" to detect cross-compiled sdl >&2
-fi
 
-sdl_too_old=no
-if test "$sdl" != "no" ; then
   cat > $TMPC << EOF
 #include 
 #undef main /* We don't want SDL to override our main() */
@@ -2919,6 +2922,10 @@ EOF
 fi
 sdl=no
   fi # sdl compile test
+}
+
+if test "$sdl" != "no" ; then
+  sdl_probe
 fi
 
 if test "$sdl" = "yes" ; then
-- 
2.9.3

Re: [Qemu-devel] [PULL for-2.12 0/8] s390x fixes for -rc3

[Peter Maydell]

On 9 April 2018 at 16:15, Cornelia Huck  wrote:
> The following changes since commit 2a6bcfdebe4115993a032395d459f5e0cf27a01e:
>
>   Merge remote-tracking branch 'remotes/famz/tags/testing-pull-request' into 
> staging (2018-04-09 10:21:14 +0100)
>
> are available in the Git repository at:
>
>   git://github.com/cohuck/qemu tags/s390x-20180409
>
> for you to fetch changes up to b3a184f51fbed3401694a9619e6842f882a64fee:
>
>   s390x: load_psw() should only exchange the PSW for KVM (2018-04-09 13:59:06 
> +0200)
>
> 
> Fixes for s390x: kvm, vfio-ccw, ipl code, bios. Includes a rebuild
> of s390-ccw.img and s390-netboot.img.
>
> 

Applied, thanks.

-- PMM

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 09:33, Thomas Huth wrote:
> On 09.04.2018 18:50, Laszlo Ersek wrote:
>> On 04/09/18 10:19, Gerd Hoffmann wrote:
> +{ 'enum' : 'SystemFirmwareType',
> +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }

 The naming here is quite a bad mixture between firmware interface
 ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There
 could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ...
 so I'd suggest to rather name them 'seabios' and 'edk2' here instead.
>>>
>>> uboot for example implements uefi unterfaces too (dunno how complete,
>>> but reportly recent versions can run uefi shell and grub just fine).
>>
>> Indeed: when I was struggling with this enum type and tried to look for
>> more firmware types to add, my googling turned up the "UEFI on Top of
>> U-Boot" whitepaper, from Alex and Andreas :)
>>
>> Again, this reaches to the root of the problem: when a user creates a
>> new domain, using high-level tools, they just want to tick "UEFI". (Dan
>> has emphasized this to me several times, so I think I get the idea by
>> now, if not the full environment.) We cannot ask the user, "please be
>> more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?"
> 
> But you are designing a rather low-level interface here, which should
> IMHO rather be precise than fuzzy. So should this "just want to tick
> UEFI" rather be handled in the high-level tools instead?
> 
> Alternatively, what about providing some kind of "alias" or "nickname"
> setting here, too? So the EDK2 builds would get
> SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example.

I hope I understand you right -- I think your suggestion ties in with my
other email I just sent in this thread. So, we could tell libvirtd,
"this firmware is of type 'UEFI', and you must use the 'ovmf_smm'
mapping method to run it, with this file or that file as varstore template".

We could even describe the parameters for this or that mapping method
structurally in the schema (in a discriminated union in QAPI JSON, or in
an XSD choice element). For example, "ovmf" and "ovmf_smm" would both
take "OvmfSplitFileOptions" -- a list of single varstore template files
with feature enum contants attached  --, while "SeaBiosOptions" would be
an empty structure.

I feel the key question here is whether we are allowed to directly
reference a mapping method we know libvirt implements. If we are, that
makes things a lot clearer (and easier, I should hope).

Thanks
Laszlo

Re: [Qemu-devel] [Qemu-arm] Crash when running hello-world unikernel for ARM

[Peter Maydell]

On 10 April 2018 at 09:16, Ajay Garg  wrote:
> On Tue, Apr 10, 2018 at 1:08 PM, Peter Maydell  
> wrote:
>> What hardware (what CPU, board, etc) is this "rumprun" software
>> expecting to run on?
>
> Yep, just to ensure that there are no cross-compiling issues, I am
> building rumprun on the pseudo-real hardware itself.
> In our case, the pseudo-real hardware are :
>
> a)
> An ARM32 "virt" hardware/machine in a qemu environment
> (https://translatedcode.wordpress.com/2016/11/03/installing-debian-on-qemus-32-bit-arm-virt-board/)
>
> Once I start  this machine, all environment is arm32, and I compile
> rumprun within this environemnt without any cross-compiling.
>
> b)
> A beaglebone-green-wireless.
> This is a arm32 machine bottoms-up, so no question of cross-compiling
> whatsoever here :)
>
> In both cases, I then use qemu-system-arm (on the "virt" machine, and
> beaglebone-green-wireless itself).

That's telling me what setups you're trying to compile in,
which doesn't correspond necessarily to what the guest
OS is built to run on.

> One query : It is apparent that there is nested qemu-virtualization in
> step a), could that be an issue?

Why are you running this in a nested setup? I don't understand
the purpose of doing that. It would be simpler and faster to
just run the guest on a QEMU running in your native host system.

Assuming this is the source for the guest you're trying to run:

https://github.com/rumpkernel/rumprun/tree/master/platform/hw/arch

that suggests that the only Arm board it supports is "integrator"
(which is an absolutely ancient devboard with very little memory,
no PCI and no virtio support). You need to confirm what Arm hardware
this 'rumpkernel' is actually intended to run on, and then give QEMU
the right command line arguments to emulate that hardware. I can't
really help any further, I'm afraid -- you need somebody who knows
about this guest OS.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 for-2.13] vfio-ccw: introduce vfio_ccw_get_device()

[Cornelia Huck]

On Mon, 09 Apr 2018 12:15:10 +0200
Greg Kurz  wrote:

> A recent patch fixed leaks of the dynamically allocated vcdev->vdev.name
> field in vfio_ccw_realize(), but we now have three freeing sites for it.
> This is unfortunate and seems to indicate something is wrong with its
> life cycle.
> 
> The root issue is that vcdev->vdev.name is set before vfio_get_device()
> is called, which theoretically prevents to call vfio_put_device() to
> do the freeing. Well actually, we could call it anyway  because
> vfio_put_base_device() is a nop if the device isn't attached, but this
> would be confusing.
> 
> This patch hence moves all the logic of attaching the device, including
> the "already attached" check, to a separate vfio_ccw_get_device() function,
> counterpart of vfio_put_device(). While here, vfio_put_device() is renamed
> to vfio_ccw_put_device() for consistency.
> 
> Based-on: <152311222681.203086.8874800175539040298.stgit@bahia>
> Signed-off-by: Greg Kurz 
> ---
> v3: - make vfio_ccw_get_device() void and check err in caller
> ---
>  hw/vfio/ccw.c |   56 
>  1 file changed, 36 insertions(+), 20 deletions(-)

Thanks, applied.

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Thomas Huth]

On 10.04.2018 11:05, Daniel P. Berrangé wrote:
> On Mon, Apr 09, 2018 at 06:34:41PM +0200, Laszlo Ersek wrote:
>> On 04/09/18 09:26, Thomas Huth wrote:
>>>  Hi Laszlo,
>>>
>>> On 07.04.2018 02:01, Laszlo Ersek wrote:
 Add a schema that describes the properties of virtual machine firmware.

 Each firmware executable installed on a host system should come with a
 JSON file that conforms to this schema, and informs the management
 applications about the firmware's properties.

 In addition, a configuration directory with symlinks to the JSON files
 should exist, with the symlinks carefully named to reflect a priority
 order. Management applications can then search this directory in priority
 order for the first firmware executable that satisfies their search
 criteria. The found JSON file provides the management layer with domain
 configuration bits that are required to run the firmware binary.
>>> [...]
 +##
 +# @FirmwareDevice:
 +#
 +# Defines the device types that a firmware file can be mapped into.
 +#
 +# @memory: The firmware file is to be mapped into memory.
 +#
 +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
 +#  similar to @memory but may imply additional processing that is
 +#  specific to the target architecture.
 +#
 +# @flash: The firmware file is to be mapped into a pflash chip.
 +#
 +# Since: 2.13
 +##
 +{ 'enum' : 'FirmwareDevice',
 +  'data' : [ 'memory', 'kernel', 'flash' ] }
>>>
>>> This is not fully clear to me... what is this exactly good for? Is this
>>> a way to say how the firmware should be loaded, i.e. via "-bios",
>>> "-kernel" or "-pflash" parameter? If so, the term "memory" is quite
>>> misleading since files that are loaded via -bios can also end up in an
>>> emulated ROM chip.
>>
>> I threw in "-kernel" because, although it also (usually?) means
>> "memory", I expected people would want it separate.
> 
> What platform / scenario actually uses -kernel to load firmware.

I think uboot uses -kernel in certain cases, see e.g.:

https://balau82.wordpress.com/2010/03/10/u-boot-for-arm-on-qemu/

> If you
> have loaded firmware using -kernel, how do you then load the actual
> kernel ?

The kernel is then loaded from disk or network or another boot device.

 Thomas

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Mon, Apr 09, 2018 at 07:57:54PM +0200, Laszlo Ersek wrote:
> On 04/09/18 10:49, Daniel P. Berrangé wrote:
> > On Sat, Apr 07, 2018 at 02:01:17AM +0200, Laszlo Ersek wrote:
> >> Add a schema that describes the properties of virtual machine firmware.
> >>
> >> Each firmware executable installed on a host system should come with a
> >> JSON file that conforms to this schema, and informs the management
> >> applications about the firmware's properties.
> >>
> >> In addition, a configuration directory with symlinks to the JSON files
> >> should exist, with the symlinks carefully named to reflect a priority
> >> order. Management applications can then search this directory in priority
> >> order for the first firmware executable that satisfies their search
> >> criteria. The found JSON file provides the management layer with domain
> >> configuration bits that are required to run the firmware binary.
> >>
> >> Cc: "Daniel P. Berrange" 
> >> Cc: Alexander Graf 
> >> Cc: Ard Biesheuvel 
> >> Cc: David Gibson 
> >> Cc: Eric Blake 
> >> Cc: Gary Ching-Pang Lin 
> >> Cc: Gerd Hoffmann 
> >> Cc: Kashyap Chamarthy 
> >> Cc: Markus Armbruster 
> >> Cc: Michael Roth 
> >> Cc: Michal Privoznik 
> >> Cc: Peter Krempa 
> >> Cc: Peter Maydell 
> >> Cc: Thomas Huth 
> >> Signed-off-by: Laszlo Ersek 
> >> ---
> >>
> >> Notes:
> >> Folks on the CC list, please try to see if the suggested schema is
> >> flexible enough to describe the virtual firmware(s) that you are
> >> familiar with. Thanks!
> >>
> >>  Makefile  |   9 ++
> >>  Makefile.objs |   4 +
> >>  qapi/firmware.json| 343 
> >> ++
> >>  qapi/qapi-schema.json |   1 +
> >>  qmp.c |   5 +
> >>  .gitignore|   4 +
> >>  6 files changed, 366 insertions(+)
> >>  create mode 100644 qapi/firmware.json
> >>
> > 
> >> diff --git a/qapi/firmware.json b/qapi/firmware.json
> >> new file mode 100644
> >> index ..f267240f44dd
> >> --- /dev/null
> >> +++ b/qapi/firmware.json
> >> @@ -0,0 +1,343 @@
> >> +# -*- Mode: Python -*-
> >> +
> >> +##
> >> +# = Firmware
> >> +##
> >> +
> >> +##
> >> +# @FirmwareDevice:
> >> +#
> >> +# Defines the device types that a firmware file can be mapped into.
> >> +#
> >> +# @memory: The firmware file is to be mapped into memory.
> >> +#
> >> +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
> >> +#  similar to @memory but may imply additional processing that is
> >> +#  specific to the target architecture.
> >> +#
> >> +# @flash: The firmware file is to be mapped into a pflash chip.
> >> +#
> >> +# Since: 2.13
> >> +##
> >> +{ 'enum' : 'FirmwareDevice',
> >> +  'data' : [ 'memory', 'kernel', 'flash' ] }
> >> +
> >> +##
> >> +# @FirmwareAccess:
> >> +#
> >> +# Defines the possible permissions for a given access mode to a device 
> >> that
> >> +# maps a firmware file.
> >> +#
> >> +# @denied: The access is denied.
> >> +#
> >> +# @permitted: The access is permitted.
> >> +#
> >> +# @restricted-to-secure-context: The access is permitted for guest code 
> >> that
> >> +#runs in a secure context; otherwise the 
> >> access
> >> +#is denied. The definition of "secure 
> >> context"
> >> +#is specific to the target architecture.
> >> +#
> >> +# Since: 2.13
> >> +##
> >> +{ 'enum' : 'FirmwareAccess',
> >> +  'data' : [ 'denied', 'permitted', 'restricted-to-secure-context' ] }
> > 
> > I'm not really understanding the purpose of this - what does it map to
> > on the command line ?
> 
> That's difficult to answer generally, because -bios and -kernel have
> different meanings per board type. So I didn't aim at command line
> switches here; instead I tried to capture where and how the firmware
> wants to "end up" in the virtual hardware. How that maps to a particular
> board is a separate question.

I tend to think that defining a mapping to command line arguments is a key
feature that this should cover. Even if there variations across boards, QEMU
still has a small finite set of approaches to configure firmware, so it does
not feel unreasonable to define what they are and how they map to thes firmware
files.

Your FirmwareDevice enum above with "memory", "kernel" and "flash" has
pretty much suggested the -bios, -kernel or -drive if=flash args anway

> So, the schema intends to describe the mapping that the firmware expects
> from the board. How that is implemented on the QEMU command line is left
> as an exercise to ... libvirtd. :)

I think this is pretty unhelpful. Essentially that is saying here is a big
pile of information about firmware, but we're not going to tell you how to
use it correctly, everyone must figure it out themselves.


> >> +##
> >> +# @FirmwareFile:
> >> +#
> >> +# Gathers the common traits of system firmware executables and NVRAM 
> >> templates.
> >> +#
> >> +# @pathname: absolute p

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 08:27, Gerd Hoffmann wrote:
>   Hi,
> 
>> - I considered adding wildcards (say, blacklist "all" i440fx machtypes,
>> present and future, for SMM-requiring OVMF builds), but then you get
>> into version sorting and similar mess. I considered fnmatch() --
>> basically simple ? and * wildcards -- but that's not expressive enough.
> 
> I'd suggest whitelist with wildcards.  So the smm builds would get
> "pc-q35-*".
> 
> libvirt knows about aliases, so it should be able to handle the "q35"
> shortcut like "pc-q35-${latest}".
> 
> Or do you see another issue?

Well, one issue I see is version sorting; I should say "Q35 but no
earlier than 2.4", and lexicographically, "2.11" sorts before "2.4".

Anyway (also asking for Thomas's input here): if we run with your idea
to refer to exact mapping methods / firmware *implementation* types that
we know libvirt implements / supports as a "white box", do we still deem
machine type identification necessary? Because, libvirt already knows
(for example) that "ovmf_smm" requires pc-q35-2.4 or later. So we just
have to make a *reference* to that knowledge in the JSON file.

And, really, this seems to reinforce my point that the schema should
live in the libvirtd tree, not in the QEMU tree. In that case, perhaps
it would be a better fit to work with an XSD, and firmware packages
should install XML files? Personally I'm a lot more attracted to
XML/XSD; I think the tooling is better too. I just don't see how QEMU is
involved.

Opinions please :)
Thanks!
Laszlo

Re: [Qemu-devel] [PATCH] migration: Don't activate block devices if using -S

[Kevin Wolf]

Am 10.04.2018 um 10:45 hat Dr. David Alan Gilbert geschrieben:
> * Kevin Wolf (kw...@redhat.com) wrote:
> > Am 10.04.2018 um 09:36 hat Jiri Denemark geschrieben:
> > > On Mon, Apr 09, 2018 at 15:40:03 +0200, Kevin Wolf wrote:
> > > > Am 09.04.2018 um 12:27 hat Dr. David Alan Gilbert geschrieben:
> > > > > It's a fairly hairy failure case they had; if I remember correctly 
> > > > > it's:
> > > > >   a) Start migration
> > > > >   b) Migration gets to completion point
> > > > >   c) Destination is still paused
> > > > >   d) Libvirt is restarted on the source
> > > > >   e) Since libvirt was restarted it fails the migration (and hence 
> > > > > knows
> > > > >  the destination won't be started)
> > > > >   f) It now tries to resume the qemu on the source
> > > > > 
> > > > > (f) fails because (b) caused the locks to be taken on the destination;
> > > > > hence this patch stops doing that.  It's a case we don't really think
> > > > > about - i.e. that the migration has actually completed and all the 
> > > > > data
> > > > > is on the destination, but libvirt decides for some other reason to
> > > > > abandon migration.
> > > > 
> > > > If you do remember correctly, that scenario doesn't feel tricky at all.
> > > > libvirt needs to quit the destination qemu, which will inactivate the
> > > > images on the destination and release the lock, and then it can continue
> > > > the source.
> > > > 
> > > > In fact, this is so straightforward that I wonder what else libvirt is
> > > > doing. Is the destination qemu only shut down after trying to continue
> > > > the source? That would be libvirt using the wrong order of steps.
> > > 
> > > There's no connection between the two libvirt daemons in the case we're
> > > talking about so they can't really synchronize the actions. The
> > > destination daemon will kill the new QEMU process and the source will
> > > resume the old one, but the order is completely random.
> > 
> > Hm, okay...
> > 
> > > > > Yes it was a 'block-activate' that I'd wondered about.  One 
> > > > > complication
> > > > > is that if this now under the control of the management layer then we
> > > > > should stop asserting when the block devices aren't in the expected
> > > > > state and just cleanly fail the command instead.
> > > > 
> > > > Requiring an explicit 'block-activate' on the destination would be an
> > > > incompatible change, so you would have to introduce a new option for
> > > > that. 'block-inactivate' on the source feels a bit simpler.
> > > 
> > > As I said in another email, the explicit block-activate command could
> > > depend on a migration capability similarly to how pre-switchover state
> > > works.
> > 
> > Yeah, that's exactly the thing that we wouldn't need if we could use
> > 'block-inactivate' on the source instead. It feels a bit wrong to
> > design a more involved QEMU interface around the libvirt internals,
> 
> It's not necessarily 'libvirt internals' - it's a case of them having to
> cope with recovering from failures that happen around migration; it's
> not an easy problem, and if they've got a way to stop both sides running
> at the same time that's pretty important.

The 'libvirt internals' isn't that it needs an additional state where
neither source nor destination QEMU own the images, but that it has to
be between migration completion and image activation on the destination
rather than between image inactivation on the source and migration
completion. The latter would be much easier for qemu, but apparently it
doesn't work for libvirt because of how it works internally.

But as I said, I'd just implement both for symmetry and then management
tools can pick whatever makes their life easier.

> > but
> > as long as we implement both sides for symmetry and libvirt just happens
> > to pick the destination side for now, I think it's okay.
> > 
> > By the way, are block devices the only thing that need to be explicitly
> > activated? For example, what about qemu_announce_self() for network
> > cards, do we need to delay that, too?
> > 
> > In any case, I think this patch needs to be reverted for 2.12 because
> > it's wrong, and then we can create the proper solution in the 2.13
> > timefrage.
> 
> what case does this break?
> I'm a bit wary of reverting this, which fixes a known problem, on the
> basis that it causes a theoretical problem.

It breaks the API. And the final design we're having in mind now is
compatible with the old API, not with the new one exposed by this patch,
so that switch would break the API again to get back to the old state.

Do you know all the scripts that people are using around QEMU? I don't,
but I know that plenty of them exist, so I don't think we can declare
this API breakage purely theoretical.

Yes, the patch fixes a known problem, but also a problem that is a rare
corner case error that you can only hit with really bad timing. Do we
really want to risk unconditionally breaking success cases for fixing a
mostly theoretical corn

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Mon, Apr 09, 2018 at 06:50:12PM +0200, Laszlo Ersek wrote:
> On 04/09/18 10:19, Gerd Hoffmann wrote:
> >>> +{ 'enum' : 'SystemFirmwareType',
> >>> +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
> >>
> >> The naming here is quite a bad mixture between firmware interface
> >> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There
> >> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ...
> >> so I'd suggest to rather name them 'seabios' and 'edk2' here instead.
> > 
> > uboot for example implements uefi unterfaces too (dunno how complete,
> > but reportly recent versions can run uefi shell and grub just fine).
> 
> Indeed: when I was struggling with this enum type and tried to look for
> more firmware types to add, my googling turned up the "UEFI on Top of
> U-Boot" whitepaper, from Alex and Andreas :)
> 
> Again, this reaches to the root of the problem: when a user creates a
> new domain, using high-level tools, they just want to tick "UEFI". (Dan
> has emphasized this to me several times, so I think I get the idea by
> now, if not the full environment.) We cannot ask the user, "please be
> more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?"
> 
> Instead, each of those firmware images will have to come with a JSON
> document that states "uefi" in SystemFirmware.type, and the host admin
> will be responsible for establishing a priority order between them.
> Then, when the user asks for "UEFI" (and no more details), they'll get
> (compatibly with the target architecture) whichever firmware the host
> admin marked as "higher priority".

Yep, I don't think there's any problem here. If they have asked for
"uefi", they'll get whichever UEFI implementation is the default for
that host. Today it'll be an EDK2 impl, but if there's a uboot impl
of UEFI available instead, that's fine too. If both are available
we'll have some deterministic manner in which we pick one, even if it
as simple as which has alphabetically first filename.

This is really only about getting good default choices. If the user
really badly wants to have a specific firmware, they can still provide
a path to it themselves instead of having libvirt choose it.

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

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 08:18, Gerd Hoffmann wrote:
>   Hi,
> 
>>> uboot for example implements uefi unterfaces too (dunno how complete,
>>> but reportly recent versions can run uefi shell and grub just fine).
>>
>> Indeed: when I was struggling with this enum type and tried to look for
>> more firmware types to add, my googling turned up the "UEFI on Top of
>> U-Boot" whitepaper, from Alex and Andreas :)
> 
> In case you wanna play: uboot supports x86 qemu meanwhile, so you can
> try install u-boot.git-x86 from my firmware repo, then run
> "qemu-system-x86_64 -bios /usr/share/u-boot.git/x86/qemu-pc/u-boot.rom".
> 
> It certainly isn't a useful edk2 replacement atm.  It has no virtio
> drivers.  And even when using ide storage its not like it would happily
> boot a fedora live iso.  So I certainly wouldn't tag that as uefi today.
> That might change at some point in the future though.
> 
>> Again, this reaches to the root of the problem: when a user creates a
>> new domain, using high-level tools, they just want to tick "UEFI". (Dan
>> has emphasized this to me several times, so I think I get the idea by
>> now, if not the full environment.) We cannot ask the user, "please be
>> more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?"
> 
> Well, in case the uefi support in u-boot is good enough some day then it
> doesn't matter to the user whenever uboot or edk2 boots the efi guest
> from disk/iso, right?

I believe that's correct.

Laszlo

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 07:59, Gerd Hoffmann wrote:
>   Hi,
> 
>> I threw in "-kernel" because, although it also (usually?) means
>> "memory", I expected people would want it separate.
>>
>> Regarding memory vs. pflash, I thought that these two, combined with the
>> access permissions, could cover all of RAM, ROM, and read-only and
>> read-write pflash too.
>>
>> So, "-bios" (-> ROM) boils down to "memory", with write access denied --
>> please see the SeaBIOS example near the end.
> 
> Hmm, I'm wondering whenever it is useful to model things this way.  It's
> not like you can actually configure things for -bios seabios.rom or
> -kernel uboot.elf.  Only pflash allows to actually configure things, and
> there are not that many useful combinations.  The code needs
> Read+Execute.  Allowing Write could be useful in theory, to allow the
> guest doing firmware updates.  But I think nobody actually does that, so
> in practice it is fixed.  The varstore can have different permissions,
> but it's only two useful combinations.  Either allow access
> unconditionally, or allow access in secure contect (aka smm) only.

(I hope I understand your point right:)

I'm also fine if we simply define a fixed (but extensible) set of
mapping methods, basically a new enum type, that simply tells libvirtd
what this firmware *is*. IOW, directly reference a mapping method we
know libvirt implements, rather than give vague hints.

This could repurpose SystemFirmwareType, but it should become more
detailed. I'm thinking like:
- ovmf: split files without requiring SMM
- ovmf_smm: split files with SMM requirement
- seabios: exactly that
- ... other things others suggest.

So "ovmf" would refer precisely to point (3) in my email
<3381bdf1-62ea-9da7-c654-032c0c11fb4e@redhat.com">http://mid.mail-archive.com/3381bdf1-62ea-9da7-c654-032c0c11fb4e@redhat.com>,
and "ovmf_smm" would refer to point (4) in that email.

Let me post the next version soon with this idea, focusing just on OVMF
and maybe SeaBIOS. Then let us see if that RFCv2 format lends itself
easily to extensions by Thomas. :)

Thanks!
Laszlo

[Qemu-devel] [Bug 657329] Re: APIC unusable on QEMU

[Thomas Huth]

Looking through old bug tickets... can you still reproduce this issue
with the latest version of QEMU? Or could we close this ticket nowadays?


** Changed in: qemu
   Status: New => Incomplete

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

Title:
  APIC unusable on QEMU

Status in QEMU:
  Incomplete

Bug description:
  The APIC is unusable with QEMU using x86-64 system emulation.  Problem
  exists in the latest stable QEMU 0.12.5 as well as the latest git
  head.  I am using Mac OS X 10.6, 64-bit version of QEMU.

  The QEMU binary was configured with:

   ./configure --target-list=i386-softmmu,x86_64-softmmubck-i-search:
  conf_

  Problem is that the hw/apic.c file (as well as a few other naughty
  files) rely on the cpu_single_env global - which is set to NULL in
  cpu-exec.c.

  Below is a test reading the local APIC version register:

  Before taking it out:

  (qemu) xp 0xfee00030
  fee00030: 0x
  (qemu)

  After:

  (qemu) xp 0xfee00030
  fee00030: 0x00050011
  (qemu)

  Quick fix below.  I don't know if there are any side effects with
  this, if this is OK maybe we can fix it like this for the stable
  versions and fix the HEAD to not rely on the cpu_single_env global.

  diff --git a/cpu-exec.c b/cpu-exec.c
  index dbdfdcc..3e966d7 100644
  --- a/cpu-exec.c
  +++ b/cpu-exec.c
  @@ -674,7 +674,17 @@ int cpu_exec(CPUState *env1)
   env = (void *) saved_env_reg;
   
   /* fail safe : never use cpu_single_env outside cpu_exec() */
  +#warning fixup devices which rely on this
  +#if 0
  +/*
  + * Hello.  This is wrapped around an #if 0 ... #endif because that's
  + * what should happen.  However, certain naughty devices (like the APIC
  + * for instance, and a few others), access this global variable.
  + *
  + * So this is here for now ... until we fix up those devices.
  + */
   cpu_single_env = NULL;
  +#endif
   return ret;
   }

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

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Daniel P . Berrangé]

On Mon, Apr 09, 2018 at 06:34:41PM +0200, Laszlo Ersek wrote:
> On 04/09/18 09:26, Thomas Huth wrote:
> >  Hi Laszlo,
> > 
> > On 07.04.2018 02:01, Laszlo Ersek wrote:
> >> Add a schema that describes the properties of virtual machine firmware.
> >>
> >> Each firmware executable installed on a host system should come with a
> >> JSON file that conforms to this schema, and informs the management
> >> applications about the firmware's properties.
> >>
> >> In addition, a configuration directory with symlinks to the JSON files
> >> should exist, with the symlinks carefully named to reflect a priority
> >> order. Management applications can then search this directory in priority
> >> order for the first firmware executable that satisfies their search
> >> criteria. The found JSON file provides the management layer with domain
> >> configuration bits that are required to run the firmware binary.
> > [...]
> >> +##
> >> +# @FirmwareDevice:
> >> +#
> >> +# Defines the device types that a firmware file can be mapped into.
> >> +#
> >> +# @memory: The firmware file is to be mapped into memory.
> >> +#
> >> +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
> >> +#  similar to @memory but may imply additional processing that is
> >> +#  specific to the target architecture.
> >> +#
> >> +# @flash: The firmware file is to be mapped into a pflash chip.
> >> +#
> >> +# Since: 2.13
> >> +##
> >> +{ 'enum' : 'FirmwareDevice',
> >> +  'data' : [ 'memory', 'kernel', 'flash' ] }
> > 
> > This is not fully clear to me... what is this exactly good for? Is this
> > a way to say how the firmware should be loaded, i.e. via "-bios",
> > "-kernel" or "-pflash" parameter? If so, the term "memory" is quite
> > misleading since files that are loaded via -bios can also end up in an
> > emulated ROM chip.
> 
> I threw in "-kernel" because, although it also (usually?) means
> "memory", I expected people would want it separate.

What platform / scenario actually uses -kernel to load firmware. If you
have loaded firmware using -kernel, how do you then load the actual
kernel ?


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

[Qemu-devel] [PATCH for-2.12] commit/stream: Reset delay_ns

[Kevin Wolf]

Streaming and the commit block job only want to apply throttling when
they actually copied data instead of skipping it, so they made the
calculation of delay_ns conditional. However, delay_ns isn't reset when
skipping some sectors, so instead of not waiting, the old delay is
applied again.

Properly reset delay_ns where needed.

Signed-off-by: Kevin Wolf 
---
 block/commit.c | 2 ++
 block/stream.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index ab4fa3c3cf..1432baeef4 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -202,6 +202,8 @@ static void coroutine_fn commit_run(void *opaque)
 
 if (copy && s->common.speed) {
 delay_ns = ratelimit_calculate_delay(&s->limit, n);
+} else {
+delay_ns = 0;
 }
 }
 
diff --git a/block/stream.c b/block/stream.c
index f3b53f49e2..1a85708fcf 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -188,6 +188,8 @@ static void coroutine_fn stream_run(void *opaque)
 s->common.offset += n;
 if (copy && s->common.speed) {
 delay_ns = ratelimit_calculate_delay(&s->limit, n);
+} else {
+delay_ns = 0;
 }
 }
 
-- 
2.13.6

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Laszlo Ersek]

On 04/10/18 09:44, Thomas Huth wrote:
> On 09.04.2018 18:34, Laszlo Ersek wrote:
>> On 04/09/18 09:26, Thomas Huth wrote:
>>>  Hi Laszlo,
>>>
>>> On 07.04.2018 02:01, Laszlo Ersek wrote:
 Add a schema that describes the properties of virtual machine firmware.

 Each firmware executable installed on a host system should come with a
 JSON file that conforms to this schema, and informs the management
 applications about the firmware's properties.

 In addition, a configuration directory with symlinks to the JSON files
 should exist, with the symlinks carefully named to reflect a priority
 order. Management applications can then search this directory in priority
 order for the first firmware executable that satisfies their search
 criteria. The found JSON file provides the management layer with domain
 configuration bits that are required to run the firmware binary.
>>> [...]
 +##
 +# @FirmwareDevice:
 +#
 +# Defines the device types that a firmware file can be mapped into.
 +#
 +# @memory: The firmware file is to be mapped into memory.
 +#
 +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
 +#  similar to @memory but may imply additional processing that is
 +#  specific to the target architecture.
 +#
 +# @flash: The firmware file is to be mapped into a pflash chip.
 +#
 +# Since: 2.13
 +##
 +{ 'enum' : 'FirmwareDevice',
 +  'data' : [ 'memory', 'kernel', 'flash' ] }
>>>
>>> This is not fully clear to me... what is this exactly good for? Is this
>>> a way to say how the firmware should be loaded, i.e. via "-bios",
>>> "-kernel" or "-pflash" parameter? If so, the term "memory" is quite
>>> misleading since files that are loaded via -bios can also end up in an
>>> emulated ROM chip.
>>
>> I threw in "-kernel" because, although it also (usually?) means
>> "memory", I expected people would want it separate.
>>
>> Regarding memory vs. pflash, I thought that these two, combined with the
>> access permissions, could cover all of RAM, ROM, and read-only and
>> read-write pflash too.
>>
>> So, "-bios" (-> ROM) boils down to "memory", with write access denied --
>> please see the SeaBIOS example near the end.
> 
> Let me ask the other way round: How does a high-level tool know whether
> it should use "-bios", "-kernel", "-pflash", "-device generic-loader" or
> "-younameit" for loading the firmware?

I expect it knows that because its developers investigate all the
supported firmware options and write dedicated code for those. My
understanding is that this JSON is not supposed to inform the mgmt layer
about unknown firmware, but to expose enough information for the mgmt
layer to pick a firmware and to compose a known & supported cmdline
config for it.


 +   'nvram-map' : 'FirmwareMapping',
 +   'templates' : [ 'FirmwareFile' ] } }
 +
 +##
 +# @SystemFirmwareType:
 +#
 +# Lists system firmware types commonly used with QEMU virtual machines.
 +#
 +# @bios: The system firmware was built from the SeaBIOS project.
 +#
 +# @slof: The system firmware was built from the Slimline Open Firmware 
 project.
 +#
 +# @uboot: The system firmware was built from the U-Boot project.
 +#
 +# @uefi: The system firmware was built from the edk2 (EFI Development Kit 
 II)
 +#project.
 +#
 +# Since: 2.13
 +##
 +{ 'enum' : 'SystemFirmwareType',
 +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
>>>
>>> The naming here is quite a bad mixture between firmware interface
>>> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There
>>> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ...
>>> so I'd suggest to rather name them 'seabios' and 'edk2' here instead.
>>
>> Sure, I'm totally ready to follow community advice here (too).
>>
>> In fact this is the one element I dislike the most about the schema --
>> it's the fuzziest part, yet it is the most important element for
>> libvirt. Because users and higher level apps just want to say "give me
>> UEFI". If I have to ask "OK, but UEFI built from edk2 or something
>> else?", then it's a lost cause already.
>>
>> It's hard to find the right level of abstraction in the naming when the
>> higher level tools (and/or ultimately the users) don't know enough to
>> ask for specifics -- I'm not saying that's bad; it's quite natural, but
>> makes things very difficult. So this enum aims to match the user story
>> "gimme UEFI and be done with it". I figure users will just utter the
>> most common buzzword form of the concept they have in mind. "edk2"
>> doesn't tell them as much as "uefi".
> 
> OK, I see your point. But I still think we should not design fuzzy
> interfaces here at this low level, this will only lead to other trouble
> later. ... thinking about this again, users seem to mix up firmware
>

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Change default cpu and gic-version setting to "max"

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 09:41:33AM +0200, Andrea Bolognani wrote:
> On Mon, 2018-04-09 at 11:29 -0500, Wei Huang wrote:
> > > > Running mach-virt machine types (i.e. "-M virt") on different systems 
> > > > can
> > > > result in various misleading warnings if -cpu and/or gic-version not 
> > > > specified.
> > > > For KVM, this can be solved mostly by using "host" type. But the "host" 
> > > > type
> > > > doesn't work for TCG. Compared with "host", the "max" type not only 
> > > > supports
> > > > auto detection under KVM mode, but also works with TCG. So this patch 
> > > > set
> > > > "max" as the default types for both -cpu and gic-version.
> > > 
> > > Hmm, generally we aim for the config provided by a machine type to be 
> > > stable
> > > across QEMU versions.
> > 
> > I understand this principle. But in reality, under KVM mode, the default
> > config most time doesn't work. If end users specify cpu type manually,
> > it still doesn't work because the host CPU is vendor-specific design
> > (e.g. "cortex-a57" doesn't work on QCOM's machine). So we end up with
> > using "-cpu host" all the time. My argument for this patch is that "-cpu
> > max" isn't worse than "-cpu host".
> 
> I figure the people not explicitly specifying a CPU model on the
> command line will probably also use '-M virt' instead of versioned
> machine types, which means they will get a different guest behavior
> after upgrading QEMU regardless.

Libvirt uses versioned machine types and does not specify -cpu unless the
user has added  to their XML. IOW libvirt assumes the default CPU
model is stable because that's what QEMU has promised in the past.

> Defaulting to 'max' for '-cpu' and 'gic-version' makes it convenient
> to quickly and concisely start a guest; if you care about guest ABI
> at all, then you are already specifying everything explicitly on the
> command line instead of relying on defaults - or using libvirt ;)

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

Re: [Qemu-devel] [PATCH] configure: don't warn SDL abi if disabled

[Fam Zheng]

On Tue, 04/10 13:40, Peter Xu wrote:
> SDL has the same problem as GTK that we might get warnings on SDL ABI
> version even if SDL is disabled.  Fix that by only probing SDL if SDL is
> enabled.  Also this should let configure be a little bit faster since we
> don't really need to probe SDL stuff when it's off.
> 
> CC: Paolo Bonzini 
> CC: Gerd Hoffmann 
> CC: Peter Maydell 
> CC: Daniel P. Berrange 
> CC: Fam Zheng 
> CC: "Philippe Mathieu-Daudé" 
> Signed-off-by: Peter Xu 
> ---
>  configure | 83 
> ++-
>  1 file changed, 45 insertions(+), 38 deletions(-)
> 
> diff --git a/configure b/configure
> index 752dd9ef32..f647026b8d 100755
> --- a/configure
> +++ b/configure
> @@ -2836,49 +2836,52 @@ fi
>  # Look for sdl configuration program (pkg-config or sdl-config).  Try
>  # sdl-config even without cross prefix, and favour pkg-config over 
> sdl-config.
>  
> -if test "$sdlabi" = ""; then
> -if $pkg_config --exists "sdl2"; then
> -sdlabi=2.0
> -elif $pkg_config --exists "sdl"; then
> -sdlabi=1.2
> -else
> -sdlabi=2.0
> -fi
> -fi
> +sdl_probe ()
> +{

We have had

function_name()
{
...

and

function_name() {
...

and

function_name () {
...

finally you invent the last in the family:

function_name ()
{

:)
.
> +  sdl_too_old=no
> +  if test "$sdlabi" = ""; then
> +  if $pkg_config --exists "sdl2"; then
> +  sdlabi=2.0
> +  elif $pkg_config --exists "sdl"; then
> +  sdlabi=1.2
> +  else
> +  sdlabi=2.0
> +  fi
> +  fi
>  
> -if test $sdlabi = "2.0"; then
> -sdl_config=$sdl2_config
> -sdlname=sdl2
> -sdlconfigname=sdl2_config
> -elif test $sdlabi = "1.2"; then
> -sdlname=sdl
> -sdlconfigname=sdl_config
> -else
> -error_exit "Unknown sdlabi $sdlabi, must be 1.2 or 2.0"
> -fi
> +  if test $sdlabi = "2.0"; then
> +  sdl_config=$sdl2_config
> +  sdlname=sdl2
> +  sdlconfigname=sdl2_config
> +  elif test $sdlabi = "1.2"; then
> +  sdlname=sdl
> +  sdlconfigname=sdl_config
> +  else
> +  error_exit "Unknown sdlabi $sdlabi, must be 1.2 or 2.0"
> +  fi
>  
> -if test "$(basename $sdl_config)" != $sdlconfigname && ! has ${sdl_config}; 
> then
> -  sdl_config=$sdlconfigname
> -fi
> +  if test "$(basename $sdl_config)" != $sdlconfigname && ! has 
> ${sdl_config}; then
> +sdl_config=$sdlconfigname
> +  fi
>  
> -if $pkg_config $sdlname --exists; then
> -  sdlconfig="$pkg_config $sdlname"
> -  sdlversion=$($sdlconfig --modversion 2>/dev/null)
> -elif has ${sdl_config}; then
> -  sdlconfig="$sdl_config"
> -  sdlversion=$($sdlconfig --version)
> -else
> -  if test "$sdl" = "yes" ; then
> -feature_not_found "sdl" "Install SDL2-devel"
> +  if $pkg_config $sdlname --exists; then
> +sdlconfig="$pkg_config $sdlname"
> +sdlversion=$($sdlconfig --modversion 2>/dev/null)
> +  elif has ${sdl_config}; then
> +sdlconfig="$sdl_config"
> +sdlversion=$($sdlconfig --version)
> +  else
> +if test "$sdl" = "yes" ; then
> +  feature_not_found "sdl" "Install SDL2-devel"
> +fi
> +sdl=no
> +# no need to do the rest
> +return
> +  fi
> +  if test -n "$cross_prefix" && test "$(basename "$sdlconfig")" = 
> sdl-config; then
> +echo warning: using "\"$sdlconfig\"" to detect cross-compiled sdl >&2
>fi
> -  sdl=no
> -fi
> -if test -n "$cross_prefix" && test "$(basename "$sdlconfig")" = sdl-config; 
> then
> -  echo warning: using "\"$sdlconfig\"" to detect cross-compiled sdl >&2
> -fi
>  
> -sdl_too_old=no
> -if test "$sdl" != "no" ; then
>cat > $TMPC << EOF
>  #include 
>  #undef main /* We don't want SDL to override our main() */
> @@ -2920,6 +2923,10 @@ EOF
>  fi
>  sdl=no
>fi # sdl compile test
> +}
> +
> +if test "$sdl" != "no" ; then
> +  sdl_probe
>  fi
>  
>  if test "$sdl" = "yes" ; then
> -- 
> 2.14.3
> 

Reviewed-by: Fam Zheng 

Re: [Qemu-devel] [PATCH] configure: don't warn GTK if disabled

[Daniel P . Berrangé]

On Mon, Apr 09, 2018 at 04:23:23PM +0800, Peter Xu wrote:
> We don't need to detect GTK ABI if GTK is disabled in general.
> Otherwise we could get this warning (when host is installed with GTK ABI
> version 2) even when configure with "--disable-gtk":
> 
> WARNING: Use of GTK 2.0 is deprecated and will be removed in
> WARNING: future releases. Please switch to using GTK 3.0
> 
> CC: Paolo Bonzini 
> CC: Gerd Hoffmann 
> CC: Peter Maydell 
> CC: Fam Zheng 
> CC: "Philippe Mathieu-Daudé" 
> Signed-off-by: Peter Xu 
> ---

Reviewed-by: Daniel P. Berrangé 



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

Re: [Qemu-devel] [PATCH] configure: don't warn SDL abi if disabled

[Daniel P . Berrangé]

On Tue, Apr 10, 2018 at 01:40:34PM +0800, Peter Xu wrote:
> SDL has the same problem as GTK that we might get warnings on SDL ABI
> version even if SDL is disabled.  Fix that by only probing SDL if SDL is
> enabled.  Also this should let configure be a little bit faster since we
> don't really need to probe SDL stuff when it's off.
> 
> CC: Paolo Bonzini 
> CC: Gerd Hoffmann 
> CC: Peter Maydell 
> CC: Daniel P. Berrange 
> CC: Fam Zheng 
> CC: "Philippe Mathieu-Daudé" 
> Signed-off-by: Peter Xu 

Reviewed-by: Daniel P. Berrangé 


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

Re: [Qemu-devel] [PATCH] migration: Don't activate block devices if using -S

[Dr. David Alan Gilbert]

* Kevin Wolf (kw...@redhat.com) wrote:
> Am 10.04.2018 um 09:36 hat Jiri Denemark geschrieben:
> > On Mon, Apr 09, 2018 at 15:40:03 +0200, Kevin Wolf wrote:
> > > Am 09.04.2018 um 12:27 hat Dr. David Alan Gilbert geschrieben:
> > > > It's a fairly hairy failure case they had; if I remember correctly it's:
> > > >   a) Start migration
> > > >   b) Migration gets to completion point
> > > >   c) Destination is still paused
> > > >   d) Libvirt is restarted on the source
> > > >   e) Since libvirt was restarted it fails the migration (and hence knows
> > > >  the destination won't be started)
> > > >   f) It now tries to resume the qemu on the source
> > > > 
> > > > (f) fails because (b) caused the locks to be taken on the destination;
> > > > hence this patch stops doing that.  It's a case we don't really think
> > > > about - i.e. that the migration has actually completed and all the data
> > > > is on the destination, but libvirt decides for some other reason to
> > > > abandon migration.
> > > 
> > > If you do remember correctly, that scenario doesn't feel tricky at all.
> > > libvirt needs to quit the destination qemu, which will inactivate the
> > > images on the destination and release the lock, and then it can continue
> > > the source.
> > > 
> > > In fact, this is so straightforward that I wonder what else libvirt is
> > > doing. Is the destination qemu only shut down after trying to continue
> > > the source? That would be libvirt using the wrong order of steps.
> > 
> > There's no connection between the two libvirt daemons in the case we're
> > talking about so they can't really synchronize the actions. The
> > destination daemon will kill the new QEMU process and the source will
> > resume the old one, but the order is completely random.
> 
> Hm, okay...
> 
> > > > Yes it was a 'block-activate' that I'd wondered about.  One complication
> > > > is that if this now under the control of the management layer then we
> > > > should stop asserting when the block devices aren't in the expected
> > > > state and just cleanly fail the command instead.
> > > 
> > > Requiring an explicit 'block-activate' on the destination would be an
> > > incompatible change, so you would have to introduce a new option for
> > > that. 'block-inactivate' on the source feels a bit simpler.
> > 
> > As I said in another email, the explicit block-activate command could
> > depend on a migration capability similarly to how pre-switchover state
> > works.
> 
> Yeah, that's exactly the thing that we wouldn't need if we could use
> 'block-inactivate' on the source instead. It feels a bit wrong to
> design a more involved QEMU interface around the libvirt internals,

It's not necessarily 'libvirt internals' - it's a case of them having to
cope with recovering from failures that happen around migration; it's
not an easy problem, and if they've got a way to stop both sides running
at the same time that's pretty important.

> but
> as long as we implement both sides for symmetry and libvirt just happens
> to pick the destination side for now, I think it's okay.
> 
> By the way, are block devices the only thing that need to be explicitly
> activated? For example, what about qemu_announce_self() for network
> cards, do we need to delay that, too?
> 
> In any case, I think this patch needs to be reverted for 2.12 because
> it's wrong, and then we can create the proper solution in the 2.13
> timefrage.

what case does this break?
I'm a bit wary of reverting this, which fixes a known problem, on the
basis that it causes a theoretical problem.

Dave

> Kevin
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

[Qemu-devel] [PATCH] qemu-iotests: Remove _supported_fmt dmg

[Kevin Wolf]

qemu-iotests doesn't support dmg, and the dmg block driver doesn't
support image creation. Two test cases declare dmg as supported, but
that's obviously wrong for both reasons. Remove the declaration.

Signed-off-by: Kevin Wolf 
---
 tests/qemu-iotests/183 | 2 +-
 tests/qemu-iotests/194 | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
index 20268ff7a1..c49e1ad6ef 100755
--- a/tests/qemu-iotests/183
+++ b/tests/qemu-iotests/183
@@ -43,7 +43,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
 . ./common.filter
 . ./common.qemu
 
-_supported_fmt qcow2 raw qed dmg quorum
+_supported_fmt qcow2 raw qed quorum
 _supported_proto file
 _supported_os Linux
 
diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194
index 1d4214aca3..d746ab1e21 100755
--- a/tests/qemu-iotests/194
+++ b/tests/qemu-iotests/194
@@ -21,7 +21,7 @@
 
 import iotests
 
-iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw', 'dmg'])
+iotests.verify_image_format(supported_fmts=['qcow2', 'qed', 'raw'])
 iotests.verify_platform(['linux'])
 
 with iotests.FilePath('source.img') as source_img_path, \
-- 
2.13.6

Re: [Qemu-devel] [PATCH v2 0/2] iotests: blacklist bochs and cloop for 205 and 208

[Kevin Wolf]

Am 09.04.2018 um 13:44 hat Vladimir Sementsov-Ogievskiy geschrieben:
> v2: move from unsupported_fmts to support "generic", like in bash tests.

Thanks, applied to the block branch.

Kevin

Re: [Qemu-devel] [PATCH] migration: Don't activate block devices if using -S

[Kevin Wolf]

Am 10.04.2018 um 09:36 hat Jiri Denemark geschrieben:
> On Mon, Apr 09, 2018 at 15:40:03 +0200, Kevin Wolf wrote:
> > Am 09.04.2018 um 12:27 hat Dr. David Alan Gilbert geschrieben:
> > > It's a fairly hairy failure case they had; if I remember correctly it's:
> > >   a) Start migration
> > >   b) Migration gets to completion point
> > >   c) Destination is still paused
> > >   d) Libvirt is restarted on the source
> > >   e) Since libvirt was restarted it fails the migration (and hence knows
> > >  the destination won't be started)
> > >   f) It now tries to resume the qemu on the source
> > > 
> > > (f) fails because (b) caused the locks to be taken on the destination;
> > > hence this patch stops doing that.  It's a case we don't really think
> > > about - i.e. that the migration has actually completed and all the data
> > > is on the destination, but libvirt decides for some other reason to
> > > abandon migration.
> > 
> > If you do remember correctly, that scenario doesn't feel tricky at all.
> > libvirt needs to quit the destination qemu, which will inactivate the
> > images on the destination and release the lock, and then it can continue
> > the source.
> > 
> > In fact, this is so straightforward that I wonder what else libvirt is
> > doing. Is the destination qemu only shut down after trying to continue
> > the source? That would be libvirt using the wrong order of steps.
> 
> There's no connection between the two libvirt daemons in the case we're
> talking about so they can't really synchronize the actions. The
> destination daemon will kill the new QEMU process and the source will
> resume the old one, but the order is completely random.

Hm, okay...

> > > Yes it was a 'block-activate' that I'd wondered about.  One complication
> > > is that if this now under the control of the management layer then we
> > > should stop asserting when the block devices aren't in the expected
> > > state and just cleanly fail the command instead.
> > 
> > Requiring an explicit 'block-activate' on the destination would be an
> > incompatible change, so you would have to introduce a new option for
> > that. 'block-inactivate' on the source feels a bit simpler.
> 
> As I said in another email, the explicit block-activate command could
> depend on a migration capability similarly to how pre-switchover state
> works.

Yeah, that's exactly the thing that we wouldn't need if we could use
'block-inactivate' on the source instead. It feels a bit wrong to
design a more involved QEMU interface around the libvirt internals, but
as long as we implement both sides for symmetry and libvirt just happens
to pick the destination side for now, I think it's okay.

By the way, are block devices the only thing that need to be explicitly
activated? For example, what about qemu_announce_self() for network
cards, do we need to delay that, too?

In any case, I think this patch needs to be reverted for 2.12 because
it's wrong, and then we can create the proper solution in the 2.13
timefrage.

Kevin

Re: [Qemu-devel] qom-test on netbsd can be very slow

[Daniel P . Berrangé]

On Mon, Apr 09, 2018 at 06:10:43PM +0100, Peter Maydell wrote:
> My NetBSD build system recently seems to have taken a nosedive
> in how long it takes to finish "make check". This seems to be
> because qom-test (and probably other things where the test interacts
> with the QEMU process) can run very slowly.
> 
> netbsdvm# for i in 1 2 3 4 5 6 7 8 9 10; do
> (QTEST_QEMU_BINARY=x86_64-softmmu/qemu-system-x86_64 time
> tests/qom-test -p /x86_64/qom/pc-i440fx-2.0); done
> /x86_64/qom/pc-i440fx-2.0: OK
> 8.49 real 1.18 user 7.34 sys
> /x86_64/qom/pc-i440fx-2.0: OK
>10.41 real 1.32 user 9.09 sys
> /x86_64/qom/pc-i440fx-2.0: OK
> 8.45 real 1.24 user 7.24 sys
> /x86_64/qom/pc-i440fx-2.0: OK
> 9.88 real 1.10 user 8.31 sys
> /x86_64/qom/pc-i440fx-2.0: OK
>11.60 real 1.47 user 9.90 sys
> /x86_64/qom/pc-i440fx-2.0: OK
>10.94 real 1.28 user 9.68 sys
> /x86_64/qom/pc-i440fx-2.0: OK
>10.06 real 1.32 user 8.76 sys
> /x86_64/qom/pc-i440fx-2.0: OK
>13.38 real 1.37 user12.04 sys
> /x86_64/qom/pc-i440fx-2.0: OK
>16.19 real 1.46 user14.29 sys
> /x86_64/qom/pc-i440fx-2.0: OK
> 9.70 real 1.17 user 8.51 sys
> 
> Admittedly this is running in a (KVM) VM, but still, there seems
> something wrong with how long each of these is taking. On Linux
> each run is less than a second, so there's an order-of-magnitude
> slowdown here. Further, I've occasionally seen a run take 100 seconds!
> 
> Does anybody else see this, and any ideas why it might be running slow?

My only real suggestion is to try "git bisect" as presumably this is
a regression caused by something we've merged in this dev cycle ?

> One thing I noticed looking at ktrace output is that we do
> all our reading of QMP input and output with a read syscall
> per character. I don't think that's the cause of this slowness,
> though.

IIRC, that has been long standing behaviour so would be unlikely to
explain a recent slowdown, nor the huge variation in time for some
runs.

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

Re: [Qemu-devel] [Qemu-arm] Crash when running hello-world unikernel for ARM

[Ajay Garg]

Hi Peter.

Thanks for the reply.

On Tue, Apr 10, 2018 at 1:08 PM, Peter Maydell  wrote:
> On 10 April 2018 at 05:14, Ajay Garg  wrote:
>> Thanks Alex for the reply ..
>>
>>>
>>> Can you run under -s -S and gdb step the *guest* and see where it ends
>>> up. The above error is usually indicative of the guest going off into
>>> the weeds somewhere because the hardware isn't what it expects.
>>>
>>
>> So, after your reply that it might be because of the
>> hardware-mismatch, I kinda took a detour, and installed a arm32 "virt"
>> machine on qemu on a x86_64 host, as per the steps at
>> https://translatedcode.wordpress.com/2016/11/03/installing-debian-on-qemus-32-bit-arm-virt-board/
>>
>> All went fine, and then I compiled rumprun on this "virt" guest.
>
> The host system hardware doesn't matter (except that if you're
> running suitable matching host and guest hardware you might be
> able to use hardware acceleration with KVM, but for the moment
> I would recommend concentrating on getting it working). What
> matters is that the guest software you run must match the
> hardware that you are asking QEMU to emulate. (I guess if
> rumprun automatically configures itself based on the system
> you're compiling it on then you're running a different binary,
> but surely it has a setup for manually configuring it when you're
> cross-compiling it?)
>
> What hardware (what CPU, board, etc) is this "rumprun" software
> expecting to run on?

Yep, just to ensure that there are no cross-compiling issues, I am
building rumprun on the pseudo-real hardware itself.
In our case, the pseudo-real hardware are :

a)
An ARM32 "virt" hardware/machine in a qemu environment
(https://translatedcode.wordpress.com/2016/11/03/installing-debian-on-qemus-32-bit-arm-virt-board/)

Once I start  this machine, all environment is arm32, and I compile
rumprun within this environemnt without any cross-compiling.

b)
A beaglebone-green-wireless.
This is a arm32 machine bottoms-up, so no question of cross-compiling
whatsoever here :)

In both cases, I then use qemu-system-arm (on the "virt" machine, and
beaglebone-green-wireless itself).


One query : It is apparent that there is nested qemu-virtualization in
step a), could that be an issue?


>
> (The "Trying to execute code outside RAM or ROM at 0x0010"
> symptoms very frequently mean "guest binary was not built
> to run on this emulated hardware".)
>
> thanks
> -- PMM



-- 
Regards,
Ajay

Re: [Qemu-devel] [PATCH for-2.12 v2] qemu-iotests: update 185 output

[Stefan Hajnoczi]

On Wed, Apr 04, 2018 at 06:16:12PM +0200, Max Reitz wrote:
> On 2018-04-04 17:01, Stefan Hajnoczi wrote:
>  === Start mirror job and exit qemu ===
> 
> This seems to be independent of whether there is actually data on
> TEST_IMG (the commit source), so something doesn't seem quite right with
> the block job throttling here...?

I've been playing around with this test failure.  Let's leave it broken
(!) in QEMU 2.12 because this test has uncovered a block job ratelimit
issue that's not a regression.  The fix shouldn't be rushed.

block/mirror.c calculates delay_ns but then discards it:

  static void coroutine_fn mirror_run(void *opaque)
  {
  ...

  for (;;) {
  ...delay_ns is calculated based on job speed...

  ret = 0;
  trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
  if (block_job_is_cancelled(&s->common) && s->common.force) {
  break;
  } else if (!should_complete) {
  delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
^--- we discard it!
  block_job_sleep_ns(&s->common, delay_ns);
  }
  s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
  }

  ...
  }

This is why the mirror-based tests are completing even though we'd
expect them to only reach the "next" iteration due to the job speed.

Increasing the 4 MB write operation in the test to >16 MB (the mirror
buffer size) doesn't solve the problem because the next QMP command will
race with the job's 0 ns sleep.  It would just make the test unreliable.

I'll work on the following for QEMU 2.13:

1. Avoid spurious block_job_enter() from block_job_drain().  This
   eliminates the extra block job iteration that changed the output in
   the first place.

2. Honor the job speed in block/mirror.c.

The end result will be that the test output will not require changes.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Tue, Apr 10, 2018 at 03:15:57PM +0800, Peter Xu wrote:
> On Mon, Apr 09, 2018 at 11:19:43AM +0200, Marc-André Lureau wrote:
> > Hi
> > 
> > On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu  wrote:
> > > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
> > >> Hi Peter
> > >>
> > >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu  wrote:
> > >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> > >> >> Hi
> > >> >>
> > >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu  wrote:
> > >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> > >> >> >> Hi
> > >> >> >>
> > >> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu  
> > >> >> >> wrote:
> > >> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau 
> > >> >> >> > wrote:
> > >> >> >> >
> > >> >> >> > [...]
> > >> >> >> >
> > >> >> >> >> > +/*
> > >> >> >> >> > + * Dispatch one single QMP request. The function will free 
> > >> >> >> >> > the req_obj
> > >> >> >> >> > + * and objects inside it before return.
> > >> >> >> >> > + */
> > >> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> >> >> >  {
> > >> >> >> >> > -QObject *req, *rsp = NULL, *id = NULL;
> > >> >> >> >> > +Monitor *mon, *old_mon;
> > >> >> >> >> > +QObject *req, *rsp = NULL, *id;
> > >> >> >> >> >  QDict *qdict = NULL;
> > >> >> >> >> > -MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, 
> > >> >> >> >> > parser);
> > >> >> >> >> > -Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, 
> > >> >> >> >> > qmp);
> > >> >> >> >> > -
> > >> >> >> >> > -Error *err = NULL;
> > >> >> >> >> > +bool need_resume;
> > >> >> >> >> >
> > >> >> >> >> > -req = json_parser_parse_err(tokens, NULL, &err);
> > >> >> >> >> > -if (!req && !err) {
> > >> >> >> >> > -/* json_parser_parse_err() sucks: can fail without 
> > >> >> >> >> > setting @err */
> > >> >> >> >> > -error_setg(&err, QERR_JSON_PARSING);
> > >> >> >> >> > -}
> > >> >> >> >> > -if (err) {
> > >> >> >> >> > -goto err_out;
> > >> >> >> >> > -}
> > >> >> >> >> > +req = req_obj->req;
> > >> >> >> >> > +mon = req_obj->mon;
> > >> >> >> >> > +id = req_obj->id;
> > >> >> >> >> > +need_resume = req_obj->need_resume;
> > >> >> >> >> >
> > >> >> >> >> > -qdict = qobject_to_qdict(req);
> > >> >> >> >> > -if (qdict) {
> > >> >> >> >> > -id = qdict_get(qdict, "id");
> > >> >> >> >> > -qobject_incref(id);
> > >> >> >> >> > -qdict_del(qdict, "id");
> > >> >> >> >> > -} /* else will fail qmp_dispatch() */
> > >> >> >> >> > +g_free(req_obj);
> > >> >> >> >> >
> > >> >> >> >> >  if 
> > >> >> >> >> > (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> > >> >> >> >> >  QString *req_json = qobject_to_json(req);
> > >> >> >> >> > @@ -3900,7 +3932,7 @@ static void 
> > >> >> >> >> > handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> > >> >> >> >> >  old_mon = cur_mon;
> > >> >> >> >> >  cur_mon = mon;
> > >> >> >> >>
> > >> >> >> >> There is another issue with this series, since cur_mon is 
> > >> >> >> >> global (and
> > >> >> >> >> not protected), an oob command may change the cur_mon while 
> > >> >> >> >> another
> > >> >> >> >> command is running in the main thread with unexpected 
> > >> >> >> >> consequences. I
> > >> >> >> >> don't have a clear idea what is the best way to solve it. 
> > >> >> >> >> Making the
> > >> >> >> >> variable per-thread, or going all the way to get rid of cur_mon 
> > >> >> >> >> (my
> > >> >> >> >> preference, but much harder)
> > >> >> >> >
> > >> >> >> > IMHO it is fine too.
> > >> >> >> >
> > >> >> >> > Note that this cur_mon operation is in 
> > >> >> >> > monitor_qmp_dispatch_one() now,
> > >> >> >> > which is still running in main thread.  So AFAICT all the cur_mon
> > >> >> >> > references are in main thread, and monitor IOThread does not 
> > >> >> >> > modify
> > >> >> >> > that variable at all.  Then we should probably be safe.
> > >> >> >>
> > >> >> >> But monitor_qmp_dispatch_one() is called from iothread if the 
> > >> >> >> command
> > >> >> >> is oob, so cur_mon may be updated while another command is running 
> > >> >> >> in
> > >> >> >> main thread, or am I wrong?
> > >> >> >
> > >> >> > You are right. I missed that, sorry...
> > >> >> >
> > >> >> > Would this be a simple workaround (but hopefully efficient) 
> > >> >> > solution?
> > >> >> >
> > >> >> > diff --git a/monitor.c b/monitor.c
> > >> >> > index 77f4c41cfa..99641c0c6d 100644
> > >> >> > --- a/monitor.c
> > >> >> > +++ b/monitor.c
> > >> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> > >> >> >   * Dispatch one single QMP request. The function will free the 
> > >> >> > req_obj
> > >> >> >   * and objects inside it before return.
> > >> >> >   */
> > >> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> > >> >> > +static void monitor_qmp_dispatch_one(QMPReq

Re: [Qemu-devel] [PATCH v8 19/23] SiFive RISC-V UART Device

[Antony Pavlov]

On Tue, 10 Apr 2018 08:17:32 +0200
Thomas Huth  wrote:

> On 10.04.2018 05:21, Antony Pavlov wrote:
> > On Sat,  3 Mar 2018 02:51:47 +1300
> > Michael Clark  wrote:
> > 
> >> QEMU model of the UART on the SiFive E300 and U500 series SOCs.
> >> BBL supports the SiFive UART for early console access via the SBI
> >> (Supervisor Binary Interface) and the linux kernel SBI console.
> >>
> >> The SiFive UART implements the pre qom legacy interface consistent
> >> with the 16550a UART in 'hw/char/serial.c'.
> >>
> >> Acked-by: Richard Henderson 
> >> Signed-off-by: Stefan O'Rear 
> >> Signed-off-by: Palmer Dabbelt 
> >> Signed-off-by: Michael Clark 
> >> ---
> >>  hw/riscv/sifive_uart.c | 176 
> >> +
> >>  include/hw/riscv/sifive_uart.h |  71 +
> >>  2 files changed, 247 insertions(+)
> >>  create mode 100644 hw/riscv/sifive_uart.c
> >>  create mode 100644 include/hw/riscv/sifive_uart.h
> >>
> >> diff --git a/hw/riscv/sifive_uart.c b/hw/riscv/sifive_uart.c
> >> new file mode 100644
> >> index 000..b0c3798
> >> --- /dev/null
> >> +++ b/hw/riscv/sifive_uart.c
> >> @@ -0,0 +1,176 @@
> >> +/*
> >> + * QEMU model of the UART on the SiFive E300 and U500 series SOCs.
> >> + *
> >> + * Copyright (c) 2016 Stefan O'Rear
> >> + *
> >> + * 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 "qapi/error.h"
> >> +#include "hw/sysbus.h"
> >> +#include "chardev/char.h"
> >> +#include "chardev/char-fe.h"
> >> +#include "target/riscv/cpu.h"
> >> +#include "hw/riscv/sifive_uart.h"
> >>
> >> +/*
> >> + * Not yet implemented:
> >> + *
> >> + * Transmit FIFO using "qemu/fifo8.h"
> >> + * SIFIVE_UART_IE_TXWM interrupts
> >> + * SIFIVE_UART_IE_RXWM interrupts must honor fifo watermark
> >> + * Rx FIFO watermark interrupt trigger threshold
> >> + * Tx FIFO watermark interrupt trigger threshold.
> >> + */
> >> +
> >> +static void update_irq(SiFiveUARTState *s)
> >> +{
> >> +int cond = 0;
> >> +if ((s->ie & SIFIVE_UART_IE_RXWM) && s->rx_fifo_len) {
> >> +cond = 1;
> >> +}
> >> +if (cond) {
> >> +qemu_irq_raise(s->irq);
> >> +} else {
> >> +qemu_irq_lower(s->irq);
> >> +}
> >> +}
> >> +
> >> +static uint64_t
> >> +uart_read(void *opaque, hwaddr addr, unsigned int size)
> >> +{
> >> +SiFiveUARTState *s = opaque;
> >> +unsigned char r;
> >> +switch (addr) {
> >> +case SIFIVE_UART_RXFIFO:
> >> +if (s->rx_fifo_len) {
> >> +r = s->rx_fifo[0];
> >> +memmove(s->rx_fifo, s->rx_fifo + 1, s->rx_fifo_len - 1);
> >> +s->rx_fifo_len--;
> >> +qemu_chr_fe_accept_input(&s->chr);
> >> +update_irq(s);
> >> +return r;
> >> +}
> >> +return 0x8000;
> >> +
> >> +case SIFIVE_UART_TXFIFO:
> >> +return 0; /* Should check tx fifo */
> >> +case SIFIVE_UART_IE:
> >> +return s->ie;
> >> +case SIFIVE_UART_IP:
> >> +return s->rx_fifo_len ? SIFIVE_UART_IP_RXWM : 0;
> >> +case SIFIVE_UART_TXCTRL:
> >> +return s->txctrl;
> >> +case SIFIVE_UART_RXCTRL:
> >> +return s->rxctrl;
> >> +case SIFIVE_UART_DIV:
> >> +return s->div;
> >> +}
> >> +
> >> +hw_error("%s: bad read: addr=0x%x\n",
> >> +__func__, (int)addr);
> >> +return 0;
> >> +}
> >> +
> >> +static void
> >> +uart_write(void *opaque, hwaddr addr,
> >> +   uint64_t val64, unsigned int size)
> >> +{
> >> +SiFiveUARTState *s = opaque;
> >> +uint32_t value = val64;
> >> +unsigned char ch = value;
> >> +
> >> +switch (addr) {
> >> +case SIFIVE_UART_TXFIFO:
> >> +qemu_chr_fe_write(&s->chr, &ch, 1);
> >> +return;
> >> +case SIFIVE_UART_IE:
> >> +s->ie = val64;
> >> +update_irq(s);
> >> +return;
> >> +case SIFIVE_UART_TXCTRL:
> >> +s->txctrl = val64;
> >> +return;
> >> +case SIFIVE_UART_RXCTRL:
> >> +s->rxctrl = val64;
> >> +return;
> >> +case SIFIVE_UART_DIV:
> >> +s->div = val64;
> >> +return;
> >> +}
> >> +hw_error("%s: bad write: addr=0x%x v=0x%x\n",
> >> +__func__, (int)addr, (int)value);
> >> +}
> >> +
> >> +static const MemoryRegionOps uart_ops = {
> >> +.read = uart_read,
> >> +.writ

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Thomas Huth]

On 09.04.2018 18:34, Laszlo Ersek wrote:
> On 04/09/18 09:26, Thomas Huth wrote:
>>  Hi Laszlo,
>>
>> On 07.04.2018 02:01, Laszlo Ersek wrote:
>>> Add a schema that describes the properties of virtual machine firmware.
>>>
>>> Each firmware executable installed on a host system should come with a
>>> JSON file that conforms to this schema, and informs the management
>>> applications about the firmware's properties.
>>>
>>> In addition, a configuration directory with symlinks to the JSON files
>>> should exist, with the symlinks carefully named to reflect a priority
>>> order. Management applications can then search this directory in priority
>>> order for the first firmware executable that satisfies their search
>>> criteria. The found JSON file provides the management layer with domain
>>> configuration bits that are required to run the firmware binary.
>> [...]
>>> +##
>>> +# @FirmwareDevice:
>>> +#
>>> +# Defines the device types that a firmware file can be mapped into.
>>> +#
>>> +# @memory: The firmware file is to be mapped into memory.
>>> +#
>>> +# @kernel: The firmware file is to be loaded like a Linux kernel. This is
>>> +#  similar to @memory but may imply additional processing that is
>>> +#  specific to the target architecture.
>>> +#
>>> +# @flash: The firmware file is to be mapped into a pflash chip.
>>> +#
>>> +# Since: 2.13
>>> +##
>>> +{ 'enum' : 'FirmwareDevice',
>>> +  'data' : [ 'memory', 'kernel', 'flash' ] }
>>
>> This is not fully clear to me... what is this exactly good for? Is this
>> a way to say how the firmware should be loaded, i.e. via "-bios",
>> "-kernel" or "-pflash" parameter? If so, the term "memory" is quite
>> misleading since files that are loaded via -bios can also end up in an
>> emulated ROM chip.
> 
> I threw in "-kernel" because, although it also (usually?) means
> "memory", I expected people would want it separate.
> 
> Regarding memory vs. pflash, I thought that these two, combined with the
> access permissions, could cover all of RAM, ROM, and read-only and
> read-write pflash too.
> 
> So, "-bios" (-> ROM) boils down to "memory", with write access denied --
> please see the SeaBIOS example near the end.

Let me ask the other way round: How does a high-level tool know whether
it should use "-bios", "-kernel", "-pflash", "-device generic-loader" or
"-younameit" for loading the firmware?

>>> +   'nvram-map' : 'FirmwareMapping',
>>> +   'templates' : [ 'FirmwareFile' ] } }
>>> +
>>> +##
>>> +# @SystemFirmwareType:
>>> +#
>>> +# Lists system firmware types commonly used with QEMU virtual machines.
>>> +#
>>> +# @bios: The system firmware was built from the SeaBIOS project.
>>> +#
>>> +# @slof: The system firmware was built from the Slimline Open Firmware 
>>> project.
>>> +#
>>> +# @uboot: The system firmware was built from the U-Boot project.
>>> +#
>>> +# @uefi: The system firmware was built from the edk2 (EFI Development Kit 
>>> II)
>>> +#project.
>>> +#
>>> +# Since: 2.13
>>> +##
>>> +{ 'enum' : 'SystemFirmwareType',
>>> +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
>>
>> The naming here is quite a bad mixture between firmware interface
>> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There
>> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ...
>> so I'd suggest to rather name them 'seabios' and 'edk2' here instead.
> 
> Sure, I'm totally ready to follow community advice here (too).
> 
> In fact this is the one element I dislike the most about the schema --
> it's the fuzziest part, yet it is the most important element for
> libvirt. Because users and higher level apps just want to say "give me
> UEFI". If I have to ask "OK, but UEFI built from edk2 or something
> else?", then it's a lost cause already.
> 
> It's hard to find the right level of abstraction in the naming when the
> higher level tools (and/or ultimately the users) don't know enough to
> ask for specifics -- I'm not saying that's bad; it's quite natural, but
> makes things very difficult. So this enum aims to match the user story
> "gimme UEFI and be done with it". I figure users will just utter the
> most common buzzword form of the concept they have in mind. "edk2"
> doesn't tell them as much as "uefi".

OK, I see your point. But I still think we should not design fuzzy
interfaces here at this low level, this will only lead to other trouble
later. ... thinking about this again, users seem to mix up firmware
interfaces / families with concrete implementations. So maybe we need
something like:

 { 'enum' : 'SystemFirmwareType',
   'data' : [ 'seabios', 'slof', 'uboot', 'edk2', 'openbios' ] }

*and* :

 { 'enum' : 'SystemFirmwareInterface',  /* or: 'SystemFirmwareFamily' */
   'data' : [ 'bios', 'uefi', 'openfirmware' ] }

Then a high level tool can check both and pick the best match?

 Thomas

Re: [Qemu-devel] [PATCH for-2.12] fpu: Fix rounding mode for floatN_to_uintM_round_to_zero

[Peter Maydell]

On 10 April 2018 at 06:59, Richard Henderson
 wrote:
> We incorrectly passed in the current rounding mode
> instead of float_round_to_zero.
>
> Signed-off-by: Richard Henderson 
> ---
>
> Found while runnning SVE RISU tests; it should be visible with the
> right set of inputs to AdvSIMD RISU tests.
>
>
> r~
>
> ---
>  fpu/softfloat.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fpu/softfloat.c b/fpu/softfloat.c
> index 6e16284e66..b46dccc63e 100644
> --- a/fpu/softfloat.c
> +++ b/fpu/softfloat.c
> @@ -1486,8 +1486,8 @@ uint ## isz ## _t float ## fsz ## _to_uint ## isz ## 
> _round_to_zero \
>   (float ## fsz a, float_status *s)  \
>  {   \
>  FloatParts p = float ## fsz ## _unpack_canonical(a, s); \
> -return round_to_uint_and_pack(p, s->float_rounding_mode,\
> - UINT ## isz ## _MAX, s);   \
> +return round_to_uint_and_pack(p, float_round_to_zero,   \
> +  UINT ## isz ## _MAX, s);  \
>  }
>
>  FLOAT_TO_UINT(16, 16)
> --
> 2.14.3

Would this be likely the fix for
https://bugs.launchpad.net/qemu/+bug/1761401
?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/1] mach-virt: Change default cpu and gic-version setting to "max"

[Andrea Bolognani]

On Mon, 2018-04-09 at 11:29 -0500, Wei Huang wrote:
> > > Running mach-virt machine types (i.e. "-M virt") on different systems can
> > > result in various misleading warnings if -cpu and/or gic-version not 
> > > specified.
> > > For KVM, this can be solved mostly by using "host" type. But the "host" 
> > > type
> > > doesn't work for TCG. Compared with "host", the "max" type not only 
> > > supports
> > > auto detection under KVM mode, but also works with TCG. So this patch set
> > > "max" as the default types for both -cpu and gic-version.
> > 
> > Hmm, generally we aim for the config provided by a machine type to be stable
> > across QEMU versions.
> 
> I understand this principle. But in reality, under KVM mode, the default
> config most time doesn't work. If end users specify cpu type manually,
> it still doesn't work because the host CPU is vendor-specific design
> (e.g. "cortex-a57" doesn't work on QCOM's machine). So we end up with
> using "-cpu host" all the time. My argument for this patch is that "-cpu
> max" isn't worse than "-cpu host".

I figure the people not explicitly specifying a CPU model on the
command line will probably also use '-M virt' instead of versioned
machine types, which means they will get a different guest behavior
after upgrading QEMU regardless.

Defaulting to 'max' for '-cpu' and 'gic-version' makes it convenient
to quickly and concisely start a guest; if you care about guest ABI
at all, then you are already specifying everything explicitly on the
command line instead of relying on defaults - or using libvirt ;)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [Qemu-arm] Crash when running hello-world unikernel for ARM

[Peter Maydell]

On 10 April 2018 at 05:14, Ajay Garg  wrote:
> Thanks Alex for the reply ..
>
>>
>> Can you run under -s -S and gdb step the *guest* and see where it ends
>> up. The above error is usually indicative of the guest going off into
>> the weeds somewhere because the hardware isn't what it expects.
>>
>
> So, after your reply that it might be because of the
> hardware-mismatch, I kinda took a detour, and installed a arm32 "virt"
> machine on qemu on a x86_64 host, as per the steps at
> https://translatedcode.wordpress.com/2016/11/03/installing-debian-on-qemus-32-bit-arm-virt-board/
>
> All went fine, and then I compiled rumprun on this "virt" guest.

The host system hardware doesn't matter (except that if you're
running suitable matching host and guest hardware you might be
able to use hardware acceleration with KVM, but for the moment
I would recommend concentrating on getting it working). What
matters is that the guest software you run must match the
hardware that you are asking QEMU to emulate. (I guess if
rumprun automatically configures itself based on the system
you're compiling it on then you're running a different binary,
but surely it has a setup for manually configuring it when you're
cross-compiling it?)

What hardware (what CPU, board, etc) is this "rumprun" software
expecting to run on?

(The "Trying to execute code outside RAM or ROM at 0x0010"
symptoms very frequently mean "guest binary was not built
to run on this emulated hardware".)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] icount: fix cpu_restore_state_from_tb for non-tb-exit cases

[Paolo Bonzini]

On 09/04/2018 11:13, Pavel Dovgalyuk wrote:
> @@ -29,6 +29,7 @@ void cpu_loop_exit_noexc(CPUState *cpu)
>  {
>  /* XXX: restore cpu registers saved in host registers */
>  
> +cpu->can_do_io = !use_icount;
>  cpu->exception_index = -1;
>  siglongjmp(cpu->jmp_env, 1);
>  }
> @@ -65,14 +66,16 @@ void cpu_reloading_memory_map(void)
>  
>  void cpu_loop_exit(CPUState *cpu)
>  {
> +cpu->can_do_io = !use_icount;
>  siglongjmp(cpu->jmp_env, 1);
>  }
>  
>  void cpu_loop_exit_restore(CPUState *cpu, uintptr_t pc)
>  {
>  if (pc) {
> -cpu_restore_state(cpu, pc);
> +cpu_restore_state(cpu, pc, true);
>  }
> +cpu->can_do_io = !use_icount;
>  siglongjmp(cpu->jmp_env, 1);
>  }

This is incorrect, "cpu->can_do_io" is 1 when not in tcg_qemu_tb_exec.
In fact, in cpu_exec we have "cpu->can_do_io = 1;" immediately after
siglongjmp, so I propose adding the same "cpu->can_do_io = 1;"
assignment to cpu_exec_step_atomic.

In any case, please change the two siglongjmp of
cpu_loop_exit_{noexc,restore} to cpu_loop_exit, instead of duplicating
that cpu->can_do_io assignment.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH] migration: Don't activate block devices if using -S

[Jiri Denemark]

On Mon, Apr 09, 2018 at 15:40:03 +0200, Kevin Wolf wrote:
> Am 09.04.2018 um 12:27 hat Dr. David Alan Gilbert geschrieben:
> > It's a fairly hairy failure case they had; if I remember correctly it's:
> >   a) Start migration
> >   b) Migration gets to completion point
> >   c) Destination is still paused
> >   d) Libvirt is restarted on the source
> >   e) Since libvirt was restarted it fails the migration (and hence knows
> >  the destination won't be started)
> >   f) It now tries to resume the qemu on the source
> > 
> > (f) fails because (b) caused the locks to be taken on the destination;
> > hence this patch stops doing that.  It's a case we don't really think
> > about - i.e. that the migration has actually completed and all the data
> > is on the destination, but libvirt decides for some other reason to
> > abandon migration.
> 
> If you do remember correctly, that scenario doesn't feel tricky at all.
> libvirt needs to quit the destination qemu, which will inactivate the
> images on the destination and release the lock, and then it can continue
> the source.
> 
> In fact, this is so straightforward that I wonder what else libvirt is
> doing. Is the destination qemu only shut down after trying to continue
> the source? That would be libvirt using the wrong order of steps.

There's no connection between the two libvirt daemons in the case we're
talking about so they can't really synchronize the actions. The
destination daemon will kill the new QEMU process and the source will
resume the old one, but the order is completely random.

...
> > Yes it was a 'block-activate' that I'd wondered about.  One complication
> > is that if this now under the control of the management layer then we
> > should stop asserting when the block devices aren't in the expected
> > state and just cleanly fail the command instead.
> 
> Requiring an explicit 'block-activate' on the destination would be an
> incompatible change, so you would have to introduce a new option for
> that. 'block-inactivate' on the source feels a bit simpler.

As I said in another email, the explicit block-activate command could
depend on a migration capability similarly to how pre-switchover state
works.

Jirka

Re: [Qemu-devel] [qemu RFC] qapi: add "firmware.json"

[Thomas Huth]

On 09.04.2018 18:50, Laszlo Ersek wrote:
> On 04/09/18 10:19, Gerd Hoffmann wrote:
 +{ 'enum' : 'SystemFirmwareType',
 +  'data' : [ 'bios', 'slof', 'uboot', 'uefi' ] }
>>>
>>> The naming here is quite a bad mixture between firmware interface
>>> ('bios', 'uefi') and firmware implementations ('slof', 'uboot'). There
>>> could be other implementations of BIOS or UEFI than SeaBIOS and EDK2 ...
>>> so I'd suggest to rather name them 'seabios' and 'edk2' here instead.
>>
>> uboot for example implements uefi unterfaces too (dunno how complete,
>> but reportly recent versions can run uefi shell and grub just fine).
> 
> Indeed: when I was struggling with this enum type and tried to look for
> more firmware types to add, my googling turned up the "UEFI on Top of
> U-Boot" whitepaper, from Alex and Andreas :)
> 
> Again, this reaches to the root of the problem: when a user creates a
> new domain, using high-level tools, they just want to tick "UEFI". (Dan
> has emphasized this to me several times, so I think I get the idea by
> now, if not the full environment.) We cannot ask the user, "please be
> more specific, do you want UEFI from edk2, or UEFI on top of U-Boot?"

But you are designing a rather low-level interface here, which should
IMHO rather be precise than fuzzy. So should this "just want to tick
UEFI" rather be handled in the high-level tools instead?

Alternatively, what about providing some kind of "alias" or "nickname"
setting here, too? So the EDK2 builds would get
SystemFirmwareType="edk2" and "SystemFirmwareAlias="uefi" for example.

 Thomas

Re: [Qemu-devel] [PATCH 4/8] virtio: add detach element for packed ring(1.1)

[Jason Wang]

On 2018年04月04日 20:54, w...@redhat.com wrote:

From: Wei Xu 

helper for packed ring


It's odd and hard to review if you put detach patch first. I think this 
patch needs to be reordered after the implementation of pop/map.


Thanks


Signed-off-by: Wei Xu 
---
  hw/virtio/virtio.c | 21 +++--
  1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 478df3d..fdee40f 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -561,6 +561,20 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const 
VirtQueueElement *elem,
   elem->out_sg[i].iov_len);
  }
  
+static void virtqueue_detach_element_split(VirtQueue *vq,

+const VirtQueueElement *elem, unsigned int len)
+{
+vq->inuse--;
+virtqueue_unmap_sg(vq, elem, len);
+}
+
+static void virtqueue_detach_element_packed(VirtQueue *vq,
+const VirtQueueElement *elem, unsigned int len)
+{
+vq->inuse -= elem->count;
+virtqueue_unmap_sg(vq, elem, len);
+}
+
  /* virtqueue_detach_element:
   * @vq: The #VirtQueue
   * @elem: The #VirtQueueElement
@@ -573,8 +587,11 @@ static void virtqueue_unmap_sg(VirtQueue *vq, const 
VirtQueueElement *elem,
  void virtqueue_detach_element(VirtQueue *vq, const VirtQueueElement *elem,
unsigned int len)
  {
-vq->inuse--;
-virtqueue_unmap_sg(vq, elem, len);
+if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+virtqueue_detach_element_packed(vq, elem, len);
+} else {
+virtqueue_detach_element_split(vq, elem, len);
+}
  }
  
  /* virtqueue_unpop:

Re: [Qemu-devel] [PATCH for-2.12] make-release: add skiboot .version file

[Peter Maydell]

On 10 April 2018 at 01:57, Michael Roth  wrote:
> This is needed to build skiboot from tarball-distributed sources
> since the git data the make_release.sh script relies on to generate
> it is not available.
>
> Cc: qemu-sta...@nongnu.org
> Reported-by: Michael Tokarev 
> Signed-off-by: Michael Roth 
> ---
>  scripts/make-release | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/make-release b/scripts/make-release
> index 04fa9defdc..c14f75b12c 100755
> --- a/scripts/make-release
> +++ b/scripts/make-release
> @@ -19,6 +19,7 @@ pushd ${destination}
>  git checkout "v${version}"
>  git submodule update --init
>  (cd roms/seabios && git describe --tags --long --dirty > .version)
> +(cd roms/skiboot && ./make_version.sh > .version)
>  # FIXME: The following line is a workaround for avoiding filename collisions
>  # when unpacking u-boot sources on case-insensitive filesystems. Once we
>  # update to something with u-boot commit 610eec7f0 we can drop this line.

Seeing this comment in the context part of this patch reminds me:
have we updated to a u-boot that lets us drop the workaround yet?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/8] virtio: add empty check for packed ring

[Jason Wang]

On 2018年04月04日 20:53, w...@redhat.com wrote:

From: Wei Xu 

helper for ring empty check.


And descriptor read.



Signed-off-by: Wei Xu 
---
  hw/virtio/virtio.c | 62 +++---
  1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 73a35a4..478df3d 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -24,6 +24,9 @@
  #include "hw/virtio/virtio-access.h"
  #include "sysemu/dma.h"
  
+#define AVAIL_DESC_PACKED(b) ((b) << 7)

+#define USED_DESC_PACKED(b)  ((b) << 15)


Can we pass value other than 1 to this macro?


+
  /*
   * The alignment to use between consumer and producer parts of vring.
   * x86 pagesize again. This is the default, used by transports like PCI
@@ -446,10 +449,27 @@ int virtio_queue_ready(VirtQueue *vq)
  return vq->vring.avail != 0;
  }
  
+static void vring_desc_read_packed(VirtIODevice *vdev, VRingDescPacked *desc,

+MemoryRegionCache *cache, int i)
+{
+address_space_read_cached(cache, i * sizeof(VRingDescPacked),
+  desc, sizeof(VRingDescPacked));
+virtio_tswap64s(vdev, &desc->addr);
+virtio_tswap32s(vdev, &desc->len);
+virtio_tswap16s(vdev, &desc->id);
+virtio_tswap16s(vdev, &desc->flags);
+}
+
+static inline bool is_desc_avail(struct VRingDescPacked* desc)
+{
+return (!!(desc->flags & AVAIL_DESC_PACKED(1)) !=
+!!(desc->flags & USED_DESC_PACKED(1)));


Don't we need to care about endian here?


+}
+
  /* Fetch avail_idx from VQ memory only when we really need to know if
   * guest has added some buffers.
   * Called within rcu_read_lock().  */
-static int virtio_queue_empty_rcu(VirtQueue *vq)
+static int virtio_queue_empty_split_rcu(VirtQueue *vq)
  {
  if (unlikely(!vq->vring.avail)) {
  return 1;
@@ -462,7 +482,7 @@ static int virtio_queue_empty_rcu(VirtQueue *vq)
  return vring_avail_idx(vq) == vq->last_avail_idx;
  }
  
-int virtio_queue_empty(VirtQueue *vq)

+static int virtio_queue_empty_split(VirtQueue *vq)
  {
  bool empty;
  
@@ -480,6 +500,42 @@ int virtio_queue_empty(VirtQueue *vq)

  return empty;
  }
  
+static int virtio_queue_empty_packed_rcu(VirtQueue *vq)

+{
+struct VRingDescPacked desc;
+VRingMemoryRegionCaches *cache;
+
+if (unlikely(!vq->packed.desc)) {
+return 1;
+}
+
+cache = vring_get_region_caches(vq);
+vring_desc_read_packed(vq->vdev, &desc, &cache->desc_packed, 
vq->last_avail_idx);
+
+/* Make sure we see the updated flag */
+smp_mb();


What we need here is to make sure flag is read before all other fields, 
looks like this barrier can't.



+return !is_desc_avail(&desc);
+}
+
+static int virtio_queue_empty_packed(VirtQueue *vq)
+{
+bool empty;
+
+rcu_read_lock();
+empty = virtio_queue_empty_packed_rcu(vq);
+rcu_read_unlock();
+return empty;
+}
+
+int virtio_queue_empty(VirtQueue *vq)
+{
+if (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_RING_PACKED)) {
+return virtio_queue_empty_packed(vq);
+} else {
+return virtio_queue_empty_split(vq);
+}
+}
+
  static void virtqueue_unmap_sg(VirtQueue *vq, const VirtQueueElement *elem,
 unsigned int len)
  {
@@ -951,7 +1007,7 @@ void *virtqueue_pop(VirtQueue *vq, size_t sz)
  return NULL;
  }
  rcu_read_lock();
-if (virtio_queue_empty_rcu(vq)) {
+if (virtio_queue_empty_split_rcu(vq)) {


I think you'd better have a switch inside virtio_queue_empty_rcu() like 
virtio_queue_empty() here.


Thanks


  goto done;
  }
  /* Needed after virtio_queue_empty(), see comment in

Re: [Qemu-devel] [PATCH v8 14/23] monitor: separate QMP parser and dispatcher

[Peter Xu]

On Mon, Apr 09, 2018 at 11:19:43AM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Sun, Apr 8, 2018 at 5:02 AM, Peter Xu  wrote:
> > On Wed, Apr 04, 2018 at 03:58:56PM +0200, Marc-André Lureau wrote:
> >> Hi Peter
> >>
> >> On Wed, Mar 28, 2018 at 6:02 AM, Peter Xu  wrote:
> >> > On Mon, Mar 26, 2018 at 11:46:13AM +0200, Marc-André Lureau wrote:
> >> >> Hi
> >> >>
> >> >> On Mon, Mar 26, 2018 at 11:08 AM, Peter Xu  wrote:
> >> >> > On Mon, Mar 26, 2018 at 10:33:27AM +0200, Marc-André Lureau wrote:
> >> >> >> Hi
> >> >> >>
> >> >> >> On Mon, Mar 26, 2018 at 10:07 AM, Peter Xu  wrote:
> >> >> >> > On Fri, Mar 23, 2018 at 05:18:53PM +0100, Marc-André Lureau wrote:
> >> >> >> >
> >> >> >> > [...]
> >> >> >> >
> >> >> >> >> > +/*
> >> >> >> >> > + * Dispatch one single QMP request. The function will free the 
> >> >> >> >> > req_obj
> >> >> >> >> > + * and objects inside it before return.
> >> >> >> >> > + */
> >> >> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> >> >> >  {
> >> >> >> >> > -QObject *req, *rsp = NULL, *id = NULL;
> >> >> >> >> > +Monitor *mon, *old_mon;
> >> >> >> >> > +QObject *req, *rsp = NULL, *id;
> >> >> >> >> >  QDict *qdict = NULL;
> >> >> >> >> > -MonitorQMP *mon_qmp = container_of(parser, MonitorQMP, 
> >> >> >> >> > parser);
> >> >> >> >> > -Monitor *old_mon, *mon = container_of(mon_qmp, Monitor, 
> >> >> >> >> > qmp);
> >> >> >> >> > -
> >> >> >> >> > -Error *err = NULL;
> >> >> >> >> > +bool need_resume;
> >> >> >> >> >
> >> >> >> >> > -req = json_parser_parse_err(tokens, NULL, &err);
> >> >> >> >> > -if (!req && !err) {
> >> >> >> >> > -/* json_parser_parse_err() sucks: can fail without 
> >> >> >> >> > setting @err */
> >> >> >> >> > -error_setg(&err, QERR_JSON_PARSING);
> >> >> >> >> > -}
> >> >> >> >> > -if (err) {
> >> >> >> >> > -goto err_out;
> >> >> >> >> > -}
> >> >> >> >> > +req = req_obj->req;
> >> >> >> >> > +mon = req_obj->mon;
> >> >> >> >> > +id = req_obj->id;
> >> >> >> >> > +need_resume = req_obj->need_resume;
> >> >> >> >> >
> >> >> >> >> > -qdict = qobject_to_qdict(req);
> >> >> >> >> > -if (qdict) {
> >> >> >> >> > -id = qdict_get(qdict, "id");
> >> >> >> >> > -qobject_incref(id);
> >> >> >> >> > -qdict_del(qdict, "id");
> >> >> >> >> > -} /* else will fail qmp_dispatch() */
> >> >> >> >> > +g_free(req_obj);
> >> >> >> >> >
> >> >> >> >> >  if 
> >> >> >> >> > (trace_event_get_state_backends(TRACE_HANDLE_QMP_COMMAND)) {
> >> >> >> >> >  QString *req_json = qobject_to_json(req);
> >> >> >> >> > @@ -3900,7 +3932,7 @@ static void 
> >> >> >> >> > handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)
> >> >> >> >> >  old_mon = cur_mon;
> >> >> >> >> >  cur_mon = mon;
> >> >> >> >>
> >> >> >> >> There is another issue with this series, since cur_mon is global 
> >> >> >> >> (and
> >> >> >> >> not protected), an oob command may change the cur_mon while 
> >> >> >> >> another
> >> >> >> >> command is running in the main thread with unexpected 
> >> >> >> >> consequences. I
> >> >> >> >> don't have a clear idea what is the best way to solve it. Making 
> >> >> >> >> the
> >> >> >> >> variable per-thread, or going all the way to get rid of cur_mon 
> >> >> >> >> (my
> >> >> >> >> preference, but much harder)
> >> >> >> >
> >> >> >> > IMHO it is fine too.
> >> >> >> >
> >> >> >> > Note that this cur_mon operation is in monitor_qmp_dispatch_one() 
> >> >> >> > now,
> >> >> >> > which is still running in main thread.  So AFAICT all the cur_mon
> >> >> >> > references are in main thread, and monitor IOThread does not modify
> >> >> >> > that variable at all.  Then we should probably be safe.
> >> >> >>
> >> >> >> But monitor_qmp_dispatch_one() is called from iothread if the command
> >> >> >> is oob, so cur_mon may be updated while another command is running in
> >> >> >> main thread, or am I wrong?
> >> >> >
> >> >> > You are right. I missed that, sorry...
> >> >> >
> >> >> > Would this be a simple workaround (but hopefully efficient) solution?
> >> >> >
> >> >> > diff --git a/monitor.c b/monitor.c
> >> >> > index 77f4c41cfa..99641c0c6d 100644
> >> >> > --- a/monitor.c
> >> >> > +++ b/monitor.c
> >> >> > @@ -4023,7 +4023,7 @@ typedef struct QMPRequest QMPRequest;
> >> >> >   * Dispatch one single QMP request. The function will free the 
> >> >> > req_obj
> >> >> >   * and objects inside it before return.
> >> >> >   */
> >> >> > -static void monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> > +static void monitor_qmp_dispatch_one(QMPRequest *req_obj, bool 
> >> >> > hack_curmon)
> >> >> >  {
> >> >> >  Monitor *mon, *old_mon;
> >> >> >  QObject *req, *rsp = NULL, *id;
> >> >> > @@ -4043,12 +4043,16 @@ static void 
> >> >> > monitor_qmp_dispatch_one(QMPRequest *req_obj)
> >> >> >  QDECREF(req_json);
> >> >> >  }
> >> >> >
> >> >> > -old_mon = cur_mon;
> >> >> > -c

Re: [Qemu-devel] [PATCH 2/8] virtio: memory cache for packed ring

[Jason Wang]

On 2018年04月04日 20:53, w...@redhat.com wrote:

From: Wei Xu

A new memory cache is introduced to for packed ring,
the code looks pretty duplicated with split(1.0) ring,
any refactor idea?


I think you can reuse the exist code as I replied in patch 1.

Thanks

Re: [Qemu-devel] [PATCH 1/8] virtio: feature bit, data structure for packed ring

[Jason Wang]

On 2018年04月04日 20:53, w...@redhat.com wrote:

From: Wei Xu 

Only minimum definitions from the spec are included
for prototype.

Signed-off-by: Wei Xu 
---
  hw/virtio/virtio.c | 47 +++---
  include/hw/virtio/virtio.h | 12 ++-
  include/standard-headers/linux/virtio_config.h |  2 ++
  3 files changed, 56 insertions(+), 5 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 006d3d1..9a6bfe7 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -39,6 +39,14 @@ typedef struct VRingDesc
  uint16_t next;
  } VRingDesc;
  
+typedef struct VRingDescPacked

+{
+uint64_t addr;
+uint32_t len;
+uint16_t id;
+uint16_t flags;
+} VRingDescPacked;
+
  typedef struct VRingAvail
  {
  uint16_t flags;
@@ -61,9 +69,18 @@ typedef struct VRingUsed
  
  typedef struct VRingMemoryRegionCaches {

  struct rcu_head rcu;
-MemoryRegionCache desc;
-MemoryRegionCache avail;
-MemoryRegionCache used;
+union {
+struct {
+MemoryRegionCache desc;
+MemoryRegionCache avail;
+MemoryRegionCache used;
+};
+struct {
+MemoryRegionCache desc_packed;
+MemoryRegionCache driver;
+MemoryRegionCache device;
+};
+};


I think we can reuse exist memory region caches? Especially consider 
device/driver area could be treated as a renaming of avail/used area.


E.g desc for desc_packed, avail for driver area and used for device area.


  } VRingMemoryRegionCaches;
  
  typedef struct VRing

@@ -77,10 +94,31 @@ typedef struct VRing
  VRingMemoryRegionCaches *caches;
  } VRing;
  
+typedef struct VRingPackedDescEvent {

+uint16_t desc_event_off:15,
+ desc_event_wrap:1;
+uint16_t desc_event_flags:2;
+} VRingPackedDescEvent ;
+
+typedef struct VRingPacked
+{
+unsigned int num;
+unsigned int num_default;
+unsigned int align;
+hwaddr desc;
+hwaddr driver;
+hwaddr device;
+VRingMemoryRegionCaches *caches;
+} VRingPacked;


Same here, can we reuse VRing here?


+
  struct VirtQueue
  {
-VRing vring;
+union {
+struct VRing vring;
+struct VRingPacked packed;
+};
  
+uint8_t wrap_counter:1;

  /* Next head to pop */
  uint16_t last_avail_idx;
  
@@ -1220,6 +1258,7 @@ void virtio_reset(void *opaque)

  vdev->vq[i].vring.num = vdev->vq[i].vring.num_default;
  vdev->vq[i].inuse = 0;
  virtio_virtqueue_reset_region_cache(&vdev->vq[i]);
+vdev->vq[i].wrap_counter = 1;
  }
  }
  
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h

index 098bdaa..563e88e 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -46,6 +46,14 @@ typedef struct VirtQueueElement
  unsigned int index;
  unsigned int out_num;
  unsigned int in_num;
+
+/* Number of descriptors used by packed ring */


Do you mean the number of chained descriptors?


+uint16_t count;
+uint8_t wrap_counter:1;


What's the use of this bit? If you refer to my v1 vhost code, I used to 
have this, but it won't work for OOO completion e.g when zerocopy is 
disabled. I've dropped it now.


This is tricky and can only work when device complete descriptors in order.


+/* FIXME: Length of every used buffer for a descriptor,
+   move to dynamical allocating due to out/in sgs numbers */
+uint32_t len[VIRTQUEUE_MAX_SIZE];


Can you explain more about this?


+
  hwaddr *in_addr;
  hwaddr *out_addr;
  struct iovec *in_sg;
@@ -262,7 +270,9 @@ typedef struct VirtIORNGConf VirtIORNGConf;
  DEFINE_PROP_BIT64("any_layout", _state, _field, \
VIRTIO_F_ANY_LAYOUT, true), \
  DEFINE_PROP_BIT64("iommu_platform", _state, _field, \
-  VIRTIO_F_IOMMU_PLATFORM, false)
+  VIRTIO_F_IOMMU_PLATFORM, false), \
+DEFINE_PROP_BIT64("ring_packed", _state, _field, \
+  VIRTIO_F_RING_PACKED, true)


Remember to disable this for old machine types in next version.

Thanks

  
  hwaddr virtio_queue_get_desc_addr(VirtIODevice *vdev, int n);

  hwaddr virtio_queue_get_avail_addr(VirtIODevice *vdev, int n);
diff --git a/include/standard-headers/linux/virtio_config.h 
b/include/standard-headers/linux/virtio_config.h
index b777069..6ee5529 100644
--- a/include/standard-headers/linux/virtio_config.h
+++ b/include/standard-headers/linux/virtio_config.h
@@ -71,4 +71,6 @@
   * this is for compatibility with legacy systems.
   */
  #define VIRTIO_F_IOMMU_PLATFORM   33
+
+#define VIRTIO_F_RING_PACKED   34
  #endif /* _LINUX_VIRTIO_CONFIG_H */