Re: [PATCH v9 Kernel 2/5] vfio iommu: Add ioctl defination to get dirty pages bitmap.

[Kirti Wankhede]

On 12/5/2019 11:26 AM, Alex Williamson wrote:

On Thu, 5 Dec 2019 11:12:23 +0530
Kirti Wankhede  wrote:


On 12/5/2019 6:58 AM, Yan Zhao wrote:

On Thu, Dec 05, 2019 at 02:34:57AM +0800, Alex Williamson wrote:

On Wed, 4 Dec 2019 23:40:25 +0530
Kirti Wankhede  wrote:
  

On 12/3/2019 11:34 PM, Alex Williamson wrote:

On Mon, 25 Nov 2019 19:57:39 -0500
Yan Zhao  wrote:
  

On Fri, Nov 15, 2019 at 05:06:25AM +0800, Alex Williamson wrote:

On Fri, 15 Nov 2019 00:26:07 +0530
Kirti Wankhede  wrote:
 

On 11/14/2019 1:37 AM, Alex Williamson wrote:

On Thu, 14 Nov 2019 01:07:21 +0530
Kirti Wankhede  wrote:
   

On 11/13/2019 4:00 AM, Alex Williamson wrote:

On Tue, 12 Nov 2019 22:33:37 +0530
Kirti Wankhede  wrote:
  

All pages pinned by vendor driver through vfio_pin_pages API should be
considered as dirty during migration. IOMMU container maintains a list of
all such pinned pages. Added an ioctl defination to get bitmap of such


definition
  

pinned pages for requested IO virtual address range.


Additionally, all mapped pages are considered dirty when physically
mapped through to an IOMMU, modulo we discussed devices opting in to
per page pinning to indicate finer granularity with a TBD mechanism to
figure out if any non-opt-in devices remain.
  


You mean, in case of device direct assignment (device pass through)?


Yes, or IOMMU backed mdevs.  If vfio_dmas in the container are fully
pinned and mapped, then the correct dirty page set is all mapped pages.
We discussed using the vpfn list as a mechanism for vendor drivers to
reduce their migration footprint, but we also discussed that we would
need a way to determine that all participants in the container have
explicitly pinned their working pages or else we must consider the
entire potential working set as dirty.
   


How can vendor driver tell this capability to iommu module? Any suggestions?


I think it does so by pinning pages.  Is it acceptable that if the
vendor driver pins any pages, then from that point forward we consider
the IOMMU group dirty page scope to be limited to pinned pages?  There

we should also be aware of that dirty page scope is pinned pages + unpinned 
pages,
which means ever since a page is pinned, it should be regarded as dirty
no matter whether it's unpinned later. only after log_sync is called and
dirty info retrieved, its dirty state should be cleared.


Yes, good point.  We can't just remove a vpfn when a page is unpinned
or else we'd lose information that the page potentially had been
dirtied while it was pinned.  Maybe that vpfn needs to move to a dirty
list and both the currently pinned vpfns and the dirty vpfns are walked
on a log_sync.  The dirty vpfns list would be cleared after a log_sync.
The container would need to know that dirty tracking is enabled and
only manage the dirty vpfns list when necessary.  Thanks,
  


If page is unpinned, then that page is available in free page pool for
others to use, then how can we say that unpinned page has valid data?

If suppose, one driver A unpins a page and when driver B of some other
device gets that page and he pins it, uses it, and then unpins it, then
how can we say that page has valid data for driver A?

Can you give one example where unpinned page data is considered reliable
and valid?


We can only pin pages that the user has already allocated* and mapped
through the vfio DMA API.  The pinning of the page simply locks the
page for the vendor driver to access it and unpinning that page only
indicates that access is complete.  Pages are not freed when a vendor
driver unpins them, they still exist and at this point we're now
assuming the device dirtied the page while it was pinned.  Thanks,

Alex

* An exception here is that the page might be demand allocated and the
act of pinning the page could actually allocate the backing page for
the user if they have not faulted the page to trigger that allocation
previously.  That page remains mapped for the user's virtual address
space even after the unpinning though.
  


Yes, I can give an example in GVT.
when a gem_object is allocated in guest, before submitting it to guest
vGPU, gfx cmds in its ring buffer need to be pinned into GGTT to get a
global graphics address for hardware access. At that time, we shadow
those cmds and pin pages through vfio pin_pages(), and submit the shadow
gem_object to physial hardware.
After guest driver thinks the submitted gem_object has completed hardware
DMA, it unnpinnd those pinned GGTT graphics memory addresses. Then in
host, we unpin the shadow pages through vfio unpin_pages.
But, at this point, guest driver is still free to access the gem_object
through vCPUs, and guest user space is probably still mapping an object
into the gem_object in guest driver.
So, missing the dirty page tracking for unpinned pages would cause
data inconsitency.
   


If pages are accessed by guest through vCPUs, then RAM module i

Re: [RFC PATCH 0/9] Introduce mediate ops in vfio-pci

[Jason Wang]

Hi:

On 2019/12/5 上午11:24, Yan Zhao wrote:

For SRIOV devices, VFs are passthroughed into guest directly without host
driver mediation. However, when VMs migrating with passthroughed VFs,
dynamic host mediation is required to  (1) get device states, (2) get
dirty pages. Since device states as well as other critical information
required for dirty page tracking for VFs are usually retrieved from PFs,
it is handy to provide an extension in PF driver to centralizingly control
VFs' migration.

Therefore, in order to realize (1) passthrough VFs at normal time, (2)
dynamically trap VFs' bars for dirty page tracking and



A silly question, what's the reason for doing this, is this a must for 
dirty page tracking?




  (3) centralizing
VF critical states retrieving and VF controls into one driver, we propose
to introduce mediate ops on top of current vfio-pci device driver.


_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
   
  __   register mediate ops|  ___ ___|

|  |<---| VF|   |   |
| vfio-pci |  | |  mediate  |   | PF driver |   |
|__|--->|   driver  |   |___|
  |open(pdev)  |  ---  | |
  ||
  ||_ _ _ _ _ _ _ _ _ _ _ _|_ _ _ _ _|
 \|/  \|/
--- 
|VF   | |PF|
--- 


VF mediate driver could be a standalone driver that does not bind to
any devices (as in demo code in patches 5-6) or it could be a built-in
extension of PF driver (as in patches 7-9) .

Rather than directly bind to VF, VF mediate driver register a mediate
ops into vfio-pci in driver init. vfio-pci maintains a list of such
mediate ops.
(Note that: VF mediate driver can register mediate ops into vfio-pci
before vfio-pci binding to any devices. And VF mediate driver can
support mediating multiple devices.)

When opening a device (e.g. a VF), vfio-pci goes through the mediate ops
list and calls each vfio_pci_mediate_ops->open() with pdev of the opening
device as a parameter.
VF mediate driver should return success or failure depending on it
supports the pdev or not.
E.g. VF mediate driver would compare its supported VF devfn with the
devfn of the passed-in pdev.
Once vfio-pci finds a successful vfio_pci_mediate_ops->open(), it will
stop querying other mediate ops and bind the opening device with this
mediate ops using the returned mediate handle.

Further vfio-pci ops (VFIO_DEVICE_GET_REGION_INFO ioctl, rw, mmap) on the
VF will be intercepted into VF mediate driver as
vfio_pci_mediate_ops->get_region_info(),
vfio_pci_mediate_ops->rw,
vfio_pci_mediate_ops->mmap, and get customized.
For vfio_pci_mediate_ops->rw and vfio_pci_mediate_ops->mmap, they will
further return 'pt' to indicate whether vfio-pci should further
passthrough data to hw.

when vfio-pci closes the VF, it calls its vfio_pci_mediate_ops->release()
with a mediate handle as parameter.

The mediate handle returned from vfio_pci_mediate_ops->open() lets VF
mediate driver be able to differentiate two opening VFs of the same device
id and vendor id.

When VF mediate driver exits, it unregisters its mediate ops from
vfio-pci.


In this patchset, we enable vfio-pci to provide 3 things:
(1) calling mediate ops to allow vendor driver customizing default
region info/rw/mmap of a region.
(2) provide a migration region to support migration



What's the benefit of introducing a region? It looks to me we don't 
expect the region to be accessed directly from guest. Could we simply 
extend device fd ioctl for doing such things?




(3) provide a dynamic trap bar info region to allow vendor driver
control trap/untrap of device pci bars

This vfio-pci + mediate ops way differs from mdev way in that
(1) medv way needs to create a 1:1 mdev device on top of one VF, device
specific mdev parent driver is bound to VF directly.
(2) vfio-pci + mediate ops way does not create mdev devices and VF
mediate driver does not bind to VFs. Instead, vfio-pci binds to VFs.

The reason why we don't choose the way of writing mdev parent driver is
that
(1) VFs are almost all the time directly passthroughed. Directly binding
to vfio-pci can make most of the code shared/reused.



Can we split out the common parts from vfio-pci?



  If we write a
vendor specific mdev parent driver, most of the code (like passthrough
style of rw/mmap) still needs to be copied from vfio-pci driver, which is
actually a duplicated and tedious work.



The mediate ops looks quite similar to what vfio-mdev did. And it looks 
to me we need to consider live migration for mdev as well. In that case, 
do we still expect me

Re: [PATCH v9 Kernel 2/5] vfio iommu: Add ioctl defination to get dirty pages bitmap.

[Alex Williamson]

On Thu, 5 Dec 2019 11:49:00 +0530
Kirti Wankhede  wrote:

> On 12/5/2019 11:26 AM, Alex Williamson wrote:
> > On Thu, 5 Dec 2019 11:12:23 +0530
> > Kirti Wankhede  wrote:
> >   
> >> On 12/5/2019 6:58 AM, Yan Zhao wrote:  
> >>> On Thu, Dec 05, 2019 at 02:34:57AM +0800, Alex Williamson wrote:  
>  On Wed, 4 Dec 2019 23:40:25 +0530
>  Kirti Wankhede  wrote:
>  
> > On 12/3/2019 11:34 PM, Alex Williamson wrote:  
> >> On Mon, 25 Nov 2019 19:57:39 -0500
> >> Yan Zhao  wrote:
> >> 
> >>> On Fri, Nov 15, 2019 at 05:06:25AM +0800, Alex Williamson wrote:  
>  On Fri, 15 Nov 2019 00:26:07 +0530
>  Kirti Wankhede  wrote:
> 
> > On 11/14/2019 1:37 AM, Alex Williamson wrote:  
> >> On Thu, 14 Nov 2019 01:07:21 +0530
> >> Kirti Wankhede  wrote:
> >>  
> >>> On 11/13/2019 4:00 AM, Alex Williamson wrote:  
>  On Tue, 12 Nov 2019 22:33:37 +0530
>  Kirti Wankhede  wrote:
>  
> > All pages pinned by vendor driver through vfio_pin_pages API 
> > should be
> > considered as dirty during migration. IOMMU container maintains 
> > a list of
> > all such pinned pages. Added an ioctl defination to get bitmap 
> > of such  
> 
>  definition
>  
> > pinned pages for requested IO virtual address range.  
> 
>  Additionally, all mapped pages are considered dirty when 
>  physically
>  mapped through to an IOMMU, modulo we discussed devices opting 
>  in to
>  per page pinning to indicate finer granularity with a TBD 
>  mechanism to
>  figure out if any non-opt-in devices remain.
>  
> >>>
> >>> You mean, in case of device direct assignment (device pass 
> >>> through)?  
> >>
> >> Yes, or IOMMU backed mdevs.  If vfio_dmas in the container are 
> >> fully
> >> pinned and mapped, then the correct dirty page set is all mapped 
> >> pages.
> >> We discussed using the vpfn list as a mechanism for vendor drivers 
> >> to
> >> reduce their migration footprint, but we also discussed that we 
> >> would
> >> need a way to determine that all participants in the container have
> >> explicitly pinned their working pages or else we must consider the
> >> entire potential working set as dirty.
> >>  
> >
> > How can vendor driver tell this capability to iommu module? Any 
> > suggestions?  
> 
>  I think it does so by pinning pages.  Is it acceptable that if the
>  vendor driver pins any pages, then from that point forward we 
>  consider
>  the IOMMU group dirty page scope to be limited to pinned pages?  
>  There  
> >>> we should also be aware of that dirty page scope is pinned pages + 
> >>> unpinned pages,
> >>> which means ever since a page is pinned, it should be regarded as 
> >>> dirty
> >>> no matter whether it's unpinned later. only after log_sync is called 
> >>> and
> >>> dirty info retrieved, its dirty state should be cleared.  
> >>
> >> Yes, good point.  We can't just remove a vpfn when a page is unpinned
> >> or else we'd lose information that the page potentially had been
> >> dirtied while it was pinned.  Maybe that vpfn needs to move to a dirty
> >> list and both the currently pinned vpfns and the dirty vpfns are walked
> >> on a log_sync.  The dirty vpfns list would be cleared after a log_sync.
> >> The container would need to know that dirty tracking is enabled and
> >> only manage the dirty vpfns list when necessary.  Thanks,
> >> 
> >
> > If page is unpinned, then that page is available in free page pool for
> > others to use, then how can we say that unpinned page has valid data?
> >
> > If suppose, one driver A unpins a page and when driver B of some other
> > device gets that page and he pins it, uses it, and then unpins it, then
> > how can we say that page has valid data for driver A?
> >
> > Can you give one example where unpinned page data is considered reliable
> > and valid?  
> 
>  We can only pin pages that the user has already allocated* and mapped
>  through the vfio DMA API.  The pinning of the page simply locks the
>  page for the vendor driver to access it and unpinning that page only
>  indicates that access is complete.  Pages are not freed when a vendor
>  driver unpins them, they still exist and at this point we're now
>  assuming the device dirtied the page while it was pinned.  Thanks,
> 
>  Alex
> 
>  * 

[PATCH] travis.yml: Drop libcap-dev

[Greg Kurz]

Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
aren't any users of libcap anymore. No need to install libcap-dev.

Signed-off-by: Greg Kurz 
---

Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
Like with the docker and the gitlab CI patches, if I get an ack from Alex
I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
begins. I'll make sure the SHA1 for Paolo's patch remains the same.

 .travis.yml |1 -
 1 file changed, 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c18a..6cb8af6fa599 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,7 +26,6 @@ addons:
   - libaio-dev
   - libattr1-dev
   - libbrlapi-dev
-  - libcap-dev
   - libcap-ng-dev
   - libgcc-4.8-dev
   - libgnutls28-dev

Re: virtiofsd: Where should it live?

[Markus Armbruster]

Daniel P. Berrangé  writes:

> On Tue, Dec 03, 2019 at 11:06:44AM +, Peter Maydell wrote:
>> On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  
>> wrote:
>> >
>> > We seem to be coming to the conclusion something that:
>> >
>> >   a) It should live in the qemu tree
>> >   b) It shouldn't live under contrib
>> >   c) We'll create a new top level, i.e. 'daemons'
>> >   d) virtiofsd will be daemons/virtiofsd
>> >
>> > Now, somethings I'm less clear on:
>> >   e) What else would move into daemons?  It was suggested
>> > that if we've got virtiofsd in there, then we should move
>> > libvhost-user - which I understand, but then it's not a
>> > 'daemons'.
>> > Are there any otehr daemons that should move?
>> 
>> I like the idea of a new top level directory, but I think
>> 'daemons' is a bit too specific -- for instance it seems to
>> me that qemu-img would be sensible to move out of the root,
>> and that's not a daemon.
>
> Do we really need an extra directory level ?

+1

> IIUC, the main point against having $GIT_ROOT/virtiofsd is that
> the root of our repo is quite cluttered already.
>
> Rather than trying to create a multi-level hierarchy which adds
> a debate around naming, why not address the clutter by moving
> *ALL* the .c/.h files out of the root so that we have a flatter
> tree:
>
>   $GITROOT
> +- qemu-system
> |   +- vl.c
> |   +- ...most other files...

Sounds good to me.

> +- qemu-img
> |   +- qemu-img.c

Perhaps this one can all go into existing block/, similar to how
pr-manager-helper.c is in scsi/, and virtfs-proxy-helper.c is in fsdev/.
Up to the block maintainers, of course.

> +- qemu-nbd
> |   +- qemu-nbd.c

block/ or nbd/?

> +- qemu-io
> |   +- qemu-io.c
> |   +- qemu-io-cmds.c

block/?

> +- qemu-bridge-helper

net/?

> |   ...
> +- qemu-edid

Has its own MAINTAINERS section, together with hw/display/edit* and
include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
idea.  Gerd?

> +- qemu-keymap

Not covered by MAINTAINERS.  scripts/get_maintainer.pl --git-blame
points to Gerd.

> +- qga  (already exists)

Yes.

> Then we can add virtiofsd and other programs at the root with no big
> issue.

We don't *have* to put each program into its own directory.  Simple ones
could also share one.  We just need a directory name.

Re: [PATCH] travis.yml: Drop libcap-dev

[Thomas Huth]

On 04/12/2019 09.01, Greg Kurz wrote:
> Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
> aren't any users of libcap anymore. No need to install libcap-dev.
> 
> Signed-off-by: Greg Kurz 
> ---
> 
> Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
> Like with the docker and the gitlab CI patches, if I get an ack from Alex
> I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
> begins. I'll make sure the SHA1 for Paolo's patch remains the same.

I hope you don't have to rebase! Otherwise simply say "One of the
previous changes ..." or so in the commit message instead.

>  .travis.yml |1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/.travis.yml b/.travis.yml
> index 445b0646c18a..6cb8af6fa599 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -26,7 +26,6 @@ addons:
>- libaio-dev
>- libattr1-dev
>- libbrlapi-dev
> -  - libcap-dev
>- libcap-ng-dev
>- libgcc-4.8-dev
>- libgnutls28-dev

Reviewed-by: Thomas Huth 

Re: [PATCH] travis.yml: Drop libcap-dev

[Greg Kurz]

On Wed, 4 Dec 2019 09:07:42 +0100
Thomas Huth  wrote:

> On 04/12/2019 09.01, Greg Kurz wrote:
> > Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
> > aren't any users of libcap anymore. No need to install libcap-dev.
> > 
> > Signed-off-by: Greg Kurz 
> > ---
> > 
> > Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
> > Like with the docker and the gitlab CI patches, if I get an ack from Alex
> > I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
> > begins. I'll make sure the SHA1 for Paolo's patch remains the same.
> 
> I hope you don't have to rebase! Otherwise simply say "One of the
> previous changes ..." or so in the commit message instead.
> 

I don't expect to rebase, hence the SHA1.

> >  .travis.yml |1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/.travis.yml b/.travis.yml
> > index 445b0646c18a..6cb8af6fa599 100644
> > --- a/.travis.yml
> > +++ b/.travis.yml
> > @@ -26,7 +26,6 @@ addons:
> >- libaio-dev
> >- libattr1-dev
> >- libbrlapi-dev
> > -  - libcap-dev
> >- libcap-ng-dev
> >- libgcc-4.8-dev
> >- libgnutls28-dev
> 
> Reviewed-by: Thomas Huth 
> 

Thanks.

Re: virtiofsd: Where should it live?

[Gerd Hoffmann]

  Hi,

> > |   ...
> > +- qemu-edid
> 
> Has its own MAINTAINERS section, together with hw/display/edit* and
> include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
> idea.  Gerd?

Sort-of makes sense.  My personal preference would be a tools/ directory
for all those small utilities though.

> > +- qemu-keymap
> 
> Not covered by MAINTAINERS.  scripts/get_maintainer.pl --git-blame
> points to Gerd.

Generates the keymaps in pc-bios/keymaps/

cheers,
  Gerd

Re: [PATCH] scripts: Fix undefinited name 'file' error for python3

[Thomas Huth]

On 04/12/2019 07.48, Han Han wrote:
> Anyone help to review it?

 Hi!

When sending patches to the qemu-devel mailing list, please always make
sure to put the corresponding maintainers on CC:, otherwise your mails
might get lost in the high traffic of the mailing list. For this case,
it would have been good to CC: the "Migration" and "Python script"
maintainers, see the corresponding sections of the MAINTAINERS file in
the top most directory of the QEMU sources.

Anyway, it seems someone else ran into the same issue already, too, and
 it got already fixed here:

https://git.qemu.org/?p=qemu.git;a=commitdiff;h=e8d0ac5801edda91412e5

  Thomas


> On Tue, Nov 26, 2019 at 1:54 PM Han Han  > wrote:
> 
> ping
> 
> On Wed, Nov 13, 2019 at 9:17 PM Han Han  > wrote:
> 
> In python3, 'file' is no longer a keyword for file type object.
> So it
> will can error when run the scripts by python3:
> 
> $ python3 ./scripts/vmstate-static-checker.py -s 4.0.json -d
> 4.1.json
> Traceback (most recent call last):
>   File "./scripts/vmstate-static-checker.py", line 431, in 
>     sys.exit(main())
>   File "./scripts/vmstate-static-checker.py", line 378, in main
>     parser.add_argument('-s', '--src', type=file, required=True,
> NameError: name 'file' is not defined
> 
> Replace file type to argparse.FileType('r').
> 
> Signed-off-by: Han Han mailto:h...@redhat.com>>
> ---
>  scripts/vmstate-static-checker.py | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/vmstate-static-checker.py
> b/scripts/vmstate-static-checker.py
> index d3467288..14f199a0 100755
> --- a/scripts/vmstate-static-checker.py
> +++ b/scripts/vmstate-static-checker.py
> @@ -375,9 +375,9 @@ def main():
>      help_text = "Parse JSON-formatted vmstate dumps from QEMU
> in files SRC and DEST.  Checks whether migration from SRC to
> DEST QEMU versions would break based on the VMSTATE information
> contained within the JSON outputs.  The JSON output is created
> from a QEMU invocation with the -dump-vmstate parameter and a
> filename argument to it.  Other parameters to QEMU do not
> matter, except the -M (machine type) parameter."
> 
>      parser = argparse.ArgumentParser(description=help_text)
> -    parser.add_argument('-s', '--src', type=file, required=True,
> +    parser.add_argument('-s', '--src',
> type=argparse.FileType('r'), required=True,
>                          help='json dump from src qemu')
> -    parser.add_argument('-d', '--dest', type=file, required=True,
> +    parser.add_argument('-d', '--dest',
> type=argparse.FileType('r'), required=True,
>                          help='json dump from dest qemu')
>      parser.add_argument('--reverse', required=False, default=False,
>                          action='store_true',
> -- 
> 2.23.0
> 
> 
> 
> -- 
> Best regards,
> ---
> Han Han
> Quality Engineer
> Redhat.
> 
> Email: h...@redhat.com 
> Phone: +861065339333

[PATCH v3] travis.yml: Run tcg tests with tci

[Thomas Huth]

So far we only have compile coverage for tci. But since commit
2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
for INDEX_op_ld16u_i64") has been included now, we can also run the
"tcg" and "qtest" tests with tci, so let's enable them in Travis now.
Since we don't gain much additional test coverage by compiling all
targets, and TCI is broken e.g. with the Sparc targets, we also limit
the target list to a reasonable subset now (which should still get us
test coverage by tests/boot-serial-test for example).

Tested-by: Stefan Weil 
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Thomas Huth 
---
 v3:
 - Add --disable-kvm option since we're only interested in TCG here

 .travis.yml | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c1..d73e2fb744 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -215,10 +215,11 @@ matrix:
 - TEST_CMD=""
 
 
-# We manually include builds which we disable "make check" for
+# Check the TCG interpreter (TCI)
 - env:
-- CONFIG="--enable-debug --enable-tcg-interpreter"
-- TEST_CMD=""
+- CONFIG="--enable-debug --enable-tcg-interpreter --disable-kvm 
--disable-containers
+
--target-list=alpha-softmmu,arm-softmmu,hppa-softmmu,m68k-softmmu,microblaze-softmmu,moxie-softmmu,ppc-softmmu,s390x-softmmu,x86_64-softmmu"
+- TEST_CMD="make check-qtest check-tcg V=1"
 
 
 # We don't need to exercise every backend with every front-end
-- 
2.18.1

Re: Network connection with COLO VM

[Zhang, Chen]

On 12/3/2019 9:25 PM, Dr. David Alan Gilbert wrote:

* Daniel Cho (daniel...@qnap.com) wrote:

Hi Dave,

We could use the exist interface to add netfilter and chardev, it might not
have the problem you said.

However, the netfilter and chardev on the primary at the start, that means
we could not dynamic set COLO
feature to VM?

I wasn't expecting that to be possible - I'd expect you to be able
to start in a state that looks the same as a primary+failed secondary;
but I'm not sure.


Current COLO (with Lukas's patch) can support dynamic set COLO system.

This status is same like the secondary has triggered failover, the 
primary node need to find new secondary


node to combine new COLO system.



We try to change this chardev to prevent primary VM will stuck to wait
secondary VM.

-chardev socket,id=compare1,host=127.0.0.1,port=9004,server,wait \

to

-chardev socket,id=compare1,host=127.0.0.1,port=9004,server,nowait \

But it will make primary VM's network not works. (Can't get ip), until
starting connect with secondary VM.


I think you need to check the port 9004 if already connect to the pair node.


I'm not sure of the answer to this; I've not tried doing it - I'm not
sure anyone has!
But, the colo components do track the state of tcp connections, so I'm
expecting that they have to already exist to have the state of those
connections available for when you start the secondary.


Yes, you are right.

For this status, we don't need to sync the state of tcp connections, 
because after failover


(or just solo COLO primary node), we have empty all the tcp connections 
state in COLO module.


In the processing of build new COLO pair, we will sync all the VM state 
to secondary node and re-build


new track things in COLO module.






Otherwise, the primary VM with netfileter / chardev and without netfilter /
chardev , they takes very different
booting time.
Without  netfilter / chardev : about 1 mins
With   netfilter / chardev : about 5 mins
Is this an issue?

that sounds like it needs investigating.

Dave


Yes, In previous COLO use cases, we need make primary node and secondary 
node boot in the same time.


I did't expect such a big difference on netfilter/chardev.

I think you can try without netfilter/chardev, after VM boot, re-build 
the netfilter/chardev related work with chardev server nowait.



Thanks

Zhang Chen




Best regards,
Daniel Cho


Dr. David Alan Gilbert  於 2019年12月2日 週一 下午5:58寫道：


* Daniel Cho (daniel...@qnap.com) wrote:

Hi Zhang,

We use qemu-4.1.0 release on this case.

I think we need use block mirror to sync the disk to secondary node

first,

then stop the primary VM and build COLO system.

In the stop moment, you need add some netfilter and chardev socket node

for

COLO, maybe you need re-check this part.


Our test was already follow those step. Maybe I could describe the detail
of the test flow and issues.


Step 1:

Create primary VM without any netfilter and chardev for COLO, and using
other host ping primary VM continually.


Step 2:

Create secondary VM (the same device/drive with primary VM), and do block
mirror sync ( ping to primary VM normally )


Step 3:

After block mirror sync finish, add those netfilter and chardev to

primary

VM and secondary VM for COLO ( *Can't* ping to primary VM but those

packets

will be received later )


Step 4:

Start migrate primary VM to secondary VM, and primary VM & secondary VM

are

running ( ping to primary VM works and receive those packets on step 3
status )




Between Step 3 to Step 4, it will take 10~20 seconds in our environment.

I could image this issue (delay reply packets) is because of setting COLO
proxy for temporary status,

but we thought 10~20 seconds might a little long. (If primary VM is

already

doing some jobs, it might lose the data.)


Could we reduce those time? or those delay is depends on different VM?

I think you need to set up the netfilter and chardev on the primary at
the start;  the filter contains the state of the TCP connections working
with the VM, so adding it later can't gain that state for existing
connections.

Dave


Best Regard,

Daniel Cho.



Zhang, Chen  於 2019年11月30日 週六 上午2:04寫道：





*From:* Daniel Cho 
*Sent:* Friday, November 29, 2019 10:43 AM
*To:* Zhang, Chen 
*Cc:* Dr. David Alan Gilbert ;

lukasstra...@web.de;

qemu-devel@nongnu.org
*Subject:* Re: Network connection with COLO VM



Hi David,  Zhang,



Thanks for replying my question.

We know why will occur this issue.

As you said, the COLO VM's network needs

colo-proxy to control packets, so the guest's

interface should set the filter to solve the problem.



But we found another question, when we set the

fault-tolerance feature to guest (primary VM is running,

secondary VM is pausing), the guest's network would not

responds any request for a while (in our environment

about 20~30 secs) after secondary VM runs.



Does it be a normal situation, or a known issue?



Our test is creating primary VM for a while, then 

Re: [PATCH] scripts: Fix undefinited name 'file' error for python3

[Han Han]

On Wed, Dec 4, 2019 at 4:23 PM Thomas Huth  wrote:

> On 04/12/2019 07.48, Han Han wrote:
> > Anyone help to review it?
>
>  Hi!
>
> When sending patches to the qemu-devel mailing list, please always make
> sure to put the corresponding maintainers on CC:, otherwise your mails
> might get lost in the high traffic of the mailing list. For this case,
> it would have been good to CC: the "Migration" and "Python script"
> maintainers, see the corresponding sections of the MAINTAINERS file in
> the top most directory of the QEMU sources.
>
OK. Thanks for your advice

>
> Anyway, it seems someone else ran into the same issue already, too, and
>  it got already fixed here:
>
> https://git.qemu.org/?p=qemu.git;a=commitdiff;h=e8d0ac5801edda91412e5
>
>   Thomas
>
>
> > On Tue, Nov 26, 2019 at 1:54 PM Han Han  > > wrote:
> >
> > ping
> >
> > On Wed, Nov 13, 2019 at 9:17 PM Han Han  > > wrote:
> >
> > In python3, 'file' is no longer a keyword for file type object.
> > So it
> > will can error when run the scripts by python3:
> >
> > $ python3 ./scripts/vmstate-static-checker.py -s 4.0.json -d
> > 4.1.json
> > Traceback (most recent call last):
> >   File "./scripts/vmstate-static-checker.py", line 431, in
> 
> > sys.exit(main())
> >   File "./scripts/vmstate-static-checker.py", line 378, in main
> > parser.add_argument('-s', '--src', type=file, required=True,
> > NameError: name 'file' is not defined
> >
> > Replace file type to argparse.FileType('r').
> >
> > Signed-off-by: Han Han mailto:h...@redhat.com
> >>
> > ---
> >  scripts/vmstate-static-checker.py | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/scripts/vmstate-static-checker.py
> > b/scripts/vmstate-static-checker.py
> > index d3467288..14f199a0 100755
> > --- a/scripts/vmstate-static-checker.py
> > +++ b/scripts/vmstate-static-checker.py
> > @@ -375,9 +375,9 @@ def main():
> >  help_text = "Parse JSON-formatted vmstate dumps from QEMU
> > in files SRC and DEST.  Checks whether migration from SRC to
> > DEST QEMU versions would break based on the VMSTATE information
> > contained within the JSON outputs.  The JSON output is created
> > from a QEMU invocation with the -dump-vmstate parameter and a
> > filename argument to it.  Other parameters to QEMU do not
> > matter, except the -M (machine type) parameter."
> >
> >  parser = argparse.ArgumentParser(description=help_text)
> > -parser.add_argument('-s', '--src', type=file, required=True,
> > +parser.add_argument('-s', '--src',
> > type=argparse.FileType('r'), required=True,
> >  help='json dump from src qemu')
> > -parser.add_argument('-d', '--dest', type=file,
> required=True,
> > +parser.add_argument('-d', '--dest',
> > type=argparse.FileType('r'), required=True,
> >  help='json dump from dest qemu')
> >  parser.add_argument('--reverse', required=False,
> default=False,
> >  action='store_true',
> > --
> > 2.23.0
> >
> >
> >
> > --
> > Best regards,
> > ---
> > Han Han
> > Quality Engineer
> > Redhat.
> >
> > Email: h...@redhat.com 
> > Phone: +861065339333
>
>

-- 
Best regards,
---
Han Han
Quality Engineer
Redhat.

Email: h...@redhat.com
Phone: +861065339333

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Pankaj Gupta]

> From: Pan Nengyuan 
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq
> pointer.
> 
> Signed-off-by: Michael S. Tsirkin 
> Signed-off-by: Pan Nengyuan 
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c | 16 +++-
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>  return &vdev->vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +vq->vring.num = 0;
> +vq->vring.num_default = 0;
> +vq->handle_output = NULL;
> +vq->handle_aio_output = NULL;
> +g_free(vq->used_elems);
> +vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>  if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>  abort();
>  }
>  
> -vdev->vq[n].vring.num = 0;
> -vdev->vq[n].vring.num_default = 0;
> -vdev->vq[n].handle_output = NULL;
> -vdev->vq[n].handle_aio_output = NULL;
> -g_free(vdev->vq[n].used_elems);
> +virtio_delete_queue(&vdev->vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int
> queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>  unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> --
> 2.7.2.windows.1
> 
> 
Overall it ooks good to me.

Just one point: e.g in virtio_rng: "virtio_rng_device_unrealize" function
We are doing : virtio_del_queue(vdev, 0);

One can directly call "virtio_delete_queue". It can become confusing
to call multiple functions for same purpose. Instead, Can we make 
"virtio_delete_queue" static inline?

Other than that:
Reviewed-by: Pankaj Gupta 

> 
> 

Re: [PATCH 00/21] Error handling fixes, may contain 4.2 material

[Markus Armbruster]

"Michael S. Tsirkin"  writes:

> On Sat, Nov 30, 2019 at 08:42:19PM +0100, Markus Armbruster wrote:
>> PATCH 2-4 fix crash bugs.  Including them would be a no-brainer at
>> -rc0.  But we're post -rc3, and even for crash bugs we require a
>> certain likelihood of users getting bitten.
>> 
>> Jens, please assess impact of PATCH 2's crash bug.
>> 
>> Kevin, please do the same for PATCH 3.
>> 
>> Daniel, please do the same for PATCH 4.
>
> virtio things:
>
> Reviewed-by: Michael S. Tsirkin 

In my haste to get this into -rc4, I lost your r-bys.  Sorry about that!

> Jason do you want to pick these?

Merged in commit 39032981fa851d25fb27527f25f046fed800e585.

[PATCH] target/i386: relax assert when old host kernels don't include msrs

[Catherine Ho]

Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
add vmx msr entry although older host kernels don't include them.

But old host kernel + newest qemu will cause a qemu crash as follows:
qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
cpu->kvm_msr_buf->nmsrs' failed.

This fixes it by relaxing the condition.

Cc: Paolo Bonzini 
Signed-off-by: Catherine Ho 
---
 target/i386/kvm.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target/i386/kvm.c b/target/i386/kvm.c
index bf16556..a8c44bf 100644
--- a/target/i386/kvm.c
+++ b/target/i386/kvm.c
@@ -2936,7 +2936,7 @@ static int kvm_put_msrs(X86CPU *cpu, int level)
  (uint32_t)e->index, (uint64_t)e->data);
 }
 
-assert(ret == cpu->kvm_msr_buf->nmsrs);
+assert(ret <= cpu->kvm_msr_buf->nmsrs);
 return 0;
 }
 
-- 
1.7.1

Re: [RFC] QEMU Gating CI

[Thomas Huth]

On 03/12/2019 15.07, Alex Bennée wrote:
[...]
>> GitLab Jobs and Pipelines
>> -
>>
>> GitLab CI is built around two major concepts: jobs and pipelines.  The
>> current GitLab CI configuration in QEMU uses jobs only (or putting it
>> another way, all jobs in a single pipeline stage).

Yeah, the initial gitlab-ci.yml file was one of the very first YAML file
and one the very first CI files that I wrote, with hardly any experience
in this area ... there is definitely a lot of room for improvement here!

>>  Consider the
>> folowing job definition[9]:
>>
>>build-tci:
>> script:
>> - TARGETS="aarch64 alpha arm hppa m68k microblaze moxie ppc64 s390x 
>> x86_64"
>> - ./configure --enable-tcg-interpreter
>>  --target-list="$(for tg in $TARGETS; do echo -n ${tg}'-softmmu '; 
>> done)"
>> - make -j2
>> - make tests/boot-serial-test tests/cdrom-test tests/pxe-test
>> - for tg in $TARGETS ; do
>> export QTEST_QEMU_BINARY="${tg}-softmmu/qemu-system-${tg}" ;
>> ./tests/boot-serial-test || exit 1 ;
>> ./tests/cdrom-test || exit 1 ;
>>   done
>> - QTEST_QEMU_BINARY="x86_64-softmmu/qemu-system-x86_64" ./tests/pxe-test
>> - QTEST_QEMU_BINARY="s390x-softmmu/qemu-system-s390x" ./tests/pxe-test 
>> -m slow
>>
>> All the lines under "script" are performed sequentially.  It should be
>> clear that there's the possibility of breaking this down into multiple
>> stages, so that a build happens first, and then "common set of tests"
>> run in parallel.  Using the example above, it would look something
>> like:
>>
>>+---++
>>|  BUILD STAGE  |TEST STAGE  |
>>+---++
>>|   +---+   |  +--+  |
>>|   | build |   |  | boot-serial-test |  |
>>|   +---+   |  +--+  |
>>|   ||
>>|   |  +--+  |
>>|   |  | cdrom-test   |  |
>>|   |  +--+  |
>>|   ||
>>|   |  +--+  |
>>|   |  | x86_64-pxe-test  |  |
>>|   |  +--+  |
>>|   ||
>>|   |  +--+  |
>>|   |  | s390x-pxe-test   |  |
>>|   |  +--+  |
>>|   ||
>>+---++
>>
>> Of course it would be silly to break down that job into smaller jobs that
>> would run individual tests like "boot-serial-test" or "cdrom-test".  Still,
>> the pipeline approach is valid because:
>>
>>  * Common set of tests would run in parallel, giving a quicker result
>>turnaround

Ok, full ack for the idea to use separate pipelines for the testing
(Philippe once showed me this idea already, too, he's using it for EDK2
testing IIRC). But the example with the build-tci is quite bad. The
single steps here are basically just a subset of "check-qtest" to skip
the tests that we are not interested in here. If we don't care about
losing some minutes of testing, we can simply replace all those steps
with "make check-qtest" again.

I think what we really want to put into different pipelines are the
sub-steps of "make check", i.e.:

- check-block
- check-qapi-schema
- check-unit
- check-softfloat
- check-qtest
- check-decodetree

And of course also the other ones that are not included in "make check"
yet, e.g. "check-acceptance" etc.

> check-unit is a good candidate for parallel tests. The others depends -
> I've recently turned most make check's back to -j 1 on travis because
> it's a real pain to see what test has hung when other tests keep
> running.

If I understood correctly, it's not about running the check steps in
parallel with "make -jXX" in one pipeline, but rather about running the
different test steps in different pipelines. So you get a separate
output for each test subsystem.

>> Current limitations for a multi-stage pipeline
>> ~~
>>
>> Because it's assumed that each job will happen in an isolated and
>> independent execution environment, jobs must explicitly define the
>> resources that will be shared between stages.  GitLab will make sure
>> the same source code revision will be available on all jobs
>> automatically.  Additionaly, GitLab supports the concept of artifacts.
>> By defining artifacts in the "build" stage, jobs in the "test" stage
>> can expect to have a copy of those artifacts automatically.
>>
>> In theory, there's nothing that prevents an entire QEMU build
>> directory, to be treated as an artifact.  In practice, there are
>> predefined limits on GitLab that prevents that from being possible,
>> resulting in errors such as:
>>
>>Uploading artifacts...
>>build: found 3164 matching fil

Re: [PATCH] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Laurent Vivier]

On 04/12/2019 04:02, pannengyuan wrote:
> 
> 
> On 2019/12/3 16:32, Laurent Vivier wrote:
>> On 03/12/2019 01:53, pannengyuan wrote:
>>>
>>>
>>> On 2019/12/2 21:58, Laurent Vivier wrote:
 On 02/12/2019 12:15, pannengy...@huawei.com wrote:
> From: PanNengyuan 
>
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
>
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized 
> /mnt/sdb/qemu-4.2.0-rc0/hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool 
> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject 
> /mnt/sdb/qemu-4.2.0-rc0/qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool 
> /mnt/sdb/qemu-4.2.0-rc0/qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize 
> /mnt/sdb/qemu-4.2.0-rc0/hw/virtio/virtio-pci.c:1801
>
> Reported-by: Euler Robot 
> Signed-off-by: PanNengyuan 
> ---
>  hw/char/virtio-serial-bus.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..da9019a 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,15 @@ static void 
> virtio_serial_device_unrealize(DeviceState *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> +int i;
>  
>  QLIST_REMOVE(vser, next);
>  
> +for (i = 0; i <= vser->bus.max_nr_ports; i++) {
> +virtio_del_queue(vdev, 2 * i);
> +virtio_del_queue(vdev, 2 * i + 1);
> +}
> +

 According to virtio_serial_device_realize() and the number of
 virtio_add_queue(), I think you have more queues to delete:

   4 + 2 * vser->bus.max_nr_ports

 (for vser->ivqs[0], vser->ovqs[0], vser->c_ivq, vser->c_ovq,
 vser->ivqs[i], vser->ovqs[i]).

 Thanks,
 Laurent


>>> Thanks, but I think the queues is correct, the queues in
>>> virtio_serial_device_realize is as follow:
>>>
>>> // here is 2
>>> vser->ivqs[0] = virtio_add_queue(vdev, 128, handle_input);
>>> vser->ovqs[0] = virtio_add_queue(vdev, 128, handle_output);
>>>
>>> // here is 2
>>> vser->c_ivq = virtio_add_queue(vdev, 32, control_in);
>>> vser->c_ovq = virtio_add_queue(vdev, 32, control_out);
>>>
>>> // here 2 * (max_nr_ports - 1)  - i is from 1 to max_nr_ports - 1
>>> for (i = 1; i < vser->bus.max_nr_ports; i++) {
>>> vser->ivqs[i] = virtio_add_queue(vdev, 128, handle_input);
>>> vser->ovqs[i] = virtio_add_queue(vdev, 128, handle_output);
>>> }
>>>
>>> so the total queues number is:  2 * (vser->bus.max_nr_ports + 1)
>>>
>>
>> Yes, you're right. A comment in the code would have helped or written
>> clearly like:
>>
>> for (i = 0; i < 2 * (vser->bus.max_nr_ports + 1); i++) {
>> virtio_del_queue(vdev, i);
>> }
>>
>> Thanks,
>> Laurent
>>
>>
> yes, it would be helpful, Michael S. Tsirkin posted another way to make
> it more clear, I will reuse it in next version.

Yes, the proposition from Michael is much more better.

Thanks,
Laurent

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Janosch Frank]

On 12/3/19 6:44 PM, Cornelia Huck wrote:
> On Tue,  3 Dec 2019 08:28:11 -0500
> Janosch Frank  wrote:
> 
>> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
>> for the initial reset, and that was also called for the clear
>> reset. To be architecture compliant, we also need to clear local
>> interrupts on a normal reset.
>>
>> Because of this and the upcoming protvirt support we need to add
>> ioctls for the missing clear and normal resets.
>>
>> Signed-off-by: Janosch Frank 
>> Reviewed-by: Thomas Huth 
>> Acked-by: David Hildenbrand 
>> ---
>>  target/s390x/cpu.c   | 16 +--
>>  target/s390x/kvm-stub.c  | 10 +-
>>  target/s390x/kvm.c   | 42 
>>  target/s390x/kvm_s390x.h |  4 +++-
>>  4 files changed, 60 insertions(+), 12 deletions(-)
>>
>> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
>> index 829ce6ad54..4973365d6c 100644
>> --- a/target/s390x/cpu.c
>> +++ b/target/s390x/cpu.c
>> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, cpu_reset_type 
>> type)
>>  }
>>  
>>  /* Reset state inside the kernel that we cannot access yet from QEMU. */
> 
> For the last iteration, I asked about the 'yet' here...

I have not written those comments, I merely refuse to delete them :)
We still reset some state in the kernel, I'm not sure how much of that
is already exposed via ioctls to QEMU, so I won't remove the comment.

> 
>> -if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
>> -kvm_s390_reset_vcpu(cpu);
>> +if (kvm_enabled()) {
>> +switch (type) {
>> +case S390_CPU_RESET_CLEAR:
>> +kvm_s390_reset_vcpu_clear(cpu);
>> +break;
>> +case S390_CPU_RESET_INITIAL:
>> +kvm_s390_reset_vcpu_initial(cpu);
>> +break;
>> +case S390_CPU_RESET_NORMAL:
>> +kvm_s390_reset_vcpu_normal(cpu);
>> +break;
>> +default:
>> +g_assert_not_reached();
>> +}
>>  }
>>  }
>>  
> 
> (...)
> 
>> @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>  return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>  CPUState *cs = CPU(cpu);
>>  
>> -/* The initial reset call is needed here to reset in-kernel
>> - * vcpu data that we can't access directly from QEMU
>> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> - * Before this ioctl cpu_synchronize_state() is called in common kvm
>> - * code (kvm-all) */
>> -if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>> -error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>> +/*
>> + * The reset call is needed here to reset in-kernel vcpu data that
>> + * we can't access directly from QEMU (i.e. with older kernels
>> + * which don't support sync_regs/ONE_REG).  Before this ioctl
> 
> ...and this reference to 'older kernels' here.
> 
> Are the comments still correct/relevant?

See above

> 
>> + * cpu_synchronize_state() is called in common kvm code
>> + * (kvm-all).
>> + */
>> +if (kvm_vcpu_ioctl(cs, type)) {
>> +error_report("CPU reset failed on CPU %i type %lx",
>> + cs->cpu_index, type);
>> +}
>> +}
> 




signature.asc
Description: OpenPGP digital signature

[PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[Han Han]

This reverts commit bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8.

In 20a1922032 we allowed reboot-timeout=-1 again, so update the doc
accordingly.
---
 qemu-options.hx | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/qemu-options.hx b/qemu-options.hx
index 65c9473b73..e14d88e9b2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -327,8 +327,8 @@ format(true color). The resolution should be supported by 
the SVGA mode, so
 the recommended is 320x240, 640x480, 800x640.
 
 A timeout could be passed to bios, guest will pause for @var{rb_timeout} ms
-when boot failed, then reboot. If @option{reboot-timeout} is not set,
-guest will not reboot by default. Currently Seabios for X86
+when boot failed, then reboot. If @var{rb_timeout} is '-1', guest will not
+reboot, qemu passes '-1' to bios by default. Currently Seabios for X86
 system support it.
 
 Do strict boot via @option{strict=on} as far as firmware/BIOS
-- 
2.24.0.rc1

Re: [PATCH v3 2/4] s390x: Add missing vcpu reset functions

[Cornelia Huck]

On Wed, 4 Dec 2019 10:00:45 +0100
Janosch Frank  wrote:

> On 12/3/19 6:44 PM, Cornelia Huck wrote:
> > On Tue,  3 Dec 2019 08:28:11 -0500
> > Janosch Frank  wrote:
> >   
> >> Up to now we only had an ioctl to reset vcpu data QEMU couldn't reach
> >> for the initial reset, and that was also called for the clear
> >> reset. To be architecture compliant, we also need to clear local
> >> interrupts on a normal reset.
> >>
> >> Because of this and the upcoming protvirt support we need to add
> >> ioctls for the missing clear and normal resets.
> >>
> >> Signed-off-by: Janosch Frank 
> >> Reviewed-by: Thomas Huth 
> >> Acked-by: David Hildenbrand 
> >> ---
> >>  target/s390x/cpu.c   | 16 +--
> >>  target/s390x/kvm-stub.c  | 10 +-
> >>  target/s390x/kvm.c   | 42 
> >>  target/s390x/kvm_s390x.h |  4 +++-
> >>  4 files changed, 60 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> >> index 829ce6ad54..4973365d6c 100644
> >> --- a/target/s390x/cpu.c
> >> +++ b/target/s390x/cpu.c
> >> @@ -139,8 +139,20 @@ static void s390_cpu_reset(CPUState *s, 
> >> cpu_reset_type type)
> >>  }
> >>  
> >>  /* Reset state inside the kernel that we cannot access yet from QEMU. 
> >> */  
> > 
> > For the last iteration, I asked about the 'yet' here...  
> 
> I have not written those comments, I merely refuse to delete them :)
> We still reset some state in the kernel, I'm not sure how much of that
> is already exposed via ioctls to QEMU, so I won't remove the comment.
> 
> >   
> >> -if (kvm_enabled() && type != S390_CPU_RESET_NORMAL) {
> >> -kvm_s390_reset_vcpu(cpu);
> >> +if (kvm_enabled()) {
> >> +switch (type) {
> >> +case S390_CPU_RESET_CLEAR:
> >> +kvm_s390_reset_vcpu_clear(cpu);
> >> +break;
> >> +case S390_CPU_RESET_INITIAL:
> >> +kvm_s390_reset_vcpu_initial(cpu);
> >> +break;
> >> +case S390_CPU_RESET_NORMAL:
> >> +kvm_s390_reset_vcpu_normal(cpu);
> >> +break;
> >> +default:
> >> +g_assert_not_reached();
> >> +}
> >>  }
> >>  }
> >>
> > 
> > (...)
> >   
> >> @@ -403,17 +405,41 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
> >>  return 0;
> >>  }
> >>  
> >> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> >> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
> >>  {
> >>  CPUState *cs = CPU(cpu);
> >>  
> >> -/* The initial reset call is needed here to reset in-kernel
> >> - * vcpu data that we can't access directly from QEMU
> >> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> >> - * Before this ioctl cpu_synchronize_state() is called in common kvm
> >> - * code (kvm-all) */
> >> -if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
> >> -error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
> >> +/*
> >> + * The reset call is needed here to reset in-kernel vcpu data that
> >> + * we can't access directly from QEMU (i.e. with older kernels
> >> + * which don't support sync_regs/ONE_REG).  Before this ioctl  
> > 
> > ...and this reference to 'older kernels' here.
> > 
> > Are the comments still correct/relevant?  
> 
> See above

Fair enough, let's keep them for now and revisit this later.

> 
> >   
> >> + * cpu_synchronize_state() is called in common kvm code
> >> + * (kvm-all).
> >> + */
> >> +if (kvm_vcpu_ioctl(cs, type)) {
> >> +error_report("CPU reset failed on CPU %i type %lx",
> >> + cs->cpu_index, type);
> >> +}
> >> +}  
> >   
> 
> 



pgpHZKd3FGrTB.pgp
Description: OpenPGP digital signature

Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.

[Damien Hedde]

On 12/2/19 3:34 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Add functions to easily add input or output clocks to a device.
>> A clock objects is added as a child of the device.
> 
> "object"
> 
>> The api is very similar the gpio's one.
> 
> "API"; "to the GPIO API".
> 
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde 
>>
> 
>> +static NamedClockList *qdev_init_clocklist(DeviceState *dev, const char 
>> *name,
>> +bool forward)
>> +{
>> +NamedClockList *ncl;
>> +
>> +/*
>> + * The clock path will be computed by the device's realize function 
>> call.
>> + * This is required to ensure the clock's canonical path is right and 
>> log
>> + * messages are meaningfull.
> 
> "meaningful"
> 
>> + */
>> +assert(name);
>> +assert(!dev->realized);
>> +
>> +/* The ncl structure will be freed in device's finalize function call */
> 
> Do you mean "in device_finalize()", or "in the finalize method
> of the device" ?  If you mean a specific function, then it's
> good to name it, so the reader can go and check that code if
> they need to confirm that there's a matching free()/deref/etc.

Yes, it is device_finalize(). I'll update the comment.

> 
>> +ncl = g_malloc0(sizeof(*ncl));
> 
> Prefer g_new0(NamedClockList, 1).
> 
>> +ncl->name = g_strdup(name);
>> +ncl->forward = forward;
>> +
>> +QLIST_INSERT_HEAD(&dev->clocks, ncl, node);
>> +return ncl;
>> +}
>> +
>> +ClockOut *qdev_init_clock_out(DeviceState *dev, const char *name)
>> +{
>> +NamedClockList *ncl;
>> +Object *clk;
>> +
>> +ncl = qdev_init_clocklist(dev, name, false);
>> +
>> +clk = object_new(TYPE_CLOCK_OUT);
>> +
>> +/* will fail if name already exists */
> 
> This is true but it would be more helpful to say
>  /*
>   * Trying to create a clock whose name clashes with some other
>   * clock or property is a bug in the caller and we will abort().
>   */
> 
> (assuming that's what's going on here).

You're right.

> 
>> +object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> +object_unref(clk); /* remove the initial ref made by object_new */
>> +
>> +ncl->out = CLOCK_OUT(clk);
>> +return ncl->out;
>> +}
>> +
>> +ClockIn *qdev_init_clock_in(DeviceState *dev, const char *name,
>> +ClockCallback *callback, void *opaque)
>> +{
>> +NamedClockList *ncl;
>> +Object *clk;
>> +
>> +ncl = qdev_init_clocklist(dev, name, false);
>> +
>> +clk = object_new(TYPE_CLOCK_IN);
>> +/*
>> + * the ref initialized by object_new will be cleared during dev 
>> finalize.
> 
> This means "in device_finalize()", I think from reading later patches ?

Yes. I'll update the comment too.
> 
>> + * It allows us to safely remove the callback.
>> + */
>> +
>> +/* will fail if name already exists */
> 
> Similar remark as for earlier comment.
> 
>> +object_property_add_child(OBJECT(dev), name, clk, &error_abort);
>> +
>> +ncl->in = CLOCK_IN(clk);
>> +if (callback) {
>> +clock_set_callback(ncl->in, callback, opaque);
>> +}
>> +return ncl->in;
>> +}
> 
>> +ClockIn *qdev_get_clock_in(DeviceState *dev, const char *name)
>> +{
>> +NamedClockList *ncl;
>> +
>> +assert(dev && name);
>> +
>> +ncl = qdev_get_clocklist(dev, name);
>> +return ncl ? ncl->in : NULL;
>> +}
> 
> Do we expect to want to be able to pass in the name of
> a clock that doesn't exist ? Should that be an error
> rather than returning NULL ?

I think it can be an error and we may assert the clock exists.

> 
>> +
>> +static ClockOut *qdev_get_clock_out(DeviceState *dev, const char *name)
>> +{
>> +NamedClockList *ncl;
>> +
>> +assert(dev && name);
>> +
>> +ncl = qdev_get_clocklist(dev, name);
>> +return ncl ? ncl->out : NULL;
> 
> Ditto.
> 
>> +}
>> +
>> +void qdev_connect_clock_out(DeviceState *dev, const char *name, ClockIn 
>> *clk,
>> +Error **errp)
>> +{
>> +ClockOut *clkout = qdev_get_clock_out(dev, name);
>> +
>> +if (!clk) {
>> +error_setg(errp, "NULL input clock");
>> +return;
>> +}
>> +
>> +if (!clkout) {
>> +error_setg(errp, "no output clock '%s' in device", name);
>> +return;
>> +}
>> +
>> +clock_connect(clk, clkout);
> 
> Do we need to support returning an error here, or would it
> always be a programming bug to try to connect a non-existent clock?

As for above, I think it can be considered an error. Should I remove the
errp and do an assert instead ?

> 
>> --- /dev/null
>> +++ b/include/hw/qdev-clock.h
>> @@ -0,0 +1,67 @@
>> +#ifndef QDEV_CLOCK_H
>> +#define QDEV_CLOCK_H
> 
> Another missing copyright/license comment.
> 
>> +
>> +#include "hw/clock.h"
>> +
>> +/**
>> + * qdev_init_clock_in:
>> + * @dev: the device in which to add a clock
> 
> "the device to add a clock input to"
> 
>> + * @name: the name of the clock (can't 

[PATCH v2 04/18] tests: Clean up initialization of Error *err variables

[Markus Armbruster]

Declaring a local Error *err without initializer looks suspicious.
Fuse the declaration with the initialization to avoid that.

Signed-off-by: Markus Armbruster 
---
 tests/test-qobject-output-visitor.c | 8 
 tests/test-string-output-visitor.c  | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tests/test-qobject-output-visitor.c 
b/tests/test-qobject-output-visitor.c
index 3e993e5ba8..d7761ebf84 100644
--- a/tests/test-qobject-output-visitor.c
+++ b/tests/test-qobject-output-visitor.c
@@ -145,10 +145,10 @@ static void 
test_visitor_out_enum_errors(TestOutputVisitorData *data,
  const void *unused)
 {
 EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-Error *err;
 
 for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-err = NULL;
+Error *err = NULL;
+
 visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
 error_free_or_abort(&err);
 visitor_reset(data);
@@ -240,11 +240,11 @@ static void 
test_visitor_out_struct_errors(TestOutputVisitorData *data,
 EnumOne bad_values[] = { ENUM_ONE__MAX, -1 };
 UserDefOne u = {0};
 UserDefOne *pu = &u;
-Error *err;
 int i;
 
 for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-err = NULL;
+Error *err = NULL;
+
 u.has_enum1 = true;
 u.enum1 = bad_values[i];
 visit_type_UserDefOne(data->ov, "unused", &pu, &err);
diff --git a/tests/test-string-output-visitor.c 
b/tests/test-string-output-visitor.c
index 02766c0f65..1be1540767 100644
--- a/tests/test-string-output-visitor.c
+++ b/tests/test-string-output-visitor.c
@@ -207,10 +207,10 @@ static void 
test_visitor_out_enum_errors(TestOutputVisitorData *data,
  const void *unused)
 {
 EnumOne i, bad_values[] = { ENUM_ONE__MAX, -1 };
-Error *err;
 
 for (i = 0; i < ARRAY_SIZE(bad_values) ; i++) {
-err = NULL;
+Error *err = NULL;
+
 visit_type_EnumOne(data->ov, "unused", &bad_values[i], &err);
 error_free_or_abort(&err);
 }
-- 
2.21.0

Re: [PATCH 02/10] hw: arm: add Xunlong Orange Pi PC machine

[Philippe Mathieu-Daudé]

On 12/3/19 8:33 PM, Niek Linnenbank wrote:

Hello Philippe,

Thanks for your quick review comments!
I'll start working on a v2 of the patches and include the changes you 
suggested.


Thanks, but I'd suggest to wait few more days to give time to others 
reviewers. Else having multiple versions of a big series reviewed at the 
same time is very confusing.
I have other minor comments on others patches, but need to find the time 
to continue reviewing.

Re: qom device lifecycle interaction with hotplug/hotunplug ?

[Jens Freimann]

On Tue, Dec 03, 2019 at 06:40:04PM -0300, Eduardo Habkost wrote:

+jfreimann, +mst

On Sat, Nov 30, 2019 at 11:10:19AM +, Peter Maydell wrote:

On Fri, 29 Nov 2019 at 20:05, Eduardo Habkost  wrote:
> So, to summarize the current issues:
>
> 1) realize triggers a plug operation implicitly.
> 2) unplug triggers unrealize implicitly.
>
> Do you expect to see use cases that will require us to implement
> realize-without-plug?

I don't think so, but only because of the oddity that
we put lots of devices on the 'sysbus' and claim that
that's plugging them into the bus. The common case of
'realize' is where one device (say an SoC) has a bunch of child
devices (like UARTs); the SoC's realize method realizes its child
devices. Those devices all end up plugged into the 'sysbus'
but there's no actual bus there, it's fictional and about
the only thing it matters for is reset propagation (which
we don't model right either). A few devices don't live on
buses at all.


That's my impression as well.



> Similarly, do you expect use cases that will require us to
> implement unplug-without-unrealize?

I don't know enough about hotplug to answer this one:
it's essentially what I'm hoping you'd be able to answer.
I vaguely had in mind that eg the user might be able to
create a 'disk' object, plug it into a SCSI bus, then
unplug it from the bus without the disk and all its data
evaporating, and maybe plug it back into the SCSI
bus (or some other SCSI bus) later ? But I don't know
anything about how we expose that kind of thing to the
user via QMP/HMP.


This ability isn't exposed to the user at all.  Our existing
interfaces are -device, device_add and device_del.

We do have something new that sounds suspiciously similar to
"unplugged but not unrealized", though: the new hidden device
API, added by commit f3a850565693 ("qdev/qbus: add hidden device
support").

Jens, Michael, what exactly is the difference between a "hidden"
device and a "unplugged" device?


"hidden" the way we use it for virtio-net failover is actually unplugged. But it
doesn't have to be that way. You can register a function that decides
if the device should be hidden, i.e. plugged now, or do something else
with it (in the virtio-net failover case we just save everything we
need to plug the device later).  


We did introduce a "unplugged but not unrealized" function too as part
of the failover feature. See "a99c4da9fc pci: mark devices partially
unplugged"

This was needed so we would be able to re-plug the device in case a
migration failed and we need to hotplug the primary device back to the
guest. To avoid the risk of not getting the resources the device needs
we don't unrealize but just trigger the unplug from the guest OS.

regards
Jens

Re: [PATCH] travis.yml: Drop libcap-dev

[Philippe Mathieu-Daudé]

On 12/4/19 9:01 AM, Greg Kurz wrote:

Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
aren't any users of libcap anymore. No need to install libcap-dev.

Signed-off-by: Greg Kurz 
---

Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
Like with the docker and the gitlab CI patches, if I get an ack from Alex
I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
begins. I'll make sure the SHA1 for Paolo's patch remains the same.

  .travis.yml |1 -
  1 file changed, 1 deletion(-)

diff --git a/.travis.yml b/.travis.yml
index 445b0646c18a..6cb8af6fa599 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -26,7 +26,6 @@ addons:
- libaio-dev
- libattr1-dev
- libbrlapi-dev
-  - libcap-dev
- libcap-ng-dev
- libgcc-4.8-dev
- libgnutls28-dev




Reviewed-by: Philippe Mathieu-Daudé 

[PATCH v2 06/18] hw/acpi: Fix legacy CPU plug error API violations

[Markus Armbruster]

legacy_acpi_cpu_plug_cb() dereferences @errp when
acpi_set_cpu_present_bit() fails.  That's wrong; see the big comment
in error.h.  Introduced in commit cc43364de7 "acpi/cpu-hotplug:
introduce helper function to keep bit setting in one place".

No caller actually passes null, and acpi_set_cpu_present_bit() can't
actually fail.

Fix anyway: drop acpi_set_cpu_present_bit()'s @errp parameter.

Cc: "Michael S. Tsirkin" 
Cc: Igor Mammedov 
Signed-off-by: Markus Armbruster 
Reviewed-by: Igor Mammedov 
---
 hw/acpi/cpu_hotplug.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 3ac2045a95..9c3bcc84de 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -55,8 +55,7 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
 },
 };
 
-static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu,
- Error **errp)
+static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu)
 {
 CPUClass *k = CPU_GET_CLASS(cpu);
 int64_t cpu_id;
@@ -74,10 +73,7 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, 
CPUState *cpu,
 void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
  AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
 {
-acpi_set_cpu_present_bit(g, CPU(dev), errp);
-if (*errp != NULL) {
-return;
-}
+acpi_set_cpu_present_bit(g, CPU(dev));
 acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
 }
 
@@ -92,7 +88,7 @@ void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, 
Object *owner,
 gpe_cpu->device = owner;
 
 CPU_FOREACH(cpu) {
-acpi_set_cpu_present_bit(gpe_cpu, cpu, &error_abort);
+acpi_set_cpu_present_bit(gpe_cpu, cpu);
 }
 }
 
-- 
2.21.0

[PATCH v2 09/18] qga: Fix guest-get-fsinfo error API violations

[Markus Armbruster]

build_guest_fsinfo_for_virtual_device() dereferences @errp when
build_guest_fsinfo_for_device() fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 46d4c5723e "qga: Add
guest-get-fsinfo command".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Michael Roth 
Signed-off-by: Markus Armbruster 
---
 qga/commands-posix.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165dae..0be527ccb8 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -1049,6 +1049,7 @@ static void build_guest_fsinfo_for_virtual_device(char 
const *syspath,
   GuestFilesystemInfo *fs,
   Error **errp)
 {
+Error *err = NULL;
 DIR *dir;
 char *dirpath;
 struct dirent *entry;
@@ -1078,10 +1079,11 @@ static void build_guest_fsinfo_for_virtual_device(char 
const *syspath,
 
 g_debug(" slave device '%s'", entry->d_name);
 path = g_strdup_printf("%s/slaves/%s", syspath, entry->d_name);
-build_guest_fsinfo_for_device(path, fs, errp);
+build_guest_fsinfo_for_device(path, fs, &err);
 g_free(path);
 
-if (*errp) {
+if (err) {
+error_propagate(errp, err);
 break;
 }
 }
-- 
2.21.0

[Bug 1846427] Re: 4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

[Matti Hameister]

I was unable to compile the qemu-git package and I currently have not
time to investigate that. But I updated to 4.1.1. I just started my
Windows 10 VM with that and after a short time of use the image was
corrupted again. Here is my full start parameter set. Maybe there is
something wrong or I should change something?

qemu-system-x86_64 -cpu Haswell-noTSX -M q35 -enable-kvm -smp
4,cores=4,threads=1,sockets=1 -net nic,model=virtio -net
user,hostname=WindowsKVM.local -drive
if=none,id=hd,file=hdd.qcow2,discard=unmap -device virtio-scsi-
pci,id=scsi --enable-kvm -device scsi-hd,drive=hd -m 4096 -drive
if=pflash,format=raw,readonly,file=/usr/share/ovmf/x64/OVMF_CODE.fd
-drive if=pflash,format=raw,file=./OVMF_VARS.fd -drive
file=Windows10ISO/Windows.iso,index=0,media=cdrom -drive file=virtio-
win-0.1.173.iso,index=1,media=cdrom -no-quit

My Linux VM is still fine.

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

Title:
  4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

Status in QEMU:
  Fix Committed

Bug description:
  I'm seeing massive corruption of qcow2 images with qemu 4.1.0 and git
  master as of 7f21573c822805a8e6be379d9bcf3ad9effef3dc after a few
  savevm/quit/loadvm cycles. I've narrowed it down to the following
  reproducer (further notes below):

  # qemu-img check debian.qcow2
  No errors were found on the image.
  251601/327680 = 76.78% allocated, 1.63% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208
  # bin/qemu/bin/qemu-system-x86_64 -machine pc-q35-4.0.1,accel=kvm -m 4096 
-chardev stdio,id=charmonitor -mon chardev=charmonitor -drive 
file=debian.qcow2,id=d -S
  qemu-system-x86_64: warning: dbind: Couldn't register with accessibility bus: 
Did not receive a reply. Possible causes include: the remote application did 
not send a reply, the message bus security policy blocked the reply, the reply 
timeout expired, or the network connection was broken.
  QEMU 4.1.50 monitor - type 'help' for more information
  (qemu) loadvm foo
  (qemu) c
  (qemu) qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  quit
  [m@nargothrond:~] qemu-img check debian.qcow2
  Leaked cluster 85179 refcount=2 reference=1
  Leaked cluster 85180 refcount=2 reference=1
  ERROR cluster 266150 refcount=0 reference=2
  [...]
  ERROR OFLAG_COPIED data cluster: l2_entry=42284 refcount=1

  9493 errors were found on the image.
  Data may be corrupted, or further writes to the image may corrupt it.

  2 leaked clusters were found on the image.
  This means waste of disk space, but no harm to data.
  259266/327680 = 79.12% allocated, 1.67% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208

  This is on a x86_64 Linux 5.3.1 Gentoo host with qemu-system-x86_64
  and accel=kvm. The compiler is gcc-9.2.0 with the rest of the system
  similarly current.

  Reproduced with qemu-4.1.0 from distribution package as well as
  vanilla git checkout of tag v4.1.0 and commit
  7f21573c822805a8e6be379d9bcf3ad9effef3dc (today's master). Does not
  happen with qemu compiled from vanilla checkout of tag v4.0.0. Build
  sequence:

  ./configure --prefix=$HOME/bin/qemu-bisect --target-list=x86_64-softmmu 
--disable-werror --disable-docs
  [...]
  CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g
  [...] (can provide full configure output if helpful)
  make -j8 install

  The kind of guest OS does not matter: seen with Debian testing 64bit,
  Windows 7 x86/x64 BIOS and Windows 7 x64 EFI.

  The virtual storage controller does not seem to matter: seen with
  VirtIO SCSI, emulated SCSI and emulated SATA AHCI.

  Caching modes (none, directsync, writeback), aio mode (threads,
  native) or discard (ignore, unmap) or detect-zeroes (off, unmap) does
  not influence occurence either.

  Having more RAM in the guest seems to increase odds of corruption:
  With 512MB to the Debian guest problem hardly occurs at all, with 4GB
  RAM it happens almost instantly.

  An automated reproducer works as follows:

  - the guest *does* mount its root fs and swap with option discard and
  my testing leaves me with the impression that file deletion rather
  than reading is causing the issue

  - foo is a snapshot of the running Debian VM which is already running
  command

  # while true ; do dd if=/dev/zero of=foo bs=10240k count=400 ; done

  to produce some I/O to the disk (4GB file with 4GB of RAM).

  - on the host a loop continuously resumes and saves the guest state
  and quits qemu inbetween:

  # while true ; do (echo loadvm foo ; echo c ; sleep 10 ; echo stop ;
  echo savevm foo ; echo quit ) | bin/qemu-bisect/bin/qemu-system-x86_64
  -machine pc-q35-3.1,accel=kvm -m 4096 -chardev stdio,id=charmonitor
  -mon chardev=charmonitor -drive file=debian.qcow2,id=d -S -displ

[PATCH v2 01/18] crypto: Fix certificate file error handling crash bug

[Markus Armbruster]

qcrypto_tls_creds_load_cert() passes uninitialized GError *gerr by
reference to g_file_get_contents().  When g_file_get_contents() fails,
it'll try to set a GError.  Unless @gerr is null by dumb luck, this
logs a ERROR_OVERWRITTEN_WARNING warning message and leaves @gerr
unchanged.  qcrypto_tls_creds_load_cert() then dereferences the
uninitialized @gerr.

Fix by initializing @gerr properly.

Fixes: 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03
Cc: "Daniel P. Berrangé" 
Signed-off-by: Markus Armbruster 
---
 crypto/tlscredsx509.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 01fc304e5d..53a4368f49 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -380,7 +380,7 @@ qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
 gnutls_x509_crt_t cert = NULL;
 g_autofree char *buf = NULL;
 gsize buflen;
-GError *gerr;
+GError *gerr = NULL;
 int ret = -1;
 int err;
 
-- 
2.21.0

[PATCH v2 00/18] Error handling fixes

[Markus Armbruster]

v2:
* Old PATCH 01-03 have been merged for 4.2
* PATCH 05-15: Commit message rephrased [David], R-bys kept

Cc: "Daniel P. Berrangé" 
Cc: "Michael S. Tsirkin" 
Cc: Christian Borntraeger 
Cc: Corey Minyard 
Cc: Cornelia Huck 
Cc: David Hildenbrand 
Cc: Halil Pasic 
Cc: Igor Mammedov 
Cc: Michael Roth 

Markus Armbruster (18):
  crypto: Fix certificate file error handling crash bug
  crypto: Fix typo in QCryptoTLSSession's  comment
  io: Fix Error usage in a comment 
  tests: Clean up initialization of Error *err variables
  exec: Fix file_ram_alloc() error API violations
  hw/acpi: Fix legacy CPU plug error API violations
  hw/core: Fix fit_load_fdt() error handling violations
  hw/ipmi: Fix realize() error API violations
  qga: Fix guest-get-fsinfo error API violations
  memory-device: Fix memory pre-plug error API violations
  s390x/event-facility: Fix realize() error API violations
  s390x/cpumodel: Fix feature property error API violations
  s390x/cpumodel: Fix realize() error API violations
  s390x/cpumodel: Fix query-cpu-model-FOO error API violations
  s390x/cpumodel: Fix query-cpu-definitions error API violations
  error: Clean up unusual names of Error * variables
  hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()
  tests-blockjob: Use error_free_or_abort()

 include/crypto/tlssession.h |  2 +-
 include/io/task.h   |  2 +-
 crypto/tlscredsx509.c   |  2 +-
 exec.c  |  6 +-
 hw/acpi/cpu_hotplug.c   | 10 +--
 hw/core/loader-fit.c| 15 ++---
 hw/intc/s390_flic_kvm.c | 16 +++--
 hw/ipmi/isa_ipmi_bt.c   |  7 ++-
 hw/ipmi/isa_ipmi_kcs.c  |  7 ++-
 hw/ipmi/pci_ipmi_bt.c   |  6 +-
 hw/ipmi/pci_ipmi_kcs.c  |  6 +-
 hw/mem/memory-device.c  |  6 +-
 hw/ppc/spapr_pci.c  | 16 ++---
 hw/ppc/spapr_pci_nvlink2.c  | 10 +--
 hw/s390x/event-facility.c   |  6 +-
 qga/commands-posix.c|  6 +-
 target/s390x/cpu_models.c   | 98 +
 tests/test-blockjob.c   | 15 +++--
 tests/test-qobject-output-visitor.c |  8 +--
 tests/test-string-output-visitor.c  |  4 +-
 20 files changed, 139 insertions(+), 109 deletions(-)

-- 
2.21.0

[PATCH v2 02/18] crypto: Fix typo in QCryptoTLSSession's comment

[Markus Armbruster]

Cc: "Daniel P. Berrangé" 
Signed-off-by: Markus Armbruster 
---
 include/crypto/tlssession.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index e01e1a9dc2..15b9cef086 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -56,7 +56,7 @@
  *
  * static int mysock_run_tls(int sockfd,
  *   QCryptoTLSCreds *creds,
- *   Error *errp)
+ *   Error **errp)
  * {
  *QCryptoTLSSession *sess;
  *
-- 
2.21.0

[PATCH v2 17/18] hw/intc/s390: Simplify error handling in kvm_s390_flic_realize()

[Markus Armbruster]

Cc: Halil Pasic 
Cc: Cornelia Huck 
Cc: Christian Borntraeger 
Signed-off-by: Markus Armbruster 
Reviewed-by: Cornelia Huck 
Acked-by: Halil Pasic 
---
 hw/intc/s390_flic_kvm.c | 10 --
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index 30d50c2369..33ea61 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -586,16 +586,17 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 
 KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &err);
 if (err) {
-goto fail;
+error_propagate(errp, err);
+return;
 }
 flic_state->fd = -1;
 
 cd.type = KVM_DEV_TYPE_FLIC;
 ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
 if (ret < 0) {
-error_setg_errno(&err, errno, "Creating the KVM device failed");
+error_setg_errno(errp, errno, "Creating the KVM device failed");
 trace_flic_create_device(errno);
-goto fail;
+return;
 }
 flic_state->fd = cd.fd;
 
@@ -603,9 +604,6 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 test_attr.group = KVM_DEV_FLIC_CLEAR_IO_IRQ;
 flic_state->clear_io_supported = !ioctl(flic_state->fd,
 KVM_HAS_DEVICE_ATTR, test_attr);
-return;
-fail:
-error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
-- 
2.21.0

[PATCH v2 10/18] memory-device: Fix memory pre-plug error API violations

[Markus Armbruster]

memory_device_get_free_addr() dereferences @errp when
memory_device_check_addable() fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 1b6d6af21b "pc-dimm: factor
out capacity and slot checks into MemoryDevice".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 hw/mem/memory-device.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/mem/memory-device.c b/hw/mem/memory-device.c
index aef148c1d7..4bc9cf0917 100644
--- a/hw/mem/memory-device.c
+++ b/hw/mem/memory-device.c
@@ -99,6 +99,7 @@ static uint64_t memory_device_get_free_addr(MachineState *ms,
 uint64_t align, uint64_t size,
 Error **errp)
 {
+Error *err = NULL;
 GSList *list = NULL, *item;
 Range as, new = range_empty;
 
@@ -123,8 +124,9 @@ static uint64_t memory_device_get_free_addr(MachineState 
*ms,
 return 0;
 }
 
-memory_device_check_addable(ms, size, errp);
-if (*errp) {
+memory_device_check_addable(ms, size, &err);
+if (err) {
+error_propagate(errp, err);
 return 0;
 }
 
-- 
2.21.0

[PATCH v2 13/18] s390x/cpumodel: Fix realize() error API violations

[Markus Armbruster]

get_max_cpu_model() dereferences @errp when
kvm_s390_get_host_cpu_model() fails, apply_cpu_model() dereferences it
when kvm_s390_apply_cpu_model() fails, and s390_realize_cpu_model()
dereferences it when get_max_cpu_model() or check_compatibility()
fail.  That's wrong; see the big comment in error.h.  All three
introduced in commit 80560137cf "s390x/cpumodel: check and apply the
CPU model".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 37 ++---
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 6a29fd3ab1..c702e34a26 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -870,6 +870,7 @@ static void check_compatibility(const S390CPUModel 
*max_model,
 
 static S390CPUModel *get_max_cpu_model(Error **errp)
 {
+Error *err = NULL;
 static S390CPUModel max_model;
 static bool cached;
 
@@ -878,22 +879,24 @@ static S390CPUModel *get_max_cpu_model(Error **errp)
 }
 
 if (kvm_enabled()) {
-kvm_s390_get_host_cpu_model(&max_model, errp);
+kvm_s390_get_host_cpu_model(&max_model, &err);
 } else {
 max_model.def = s390_find_cpu_def(QEMU_MAX_CPU_TYPE, QEMU_MAX_CPU_GEN,
   QEMU_MAX_CPU_EC_GA, NULL);
 bitmap_copy(max_model.features, qemu_max_cpu_feat, S390_FEAT_MAX);
-   }
-if (!*errp) {
-cached = true;
-return &max_model;
 }
-return NULL;
+if (err) {
+error_propagate(errp, err);
+return NULL;
+}
+cached = true;
+return &max_model;
 }
 
 static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 {
 #ifndef CONFIG_USER_ONLY
+Error *err = NULL;
 static S390CPUModel applied_model;
 static bool applied;
 
@@ -909,20 +912,23 @@ static inline void apply_cpu_model(const S390CPUModel 
*model, Error **errp)
 }
 
 if (kvm_enabled()) {
-kvm_s390_apply_cpu_model(model, errp);
+kvm_s390_apply_cpu_model(model, &err);
+if (err) {
+error_propagate(errp, err);
+return;
+}
 }
 
-if (!*errp) {
-applied = true;
-if (model) {
-applied_model = *model;
-}
+applied = true;
+if (model) {
+applied_model = *model;
 }
 #endif
 }
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+Error *err = NULL;
 S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
 S390CPU *cpu = S390_CPU(cs);
 const S390CPUModel *max_model;
@@ -939,7 +945,7 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 }
 
 max_model = get_max_cpu_model(errp);
-if (*errp) {
+if (!max_model) {
 error_prepend(errp, "CPU models are not available: ");
 return;
 }
@@ -951,8 +957,9 @@ void s390_realize_cpu_model(CPUState *cs, Error **errp)
 cpu->model->cpu_ver = max_model->cpu_ver;
 
 check_consistency(cpu->model);
-check_compatibility(max_model, cpu->model, errp);
-if (*errp) {
+check_compatibility(max_model, cpu->model, &err);
+if (err) {
+error_propagate(errp, err);
 return;
 }
 
-- 
2.21.0

[PATCH v2 08/18] hw/ipmi: Fix realize() error API violations

[Markus Armbruster]

isa_ipmi_bt_realize(), ipmi_isa_realize(), pci_ipmi_bt_realize(), and
pci_ipmi_kcs_realize() dereference @errp when IPMIInterfaceClass
method init() fails.  That's wrong; see the big comment in error.h.
Introduced in commit 0719029c47 "ipmi: Add an ISA KCS low-level
interface", then imitated in commit a9b74079cb "ipmi: Add a BT
low-level interface" and commit 12f983c6aa "ipmi: Add PCI IPMI
interfaces".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Corey Minyard 
Signed-off-by: Markus Armbruster 
---
 hw/ipmi/isa_ipmi_bt.c  | 7 +--
 hw/ipmi/isa_ipmi_kcs.c | 7 +--
 hw/ipmi/pci_ipmi_bt.c  | 6 --
 hw/ipmi/pci_ipmi_kcs.c | 6 --
 4 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/ipmi/isa_ipmi_bt.c b/hw/ipmi/isa_ipmi_bt.c
index 9a87ffd3f0..9fba5ed383 100644
--- a/hw/ipmi/isa_ipmi_bt.c
+++ b/hw/ipmi/isa_ipmi_bt.c
@@ -70,6 +70,7 @@ static void isa_ipmi_bt_lower_irq(IPMIBT *ib)
 
 static void isa_ipmi_bt_realize(DeviceState *dev, Error **errp)
 {
+Error *err = NULL;
 ISADevice *isadev = ISA_DEVICE(dev);
 ISAIPMIBTDevice *iib = ISA_IPMI_BT(dev);
 IPMIInterface *ii = IPMI_INTERFACE(dev);
@@ -85,9 +86,11 @@ static void isa_ipmi_bt_realize(DeviceState *dev, Error 
**errp)
 iib->bt.bmc->intf = ii;
 iib->bt.opaque = iib;
 
-iic->init(ii, 0, errp);
-if (*errp)
+iic->init(ii, 0, &err);
+if (err) {
+error_propagate(errp, err);
 return;
+}
 
 if (iib->isairq > 0) {
 isa_init_irq(isadev, &iib->irq, iib->isairq);
diff --git a/hw/ipmi/isa_ipmi_kcs.c b/hw/ipmi/isa_ipmi_kcs.c
index ca3ea36a3f..cc6bd817f2 100644
--- a/hw/ipmi/isa_ipmi_kcs.c
+++ b/hw/ipmi/isa_ipmi_kcs.c
@@ -69,6 +69,7 @@ static void isa_ipmi_kcs_lower_irq(IPMIKCS *ik)
 
 static void ipmi_isa_realize(DeviceState *dev, Error **errp)
 {
+Error *err = NULL;
 ISADevice *isadev = ISA_DEVICE(dev);
 ISAIPMIKCSDevice *iik = ISA_IPMI_KCS(dev);
 IPMIInterface *ii = IPMI_INTERFACE(dev);
@@ -84,9 +85,11 @@ static void ipmi_isa_realize(DeviceState *dev, Error **errp)
 iik->kcs.bmc->intf = ii;
 iik->kcs.opaque = iik;
 
-iic->init(ii, 0, errp);
-if (*errp)
+iic->init(ii, 0, &err);
+if (err) {
+error_propagate(errp, err);
 return;
+}
 
 if (iik->isairq > 0) {
 isa_init_irq(isadev, &iik->irq, iik->isairq);
diff --git a/hw/ipmi/pci_ipmi_bt.c b/hw/ipmi/pci_ipmi_bt.c
index 6ed925a665..ba9cf016b5 100644
--- a/hw/ipmi/pci_ipmi_bt.c
+++ b/hw/ipmi/pci_ipmi_bt.c
@@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIBT *ik)
 
 static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
 {
+Error *err = NULL;
 PCIIPMIBTDevice *pik = PCI_IPMI_BT(pd);
 IPMIInterface *ii = IPMI_INTERFACE(pd);
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
@@ -74,8 +75,9 @@ static void pci_ipmi_bt_realize(PCIDevice *pd, Error **errp)
 pik->bt.raise_irq = pci_ipmi_raise_irq;
 pik->bt.lower_irq = pci_ipmi_lower_irq;
 
-iic->init(ii, 8, errp);
-if (*errp) {
+iic->init(ii, 8, &err);
+if (err) {
+error_propagate(errp, err);
 return;
 }
 pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->bt.io);
diff --git a/hw/ipmi/pci_ipmi_kcs.c b/hw/ipmi/pci_ipmi_kcs.c
index eeba63baa4..99f46152f4 100644
--- a/hw/ipmi/pci_ipmi_kcs.c
+++ b/hw/ipmi/pci_ipmi_kcs.c
@@ -54,6 +54,7 @@ static void pci_ipmi_lower_irq(IPMIKCS *ik)
 
 static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
 {
+Error *err = NULL;
 PCIIPMIKCSDevice *pik = PCI_IPMI_KCS(pd);
 IPMIInterface *ii = IPMI_INTERFACE(pd);
 IPMIInterfaceClass *iic = IPMI_INTERFACE_GET_CLASS(ii);
@@ -74,8 +75,9 @@ static void pci_ipmi_kcs_realize(PCIDevice *pd, Error **errp)
 pik->kcs.raise_irq = pci_ipmi_raise_irq;
 pik->kcs.lower_irq = pci_ipmi_lower_irq;
 
-iic->init(ii, 8, errp);
-if (*errp) {
+iic->init(ii, 8, &err);
+if (err) {
+error_propagate(errp, err);
 return;
 }
 pci_register_bar(pd, 0, PCI_BASE_ADDRESS_SPACE_IO, &pik->kcs.io);
-- 
2.21.0

[PATCH v2 12/18] s390x/cpumodel: Fix feature property error API violations

[Markus Armbruster]

s390x-cpu property setters set_feature() and set_feature_group()
dereference @errp when the visitor fails.  That's wrong; see the big
comment in error.h.  Introduced in commit 0754f60429 "s390x/cpumodel:
expose features and feature groups as properties".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 7e92fb2e15..6a29fd3ab1 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -987,6 +987,7 @@ static void get_feature(Object *obj, Visitor *v, const char 
*name,
 static void set_feature(Object *obj, Visitor *v, const char *name,
 void *opaque, Error **errp)
 {
+Error *err = NULL;
 S390Feat feat = (S390Feat) opaque;
 DeviceState *dev = DEVICE(obj);
 S390CPU *cpu = S390_CPU(obj);
@@ -1002,8 +1003,9 @@ static void set_feature(Object *obj, Visitor *v, const 
char *name,
 return;
 }
 
-visit_type_bool(v, name, &value, errp);
-if (*errp) {
+visit_type_bool(v, name, &value, &err);
+if (err) {
+error_propagate(errp, err);
 return;
 }
 if (value) {
@@ -1043,6 +1045,7 @@ static void get_feature_group(Object *obj, Visitor *v, 
const char *name,
 static void set_feature_group(Object *obj, Visitor *v, const char *name,
   void *opaque, Error **errp)
 {
+Error *err = NULL;
 S390FeatGroup group = (S390FeatGroup) opaque;
 const S390FeatGroupDef *def = s390_feat_group_def(group);
 DeviceState *dev = DEVICE(obj);
@@ -1059,8 +1062,9 @@ static void set_feature_group(Object *obj, Visitor *v, 
const char *name,
 return;
 }
 
-visit_type_bool(v, name, &value, errp);
-if (*errp) {
+visit_type_bool(v, name, &value, &err);
+if (err) {
+error_propagate(errp, err);
 return;
 }
 if (value) {
-- 
2.21.0

[PATCH v2 07/18] hw/core: Fix fit_load_fdt() error handling violations

[Markus Armbruster]

fit_load_fdt() passes @errp to fit_image_addr(), then recovers from
ENOENT failures.  Passing @errp is wrong, because it works only as
long as @errp is neither @error_fatal nor @error_abort.  Messed up in
commit 3eb99edb48 "loader-fit: Wean off error_printf()".

No caller actually passes such values.

Fix anyway: splice in a local Error *err, and error_propagate().

Signed-off-by: Markus Armbruster 
---
 hw/core/loader-fit.c | 15 ---
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..c465921b8f 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, 
const void *itb,
 int cfg, void *opaque, const void *match_data,
 hwaddr kernel_end, Error **errp)
 {
+Error *err = NULL;
 const char *name;
 const void *data;
 const void *load_data;
 hwaddr load_addr;
-int img_off, err;
+int img_off;
 size_t sz;
 int ret;
 
@@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, 
const void *itb,
 return -EINVAL;
 }
 
-err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
-if (err == -ENOENT) {
+ret = fit_image_addr(itb, img_off, "load", &load_addr, &err);
+if (ret == -ENOENT) {
 load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
-error_free(*errp);
-} else if (err) {
-error_prepend(errp, "unable to read FDT load address from FIT: ");
-ret = err;
+error_free(err);
+} else if (ret) {
+error_propagate_prepend(errp, err,
+"unable to read FDT load address from FIT: ");
 goto out;
 }
 
-- 
2.21.0

[PATCH v2 15/18] s390x/cpumodel: Fix query-cpu-definitions error API violations

[Markus Armbruster]

qmp_query_cpu_definitions() passes @errp to get_max_cpu_model(), then
frees any error it gets back.  This effectively ignores errors.
Dereferencing @errp is wrong; see the big comment in error.h.  Passing
@errp is also wrong, because it works only as long as @errp is neither
@error_fatal nor @error_abort.  Introduced in commit 38cba1f4d8
"s390x: return unavailable features via query-cpu-definitions".

No caller actually passes such @errp values.

Fix anyway: simply pass NULL to get_max_cpu_model().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 3ed301b5e5..547bab8ac3 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -462,11 +462,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
**errp)
 .list = NULL,
 };
 
-list_data.model = get_max_cpu_model(errp);
-if (*errp) {
-error_free(*errp);
-*errp = NULL;
-}
+list_data.model = get_max_cpu_model(NULL);
 
 object_class_foreach(create_cpu_model_list, TYPE_S390_CPU, false,
  &list_data);
-- 
2.21.0

[PATCH v2 18/18] tests-blockjob: Use error_free_or_abort()

[Markus Armbruster]

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
---
 tests/test-blockjob.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index e670a20617..4eeb184caf 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -48,9 +48,8 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
 g_assert_cmpstr(job->job.id, ==, blk_name(blk));
 }
 } else {
-g_assert_nonnull(err);
+error_free_or_abort(&err);
 g_assert_null(job);
-error_free(err);
 }
 
 return job;
-- 
2.21.0

Re: [PATCH v2 00/18] Error handling fixes

[David Hildenbrand]

On 04.12.19 10:36, Markus Armbruster wrote:
> v2:
> * Old PATCH 01-03 have been merged for 4.2
> * PATCH 05-15: Commit message rephrased [David], R-bys kept

Thanks!


-- 
Thanks,

David / dhildenb

[PATCH v2 03/18] io: Fix Error usage in a comment

[Markus Armbruster]

Cc: "Daniel P. Berrangé" 
Signed-off-by: Markus Armbruster 
---
 include/io/task.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/io/task.h b/include/io/task.h
index 5cb9faf9f2..1abbfb8b65 100644
--- a/include/io/task.h
+++ b/include/io/task.h
@@ -119,7 +119,7 @@ typedef void (*QIOTaskWorker)(QIOTask *task,
  *   gboolean myobject_operation_timer(gpointer opaque)
  *   {
  *  QIOTask *task = QIO_TASK(opaque);
- *  Error *err;*
+ *  Error *err = NULL;
  *
  *  ...check something important...
  *   if (err) {
-- 
2.21.0

[PATCH v2 14/18] s390x/cpumodel: Fix query-cpu-model-FOO error API violations

[Markus Armbruster]

cpu_model_from_info() is a helper for qmp_query_cpu_model_expansion(),
qmp_query_cpu_model_comparison(), qmp_query_cpu_model_baseline().  It
dereferences @errp when the visitor or the QOM setter fails.  That's
wrong; see the big comment in error.h.  Introduced in commit
137974cea3 's390x/cpumodel: implement QMP interface
"query-cpu-model-expansion"'.

Its three callers have the same issue.  Introduced in commit
4e82ef0502 's390x/cpumodel: implement QMP interface
"query-cpu-model-comparison"' and commit f1a47d08ef 's390x/cpumodel:
implement QMP interface "query-cpu-model-baseline"'.

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 target/s390x/cpu_models.c | 43 ---
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index c702e34a26..3ed301b5e5 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -477,6 +477,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error 
**errp)
 static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
 Error **errp)
 {
+Error *err = NULL;
 const QDict *qdict = NULL;
 const QDictEntry *e;
 Visitor *visitor;
@@ -513,24 +514,26 @@ static void cpu_model_from_info(S390CPUModel *model, 
const CpuModelInfo *info,
 
 if (qdict) {
 visitor = qobject_input_visitor_new(info->props);
-visit_start_struct(visitor, NULL, NULL, 0, errp);
-if (*errp) {
+visit_start_struct(visitor, NULL, NULL, 0, &err);
+if (err) {
+error_propagate(errp, err);
 visit_free(visitor);
 object_unref(obj);
 return;
 }
 for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
-object_property_set(obj, visitor, e->key, errp);
-if (*errp) {
+object_property_set(obj, visitor, e->key, &err);
+if (err) {
 break;
 }
 }
-if (!*errp) {
+if (!err) {
 visit_check_struct(visitor, errp);
 }
 visit_end_struct(visitor, NULL);
 visit_free(visitor);
-if (*errp) {
+if (err) {
+error_propagate(errp, err);
 object_unref(obj);
 return;
 }
@@ -595,13 +598,15 @@ CpuModelExpansionInfo 
*qmp_query_cpu_model_expansion(CpuModelExpansionType type,
   CpuModelInfo *model,
   Error **errp)
 {
+Error *err = NULL;
 CpuModelExpansionInfo *expansion_info = NULL;
 S390CPUModel s390_model;
 bool delta_changes = false;
 
 /* convert it to our internal representation */
-cpu_model_from_info(&s390_model, model, errp);
-if (*errp) {
+cpu_model_from_info(&s390_model, model, &err);
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 
@@ -634,18 +639,21 @@ CpuModelCompareInfo 
*qmp_query_cpu_model_comparison(CpuModelInfo *infoa,
  CpuModelInfo *infob,
  Error **errp)
 {
+Error *err = NULL;
 CpuModelCompareResult feat_result, gen_result;
 CpuModelCompareInfo *compare_info;
 S390FeatBitmap missing, added;
 S390CPUModel modela, modelb;
 
 /* convert both models to our internal representation */
-cpu_model_from_info(&modela, infoa, errp);
-if (*errp) {
+cpu_model_from_info(&modela, infoa, &err);
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
-cpu_model_from_info(&modelb, infob, errp);
-if (*errp) {
+cpu_model_from_info(&modelb, infob, &err);
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 compare_info = g_new0(CpuModelCompareInfo, 1);
@@ -707,6 +715,7 @@ CpuModelBaselineInfo 
*qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 CpuModelInfo *infob,
 Error **errp)
 {
+Error *err = NULL;
 CpuModelBaselineInfo *baseline_info;
 S390CPUModel modela, modelb, model;
 uint16_t cpu_type;
@@ -714,13 +723,15 @@ CpuModelBaselineInfo 
*qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
 uint8_t max_gen;
 
 /* convert both models to our internal representation */
-cpu_model_from_info(&modela, infoa, errp);
-if (*errp) {
+cpu_model_from_info(&modela, infoa, &err);
+if (err) {
+error_propagate(errp, err);
 return NULL;
 }
 
-cpu_model_from_info(&modelb, infob, errp);
-if (*errp) {
+cpu_model_from_info(&modelb, infob, &err);
+if (err) {
+error_propagate(errp

Re: [PATCH v2 01/18] crypto: Fix certificate file error handling crash bug

[Daniel P . Berrangé]

On Wed, Dec 04, 2019 at 10:36:08AM +0100, Markus Armbruster wrote:
> qcrypto_tls_creds_load_cert() passes uninitialized GError *gerr by
> reference to g_file_get_contents().  When g_file_get_contents() fails,
> it'll try to set a GError.  Unless @gerr is null by dumb luck, this
> logs a ERROR_OVERWRITTEN_WARNING warning message and leaves @gerr
> unchanged.  qcrypto_tls_creds_load_cert() then dereferences the
> uninitialized @gerr.
> 
> Fix by initializing @gerr properly.
> 
> Fixes: 9a2fd4347c40321f5cbb4ab4220e759fcbf87d03
> Cc: "Daniel P. Berrangé" 
> Signed-off-by: Markus Armbruster 
> ---
>  crypto/tlscredsx509.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-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 :|

[PATCH v2 11/18] s390x/event-facility: Fix realize() error API violations

[Markus Armbruster]

sclp_events_bus_realize() dereferences @errp when
object_property_set_bool() fails.  That's wrong; see the big comment
in error.h.  Introduced in commit f6102c329c "s390/sclp: rework sclp
event facility initialization + device realization".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: David Hildenbrand 
Cc: Cornelia Huck 
Signed-off-by: Markus Armbruster 
Reviewed-by: David Hildenbrand 
---
 hw/s390x/event-facility.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 66205697ae..cdcf9154c4 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -339,14 +339,16 @@ out:
 
 static void sclp_events_bus_realize(BusState *bus, Error **errp)
 {
+Error *err = NULL;
 BusChild *kid;
 
 /* TODO: recursive realization has to be done in common code */
 QTAILQ_FOREACH(kid, &bus->children, sibling) {
 DeviceState *dev = kid->child;
 
-object_property_set_bool(OBJECT(dev), true, "realized", errp);
-if (*errp) {
+object_property_set_bool(OBJECT(dev), true, "realized", &err);
+if (errp) {
+error_propagate(errp, err);
 return;
 }
 }
-- 
2.21.0

Re: [PATCH v2 1/3] virtio: add ability to delete vq through a pointer

[Laurent Vivier]

On 04/12/2019 08:31, pannengy...@huawei.com wrote:
> From: Pan Nengyuan 
> 
> Devices tend to maintain vq pointers, allow deleting them trough a vq pointer.
> 
> Signed-off-by: Michael S. Tsirkin 
> Signed-off-by: Pan Nengyuan 
> ---
> Changes v2 to v1:
> - add a new function virtio_delete_queue to cleanup vq through a vq pointer
> ---
>  hw/virtio/virtio.c | 16 +++-
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 04716b5..6de3cfd 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -2330,17 +2330,23 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int 
> queue_size,
>  return &vdev->vq[i];
>  }
>  
> +void virtio_delete_queue(VirtQueue *vq)
> +{
> +vq->vring.num = 0;
> +vq->vring.num_default = 0;
> +vq->handle_output = NULL;
> +vq->handle_aio_output = NULL;
> +g_free(vq->used_elems);
> +vq->used_elems = NULL;
> +}
> +
>  void virtio_del_queue(VirtIODevice *vdev, int n)
>  {
>  if (n < 0 || n >= VIRTIO_QUEUE_MAX) {
>  abort();
>  }
>  
> -vdev->vq[n].vring.num = 0;
> -vdev->vq[n].vring.num_default = 0;
> -vdev->vq[n].handle_output = NULL;
> -vdev->vq[n].handle_aio_output = NULL;
> -g_free(vdev->vq[n].used_elems);
> +virtio_delete_queue(&vdev->vq[n]);
>  }
>  
>  static void virtio_set_isr(VirtIODevice *vdev, int value)
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index c32a815..e18756d 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -183,6 +183,8 @@ VirtQueue *virtio_add_queue(VirtIODevice *vdev, int 
> queue_size,
>  
>  void virtio_del_queue(VirtIODevice *vdev, int n);
>  
> +void virtio_delete_queue(VirtQueue *vq);
> +
>  void virtqueue_push(VirtQueue *vq, const VirtQueueElement *elem,
>  unsigned int len);
>  void virtqueue_flush(VirtQueue *vq, unsigned int count);
> 

Reviewed-by: Laurent Vivier 

[PATCH] i386: pass CLZERO to guests with EPYC CPU model on AMD ZEN platform

[Ani Sinha]

CLZERO CPUID should be passed on to the guests that use EPYC or EPYC-IBPB CPU
model when the AMD ZEN based host supports it. This change makes it recognize
this CPUID for guests which use EPYC or EPYC-IBPB CPU model.

Signed-off-by: Ani Sinha 
---
 target/i386/cpu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 69f518a..55f0691 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3813,6 +3813,8 @@ static X86CPUDefinition builtin_x86_defs[] = {
 CPUID_EXT3_MISALIGNSSE | CPUID_EXT3_SSE4A | CPUID_EXT3_ABM |
 CPUID_EXT3_CR8LEG | CPUID_EXT3_SVM | CPUID_EXT3_LAHF_LM |
 CPUID_EXT3_TOPOEXT,
+.features[FEAT_8000_0008_EBX] =
+CPUID_8000_0008_EBX_CLZERO,
 .features[FEAT_7_0_EBX] =
 CPUID_7_0_EBX_FSGSBASE | CPUID_7_0_EBX_BMI1 | CPUID_7_0_EBX_AVX2 |
 CPUID_7_0_EBX_SMEP | CPUID_7_0_EBX_BMI2 | CPUID_7_0_EBX_RDSEED |
-- 
1.9.4

Re: [PATCH v2 02/18] crypto: Fix typo in QCryptoTLSSession's comment

[Daniel P . Berrangé]

On Wed, Dec 04, 2019 at 10:36:09AM +0100, Markus Armbruster wrote:
> Cc: "Daniel P. Berrangé" 
> Signed-off-by: Markus Armbruster 
> ---
>  include/crypto/tlssession.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-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 :|

[Bug 1855072] [NEW] ARM: HCR.TVM traps are not implemented

[Julien Freche]

Public bug reported:

On AARCH64, setting HCR.TVM to 1 is supposed to trap all writes to
CTLR_EL1, TTBR0_EL1, TTBR1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, AFSR0_EL1,
AFSR1_EL1, MAIR_EL1, AMAIR_EL1, and CONTEXTIDR_EL1. However, it
currently has no effect (QEMU emulator version 4.1.1).

It is also likely that TRVM will not trap, but, I didn't verify this.

** 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/1855072

Title:
  ARM: HCR.TVM traps are not implemented

Status in QEMU:
  New

Bug description:
  On AARCH64, setting HCR.TVM to 1 is supposed to trap all writes to
  CTLR_EL1, TTBR0_EL1, TTBR1_EL1, TCR_EL1, ESR_EL1, FAR_EL1, AFSR0_EL1,
  AFSR1_EL1, MAIR_EL1, AMAIR_EL1, and CONTEXTIDR_EL1. However, it
  currently has no effect (QEMU emulator version 4.1.1).

  It is also likely that TRVM will not trap, but, I didn't verify this.

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

[PATCH v2 16/18] error: Clean up unusual names of Error * variables

[Markus Armbruster]

Local Error * variables are conventionally named @err or @local_err,
and Error ** parameters @errp.  Naming local variables like parameters
is confusing.  Clean that up.

Naming parameters like local variables is also confusing.  Left for
another day.

Signed-off-by: Markus Armbruster 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/intc/s390_flic_kvm.c| 10 +-
 hw/ppc/spapr_pci.c | 16 
 hw/ppc/spapr_pci_nvlink2.c | 10 +-
 tests/test-blockjob.c  | 16 
 4 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/hw/intc/s390_flic_kvm.c b/hw/intc/s390_flic_kvm.c
index c9ee80eaae..30d50c2369 100644
--- a/hw/intc/s390_flic_kvm.c
+++ b/hw/intc/s390_flic_kvm.c
@@ -582,10 +582,10 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 struct kvm_create_device cd = {0};
 struct kvm_device_attr test_attr = {0};
 int ret;
-Error *errp_local = NULL;
+Error *err = NULL;
 
-KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &errp_local);
-if (errp_local) {
+KVM_S390_FLIC_GET_CLASS(dev)->parent_realize(dev, &err);
+if (err) {
 goto fail;
 }
 flic_state->fd = -1;
@@ -593,7 +593,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 cd.type = KVM_DEV_TYPE_FLIC;
 ret = kvm_vm_ioctl(kvm_state, KVM_CREATE_DEVICE, &cd);
 if (ret < 0) {
-error_setg_errno(&errp_local, errno, "Creating the KVM device failed");
+error_setg_errno(&err, errno, "Creating the KVM device failed");
 trace_flic_create_device(errno);
 goto fail;
 }
@@ -605,7 +605,7 @@ static void kvm_s390_flic_realize(DeviceState *dev, Error 
**errp)
 KVM_HAS_DEVICE_ATTR, test_attr);
 return;
 fail:
-error_propagate(errp, errp_local);
+error_propagate(errp, err);
 }
 
 static void kvm_s390_flic_reset(DeviceState *dev)
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index f6fbcf99ed..723373de73 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -2042,13 +2042,13 @@ void spapr_phb_dma_reset(SpaprPhbState *sphb)
 static void spapr_phb_reset(DeviceState *qdev)
 {
 SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(qdev);
-Error *errp = NULL;
+Error *err = NULL;
 
 spapr_phb_dma_reset(sphb);
 spapr_phb_nvgpu_free(sphb);
-spapr_phb_nvgpu_setup(sphb, &errp);
-if (errp) {
-error_report_err(errp);
+spapr_phb_nvgpu_setup(sphb, &err);
+if (err) {
+error_report_err(err);
 }
 
 /* Reset the IOMMU state */
@@ -2326,7 +2326,7 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState 
*phb,
 cpu_to_be32(phb->numa_node)};
 SpaprTceTable *tcet;
 SpaprDrc *drc;
-Error *errp = NULL;
+Error *err = NULL;
 
 /* Start populating the FDT */
 _FDT(bus_off = fdt_add_subnode(fdt, 0, phb->dtbusname));
@@ -2408,9 +2408,9 @@ int spapr_dt_phb(SpaprMachineState *spapr, SpaprPhbState 
*phb,
 return ret;
 }
 
-spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &errp);
-if (errp) {
-error_report_err(errp);
+spapr_phb_nvgpu_populate_dt(phb, fdt, bus_off, &err);
+if (err) {
+error_report_err(err);
 }
 spapr_phb_nvgpu_ram_populate_dt(phb, fdt);
 
diff --git a/hw/ppc/spapr_pci_nvlink2.c b/hw/ppc/spapr_pci_nvlink2.c
index 4aa89ede23..8332d5694e 100644
--- a/hw/ppc/spapr_pci_nvlink2.c
+++ b/hw/ppc/spapr_pci_nvlink2.c
@@ -57,7 +57,7 @@ struct SpaprPhbPciNvGpuConfig {
 uint64_t nv2_atsd_current;
 int num; /* number of non empty (i.e. tgt!=0) entries in slots[] */
 SpaprPhbPciNvGpuSlot slots[NVGPU_MAX_NUM];
-Error *errp;
+Error *err;
 };
 
 static SpaprPhbPciNvGpuSlot *
@@ -153,7 +153,7 @@ static void spapr_phb_pci_collect_nvgpu(PCIBus *bus, 
PCIDevice *pdev,
 spapr_pci_collect_nvnpu(nvgpus, pdev, tgt, MEMORY_REGION(mr_npu),
 &local_err);
 }
-error_propagate(&nvgpus->errp, local_err);
+error_propagate(&nvgpus->err, local_err);
 }
 if ((pci_default_read_config(pdev, PCI_HEADER_TYPE, 1) !=
  PCI_HEADER_TYPE_BRIDGE)) {
@@ -187,9 +187,9 @@ void spapr_phb_nvgpu_setup(SpaprPhbState *sphb, Error 
**errp)
 pci_for_each_device(bus, pci_bus_num(bus),
 spapr_phb_pci_collect_nvgpu, sphb->nvgpus);
 
-if (sphb->nvgpus->errp) {
-error_propagate(errp, sphb->nvgpus->errp);
-sphb->nvgpus->errp = NULL;
+if (sphb->nvgpus->err) {
+error_propagate(errp, sphb->nvgpus->err);
+sphb->nvgpus->err = NULL;
 goto cleanup_exit;
 }
 
diff --git a/tests/test-blockjob.c b/tests/test-blockjob.c
index 7844c9ffcb..e670a20617 100644
--- a/tests/test-blockjob.c
+++ b/tests/test-blockjob.c
@@ -34,13 +34,13 @@ static BlockJob *mk_job(BlockBackend *blk, const char *id,
 int flags)
 {
 BlockJob *job;
-Error *errp = NULL;
+ 

Re: [PATCH v2 03/18] io: Fix Error usage in a comment

[Daniel P . Berrangé]

On Wed, Dec 04, 2019 at 10:36:10AM +0100, Markus Armbruster wrote:
> Cc: "Daniel P. Berrangé" 
> Signed-off-by: Markus Armbruster 
> ---
>  include/io/task.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-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: [PATCH v2 2/3] virtio-balloon: fix memory leak while attach virtio-balloon device

[Laurent Vivier]

On 04/12/2019 08:31, pannengy...@huawei.com wrote:
> From: Pan Nengyuan 
> 
> ivq/dvq/svq/free_page_vq is forgot to cleanup in
> virtio_balloon_device_unrealize, the memory leak stack is as follow:
> 
> Direct leak of 14336 byte(s) in 2 object(s) allocated from:
> #0 0x7f99fd9d8560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7f99fcb20015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x557d90638437 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x557d9064401d in virtio_balloon_device_realize 
> hw/virtio/virtio-balloon.c:793
> #4 0x557d906356f7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x557d9073f081 in device_set_realized hw/core/qdev.c:876
> #6 0x557d908b1f4d in property_set_bool qom/object.c:2080
> #7 0x557d908b655e in object_property_set_qobject qom/qom-qobject.c:26
> 
> Reported-by: Euler Robot 
> Signed-off-by: Pan Nengyuan 
> ---
> Change v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/virtio/virtio-balloon.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 40b04f5..57f3b9f 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -831,6 +831,13 @@ static void virtio_balloon_device_unrealize(DeviceState 
> *dev, Error **errp)
>  }
>  balloon_stats_destroy_timer(s);
>  qemu_remove_balloon_handler(s);
> +
> +virtio_delete_queue(s->ivq);
> +virtio_delete_queue(s->dvq);
> +virtio_delete_queue(s->svq);
> +if (s->free_page_vq) {
> +virtio_delete_queue(s->free_page_vq);
> +}
>  virtio_cleanup(vdev);
>  }
>  
> 

Reviewed-by: Laurent Vivier 

[PATCH v2 05/18] exec: Fix file_ram_alloc() error API violations

[Markus Armbruster]

When os_mem_prealloc() fails, file_ram_alloc() calls qemu_ram_munmap()
and returns null.  Except it doesn't when its @errp argument is null,
because it checks for failure with (errp && *errp).  Introduced in
commit 056b68af77 "fix qemu exit on memory hotplug when allocation
fails at prealloc time".

No caller actually passes null.

Fix anyway: splice in a local Error *err, and error_propagate().

Cc: Igor Mammedov 
Signed-off-by: Markus Armbruster 
Reviewed-by: Igor Mammedov 
---
 exec.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/exec.c b/exec.c
index ffdb518535..45695a5f2d 100644
--- a/exec.c
+++ b/exec.c
@@ -1841,6 +1841,7 @@ static void *file_ram_alloc(RAMBlock *block,
 bool truncate,
 Error **errp)
 {
+Error *err = NULL;
 MachineState *ms = MACHINE(qdev_get_machine());
 void *area;
 
@@ -1898,8 +1899,9 @@ static void *file_ram_alloc(RAMBlock *block,
 }
 
 if (mem_prealloc) {
-os_mem_prealloc(fd, area, memory, ms->smp.cpus, errp);
-if (errp && *errp) {
+os_mem_prealloc(fd, area, memory, ms->smp.cpus, &err);
+if (err) {
+error_propagate(errp, err);
 qemu_ram_munmap(fd, area, memory);
 return NULL;
 }
-- 
2.21.0

[Bug 1846427] Re: 4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

[Kevin Wolf]

I don't see anything suspicious in that command line. My only idea for a
different configuration to test would be discard=off, which would remove
a few code paths that could contain a bug.

Anyway, I think it's pretty clear now that you're hitting a different
bug than Michael. Maybe it would be better to create a new report, too,
and to continue there.

Did you upgrade from 4.0 to 4.1 when you first hit the bug or was it
from an earlier version?

It would be perfect if you could bisect the problem like Michael did
with his, but I understand that this might not be possible soon.
Alternatively, I could also debug it myself if I had a clear reproducer
(that ideally doesn't involve Windows).

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

Title:
  4.1.0: qcow2 corruption on savevm/quit/loadvm cycle

Status in QEMU:
  Fix Committed

Bug description:
  I'm seeing massive corruption of qcow2 images with qemu 4.1.0 and git
  master as of 7f21573c822805a8e6be379d9bcf3ad9effef3dc after a few
  savevm/quit/loadvm cycles. I've narrowed it down to the following
  reproducer (further notes below):

  # qemu-img check debian.qcow2
  No errors were found on the image.
  251601/327680 = 76.78% allocated, 1.63% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208
  # bin/qemu/bin/qemu-system-x86_64 -machine pc-q35-4.0.1,accel=kvm -m 4096 
-chardev stdio,id=charmonitor -mon chardev=charmonitor -drive 
file=debian.qcow2,id=d -S
  qemu-system-x86_64: warning: dbind: Couldn't register with accessibility bus: 
Did not receive a reply. Possible causes include: the remote application did 
not send a reply, the message bus security policy blocked the reply, the reply 
timeout expired, or the network connection was broken.
  QEMU 4.1.50 monitor - type 'help' for more information
  (qemu) loadvm foo
  (qemu) c
  (qemu) qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  qcow2_free_clusters failed: Invalid argument
  quit
  [m@nargothrond:~] qemu-img check debian.qcow2
  Leaked cluster 85179 refcount=2 reference=1
  Leaked cluster 85180 refcount=2 reference=1
  ERROR cluster 266150 refcount=0 reference=2
  [...]
  ERROR OFLAG_COPIED data cluster: l2_entry=42284 refcount=1

  9493 errors were found on the image.
  Data may be corrupted, or further writes to the image may corrupt it.

  2 leaked clusters were found on the image.
  This means waste of disk space, but no harm to data.
  259266/327680 = 79.12% allocated, 1.67% fragmented, 0.00% compressed clusters
  Image end offset: 18340446208

  This is on a x86_64 Linux 5.3.1 Gentoo host with qemu-system-x86_64
  and accel=kvm. The compiler is gcc-9.2.0 with the rest of the system
  similarly current.

  Reproduced with qemu-4.1.0 from distribution package as well as
  vanilla git checkout of tag v4.1.0 and commit
  7f21573c822805a8e6be379d9bcf3ad9effef3dc (today's master). Does not
  happen with qemu compiled from vanilla checkout of tag v4.0.0. Build
  sequence:

  ./configure --prefix=$HOME/bin/qemu-bisect --target-list=x86_64-softmmu 
--disable-werror --disable-docs
  [...]
  CFLAGS-O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g
  [...] (can provide full configure output if helpful)
  make -j8 install

  The kind of guest OS does not matter: seen with Debian testing 64bit,
  Windows 7 x86/x64 BIOS and Windows 7 x64 EFI.

  The virtual storage controller does not seem to matter: seen with
  VirtIO SCSI, emulated SCSI and emulated SATA AHCI.

  Caching modes (none, directsync, writeback), aio mode (threads,
  native) or discard (ignore, unmap) or detect-zeroes (off, unmap) does
  not influence occurence either.

  Having more RAM in the guest seems to increase odds of corruption:
  With 512MB to the Debian guest problem hardly occurs at all, with 4GB
  RAM it happens almost instantly.

  An automated reproducer works as follows:

  - the guest *does* mount its root fs and swap with option discard and
  my testing leaves me with the impression that file deletion rather
  than reading is causing the issue

  - foo is a snapshot of the running Debian VM which is already running
  command

  # while true ; do dd if=/dev/zero of=foo bs=10240k count=400 ; done

  to produce some I/O to the disk (4GB file with 4GB of RAM).

  - on the host a loop continuously resumes and saves the guest state
  and quits qemu inbetween:

  # while true ; do (echo loadvm foo ; echo c ; sleep 10 ; echo stop ;
  echo savevm foo ; echo quit ) | bin/qemu-bisect/bin/qemu-system-x86_64
  -machine pc-q35-3.1,accel=kvm -m 4096 -chardev stdio,id=charmonitor
  -mon chardev=charmonitor -drive file=debian.qcow2,id=d -S -display
  none ; done

  - quitting qemu inbetween saves and loads seems to be necessary for
  the problem to occur. Just continusouly in one session saving and
  loading guest state

Re: [PATCH v4 08/40] target/arm: Rename ARMMMUIdx*_S12NSE* to ARMMMUIdx*_E10_*

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> This emphasizes that they apply to the EL1&0 regime.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  8 
>  target/arm/internals.h |  4 ++--
>  target/arm/helper.c| 40 +++---
>  target/arm/translate-a64.c |  4 ++--
>  target/arm/translate.c |  6 +++---
>  5 files changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9729e62d2c..802cddd2df 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2864,8 +2864,8 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>  #define ARM_MMU_IDX_COREIDX_MASK 0x7
>  
>  typedef enum ARMMMUIdx {
> -ARMMMUIdx_S12NSE0 = 0 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S12NSE1 = 1 | ARM_MMU_IDX_A,
> +ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
> +ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1SE0 = 4 | ARM_MMU_IDX_A,
> @@ -2890,8 +2890,8 @@ typedef enum ARMMMUIdx {
>   * for use when calling tlb_flush_by_mmuidx() and friends.
>   */
>  typedef enum ARMMMUIdxBit {
> -ARMMMUIdxBit_S12NSE0 = 1 << 0,
> -ARMMMUIdxBit_S12NSE1 = 1 << 1,
> +ARMMMUIdxBit_EL10_0 = 1 << 0,
> +ARMMMUIdxBit_EL10_1 = 1 << 1,
>  ARMMMUIdxBit_S1E2 = 1 << 2,
>  ARMMMUIdxBit_S1E3 = 1 << 3,
>  ARMMMUIdxBit_S1SE0 = 1 << 4,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index f5313dd3d4..54142dd789 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -808,8 +808,8 @@ static inline void arm_call_el_change_hook(ARMCPU *cpu)
>  static inline bool regime_is_secure(CPUARMState *env, ARMMMUIdx mmu_idx)
>  {
>  switch (mmu_idx) {
> -case ARMMMUIdx_S12NSE0:
> -case ARMMMUIdx_S12NSE1:
> +case ARMMMUIdx_EL10_0:
> +case ARMMMUIdx_EL10_1:
>  case ARMMMUIdx_S1NSE0:
>  case ARMMMUIdx_S1NSE1:
>  case ARMMMUIdx_S1E2:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 6c09cda4ea..d2b90763ca 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -670,8 +670,8 @@ static void tlbiall_nsnh_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  CPUState *cs = env_cpu(env);
>  
>  tlb_flush_by_mmuidx(cs,
> -ARMMMUIdxBit_S12NSE1 |
> -ARMMMUIdxBit_S12NSE0 |
> +ARMMMUIdxBit_EL10_1 |
> +ARMMMUIdxBit_EL10_0 |
>  ARMMMUIdxBit_S2NS);
>  }
>  
> @@ -681,8 +681,8 @@ static void tlbiall_nsnh_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  CPUState *cs = env_cpu(env);
>  
>  tlb_flush_by_mmuidx_all_cpus_synced(cs,
> -ARMMMUIdxBit_S12NSE1 |
> -ARMMMUIdxBit_S12NSE0 |
> +ARMMMUIdxBit_EL10_1 |
> +ARMMMUIdxBit_EL10_0 |
>  ARMMMUIdxBit_S2NS);
>  }
>  
> @@ -3068,7 +3068,7 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
> value,
>  format64 = arm_s1_regime_using_lpae_format(env, mmu_idx);
>  
>  if (arm_feature(env, ARM_FEATURE_EL2)) {
> -if (mmu_idx == ARMMMUIdx_S12NSE0 || mmu_idx == 
> ARMMMUIdx_S12NSE1) {
> +if (mmu_idx == ARMMMUIdx_EL10_0 || mmu_idx == ARMMMUIdx_EL10_1) {
>  format64 |= env->cp15.hcr_el2 & (HCR_VM | HCR_DC);
>  } else {
>  format64 |= arm_current_el(env) == 2;
> @@ -3167,11 +3167,11 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  break;
>  case 4:
>  /* stage 1+2 NonSecure PL1: ATS12NSOPR, ATS12NSOPW */
> -mmu_idx = ARMMMUIdx_S12NSE1;
> +mmu_idx = ARMMMUIdx_EL10_1;
>  break;
>  case 6:
>  /* stage 1+2 NonSecure PL0: ATS12NSOUR, ATS12NSOUW */
> -mmu_idx = ARMMMUIdx_S12NSE0;
> +mmu_idx = ARMMMUIdx_EL10_0;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3229,10 +3229,10 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S1NSE0;
>  break;
>  case 4: /* AT S12E1R, AT S12E1W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_S12NSE1;
> +mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_EL10_1;
>  break;
>  case 6: /* AT S12E0R, AT S12E0W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_S12NSE0;
> +mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_EL10_0;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3531,8 +3531,8 @@ static void vttbr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  /* Accesses to VTTBR may

Re: [PATCH v2 00/18] Error handling fixes

[Markus Armbruster]

Neglected to mention: I'm fine with taking care of getting the whole
series merged.  Maintainers, feel free to take "your" parts through your
tree regardless.  Whatever is left after a while I'll take through mine.

Re: [PULL v2 4/6] spapr: Add /chosen to FDT only at reset time to preserve kernel and initramdisk

[Laurent Vivier]

On 04/12/2019 05:40, Alexey Kardashevskiy wrote:
> 
> 
> On 04/12/2019 15:23, Alexey Kardashevskiy wrote:
>>
>>
>> On 04/12/2019 03:09, Laurent Vivier wrote:
>>>
>>> Bad reply, the problem is with
>>>
>>> "spapr: Render full FDT on ibm,client-architecture-support"
>>
>>
>> https://git.qemu.org/?p=SLOF.git;a=blob;f=board-qemu/slof/fdt.fs;h=3e4c1b34b8af2dcebde57e548c94417e5e20e1cc;hb=HEAD#l265
>>
>> A "bit ugly" became really ugly as before we were only patching
>> interrupt-map for PHB (7 cells per line) only but now we have to patch
>> (or, rather, skip) the PCI bridge interrupt-map (9 cells per line).
>>
>> Fixing now...
> 
> 
> Basically, this:
> 
> 
> diff --git a/board-qemu/slof/fdt.fs b/board-qemu/slof/fdt.fs
> index 3e4c1b34b8af..463a2a8c0c2d 100644
> --- a/board-qemu/slof/fdt.fs
> +++ b/board-qemu/slof/fdt.fs
> @@ -300,8 +300,13 @@ fdt-claim-reserve
> \ ." Replacing in " dup node>path type cr
> >r
> s" interrupt-map" r@ get-property 0= IF
> -  ( old new prop-addr prop-len  R: node )
> -  fdt-replace-interrupt-map
> +  dup e00 = IF
> +  ( old new prop-addr prop-len  R: node )
> +  fdt-replace-interrupt-map
> +  ELSE
> + 2drop
> +  ."  no idea what this is" cr
> +  THEN
> THEN

This does not fix the problem for me.

Thanks,
Laurent

Re: [PATCH v4 09/40] target/arm: Rename ARMMMUIdx_S2NS to ARMMMUIdx_Stage2

[Alex Bennée]

Richard Henderson  writes:

> The EL1&0 regime is the only one that uses 2-stage translation.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  4 +--
>  target/arm/internals.h |  2 +-
>  target/arm/helper.c| 57 --
>  target/arm/translate-a64.c |  2 +-
>  target/arm/translate.c |  2 +-
>  5 files changed, 35 insertions(+), 32 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 802cddd2df..fdb868f2e9 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2870,7 +2870,7 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1SE0 = 4 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1SE1 = 5 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S2NS = 6 | ARM_MMU_IDX_A,
> +ARMMMUIdx_Stage2 = 6 | ARM_MMU_IDX_A,
>  ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
>  ARMMMUIdx_MPriv = 1 | ARM_MMU_IDX_M,
>  ARMMMUIdx_MUserNegPri = 2 | ARM_MMU_IDX_M,
> @@ -2896,7 +2896,7 @@ typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_S1E3 = 1 << 3,
>  ARMMMUIdxBit_S1SE0 = 1 << 4,
>  ARMMMUIdxBit_S1SE1 = 1 << 5,
> -ARMMMUIdxBit_S2NS = 1 << 6,
> +ARMMMUIdxBit_Stage2 = 1 << 6,
>  ARMMMUIdxBit_MUser = 1 << 0,
>  ARMMMUIdxBit_MPriv = 1 << 1,
>  ARMMMUIdxBit_MUserNegPri = 1 << 2,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 54142dd789..ca8be78bbf 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -813,7 +813,7 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_S1NSE0:
>  case ARMMMUIdx_S1NSE1:
>  case ARMMMUIdx_S1E2:
> -case ARMMMUIdx_S2NS:
> +case ARMMMUIdx_Stage2:
>  case ARMMMUIdx_MPrivNegPri:
>  case ARMMMUIdx_MUserNegPri:
>  case ARMMMUIdx_MPriv:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index d2b90763ca..97677f8482 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -672,7 +672,7 @@ static void tlbiall_nsnh_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  tlb_flush_by_mmuidx(cs,
>  ARMMMUIdxBit_EL10_1 |
>  ARMMMUIdxBit_EL10_0 |
> -ARMMMUIdxBit_S2NS);
> +ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiall_nsnh_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -683,7 +683,7 @@ static void tlbiall_nsnh_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  tlb_flush_by_mmuidx_all_cpus_synced(cs,
>  ARMMMUIdxBit_EL10_1 |
>  ARMMMUIdxBit_EL10_0 |
> -ARMMMUIdxBit_S2NS);
> +ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiipas2_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -704,7 +704,7 @@ static void tlbiipas2_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  
>  pageaddr = sextract64(value << 12, 0, 40);
>  
> -tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_S2NS);
> +tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiipas2_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -720,7 +720,7 @@ static void tlbiipas2_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  pageaddr = sextract64(value << 12, 0, 40);
>  
>  tlb_flush_page_by_mmuidx_all_cpus_synced(cs, pageaddr,
> - ARMMMUIdxBit_S2NS);
> + ARMMMUIdxBit_Stage2);
>  }
>  
>  static void tlbiall_hyp_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -3528,12 +3528,15 @@ static void vttbr_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  ARMCPU *cpu = env_archcpu(env);
>  CPUState *cs = CPU(cpu);
>  
> -/* Accesses to VTTBR may change the VMID so we must flush the TLB.  */
> +/*
> + * A change in VMID to the stage2 page table (Stage2) invalidates
> + * the combined stage 1&2 tlbs (EL10_1 and EL10_0).
> + */
>  if (raw_read(env, ri) != value) {
>  tlb_flush_by_mmuidx(cs,
>  ARMMMUIdxBit_EL10_1 |
>  ARMMMUIdxBit_EL10_0 |
> -ARMMMUIdxBit_S2NS);
> +ARMMMUIdxBit_Stage2);
>  raw_write(env, ri, value);
>  }
>  }
> @@ -3929,7 +3932,7 @@ static int vmalle1_tlbmask(CPUARMState *env)
>  if (arm_is_secure_below_el3(env)) {
>  return ARMMMUIdxBit_S1SE1 | ARMMMUIdxBit_S1SE0;
>  } else if (arm_feature(env, ARM_FEATURE_EL2)) {
> -return ARMMMUIdxBit_EL10_1 | ARMMMUIdxBit_EL10_0 | ARMMMUIdxBit_S2NS;
> +return ARMMMUIdxBit_EL10_1 | ARMMMUIdxBit_EL10_0 | 
> ARMMMUIdxBit_Stage2;
>  } else {
>  return ARMMMUIdxBit_EL10_1 | ARMMMUIdxBit_EL10_0;
>  }
> @@ -4083,7 +4086,7 @@ static void tlbi_aa64_ipas2e1_write(CPUARMState *env

Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.

[Philippe Mathieu-Daudé]

On 12/4/19 10:05 AM, Damien Hedde wrote:

On 12/2/19 3:34 PM, Peter Maydell wrote:

On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:



[...]

+/**
+ * qdev_pass_clock:
+ * @dev: the device to forward the clock to
+ * @name: the name of the clock to be added (can't be NULL)
+ * @container: the device which already has the clock
+ * @cont_name: the name of the clock in the container device
+ *
+ * Add a clock @name to @dev which forward to the clock @cont_name in 
@container
+ */


'container' seems odd terminology here, because I would expect
the usual use of this function to be when a 'container' object
like an SoC wants to forward a clock to one of its components;
in that case the 'container' SoC would be @dev, wouldn't it?


Yes. I agree it is confusing.
This function just allow a a device 'A' to exhibit a clock from another
device 'B' (typically a composing sub-device, inside a soc like you
said). The device A is not supposed to interact with the clock itself.
The original sub-device is the true
owner/controller of the clock and is the only one which should use the
clock API: setting a callback on it (input clock); or updating the clock
frequency (output clock).
Basically, it just add the clock into the device clock namespace without
interfering with it.


We should get this to be the same way round as qdev_pass_gpios(),
which takes "DeviceState *dev, DeviceState *container", and
passes the gpios that exist on 'dev' over to 'container' so that
'container' now has gpios which it did not before.


Ok, I'll invert the arguments.



Also, your use of 'forward to' is inconsistent: in the 'dev'
documentation you say we're forwarding the clock to 'dev',
but in the body of the documentation you say we're forwarding
the clock to the clock in 'container'.


I'll try to clarify this and make the documentation more consistent with
the comments here.



I think the way to resolve this is to stick to the terminology
in the function name itself:
  @dev: the device which has the clock
  @name: the name of the clock on @dev
  @container: the name of the device which the clock should
   be passed to
  @cont_name: the name to use for the clock on @container


I think container is confusing because depending on how we reason (clock
in a device; device in another device), we end up thinking the opposite.

Maybe we can use "exhibit" instead of "container" in the 2nd pair of
parameters, or prefix use "origin" or "owner" as a prefix for the first
pair of parameters.


@sink vs @source?


Q: if you pass a clock to another device with this function,
does it still exist to be used directly on the original
device? For qdev_pass_gpios it does not (I think), but
this is more accident of implementation than anything else.


It depends what we mean by "used by".
Original device can:
+ set the callback in case it is an input
+ update the frequency in case it is an output


Hmm here you use @input vs @output...


But since an input clock can only be connected once,
I think the logic here is that any connection should now go through the
new device. But this is not checked and using one or the other is
exactly the same.

Maybe I misunderstood the meaning of qdev_pass_gpios().

[...]

Re: [PATCH v2 3/3] virtio-serial-bus: fix memory leak while attach virtio-serial-bus

[Laurent Vivier]

On 04/12/2019 08:31, pannengy...@huawei.com wrote:
> From: Pan Nengyuan 
> 
> ivqs/ovqs/c_ivq/c_ovq is forgot to cleanup in
> virtio_serial_device_unrealize, the memory leak stack is as bellow:
> 
> Direct leak of 1290240 byte(s) in 180 object(s) allocated from:
> #0 0x7fc9bfc27560 in calloc (/usr/lib64/libasan.so.3+0xc7560)
> #1 0x7fc9bed6f015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015)
> #2 0x5650e02b83e7 in virtio_add_queue hw/virtio/virtio.c:2327
> #3 0x5650e02847b5 in virtio_serial_device_realize 
> hw/char/virtio-serial-bus.c:1089
> #4 0x5650e02b56a7 in virtio_device_realize hw/virtio/virtio.c:3504
> #5 0x5650e03bf031 in device_set_realized hw/core/qdev.c:876
> #6 0x5650e0531efd in property_set_bool qom/object.c:2080
> #7 0x5650e053650e in object_property_set_qobject qom/qom-qobject.c:26
> #8 0x5650e0533e14 in object_property_set_bool qom/object.c:1338
> #9 0x5650e04c0e37 in virtio_pci_realize hw/virtio/virtio-pci.c:1801
> 
> Reported-by: Euler Robot 
> Signed-off-by: Pan Nengyuan 
> Cc: Laurent Vivier 
> Cc: Amit Shah 
> Cc: "Marc-André Lureau" 
> Cc: Paolo Bonzini 
> ---
> Changes v2 to v1:
> - use virtio_delete_queue to cleanup vq through a vq pointer (suggested by
>   Michael S. Tsirkin)
> ---
>  hw/char/virtio-serial-bus.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index 3325904..e1cbce3 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -1126,9 +1126,17 @@ static void virtio_serial_device_unrealize(DeviceState 
> *dev, Error **errp)
>  {
>  VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>  VirtIOSerial *vser = VIRTIO_SERIAL(dev);
> +int i;
>  
>  QLIST_REMOVE(vser, next);
>  
> +virtio_delete_queue(vser->c_ivq);
> +virtio_delete_queue(vser->c_ovq);
> +for (i = 0; i < vser->bus.max_nr_ports; i++) {
> +virtio_delete_queue(vser->ivqs[i]);
> +virtio_delete_queue(vser->ovqs[i]);
> +}
> +
>  g_free(vser->ivqs);
>  g_free(vser->ovqs);
>  g_free(vser->ports_map);
> 

Reviewed-by: Laurent Vivier 

[ANNOUNCE] libslirp 4.1.0 is now available

[Marc-André Lureau]

Hi,

We are pleased to announce the availability of libslirp 4.1.0
release.

You can grab the tarball & read the changelog from the gitlab release page here:

https://gitlab.freedesktop.org/slirp/libslirp/releases

Highlights include:

 * new slirp_new() API, more flexible than slirp_init()
 * various new options needed for slirp4netns
 * tcp_emu() is now disabled by default. tcp_emu() is known to have
caused several CVEs, and not useful today in most cases. The feature
can be still enabled by setting SlirpConfig.enable_emu to true.
 * bug fixes & cleanups

Once the library installed, you may compile QEMU >= 4.0 with
--enable-slirp=system to link against it.

podman/slirp4netns should link with the system library soon
(https://github.com/rootless-containers/slirp4netns/pull/162)

Thank you to everyone involved!

-- 
Marc-André Lureau

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Thomas Huth]

On 04/12/2019 12.32, Janosch Frank wrote:
> On 12/4/19 11:48 AM, Thomas Huth wrote:
>> On 29/11/2019 10.47, Janosch Frank wrote:
>>> When a guest has saved a ipib of type 5 and call diagnose308 with
>>> subcode 10, we have to setup the protected processing environment via
>>> Ultravisor calls. The calls are done by KVM and are exposed via an API.
>>>
>>> The following steps are necessary:
>>> 1. Create a VM (register it with the Ultravisor)
>>> 2. Create secure CPUs for all of our current cpus
>>> 3. Forward the secure header to the Ultravisor (has all information on
>>> how to decrypt the image and VM information)
>>> 4. Protect image pages from the host and decrypt them
>>> 5. Verify the image integrity
>>>
>>> Only after step 5 a protected VM is allowed to run.
>>>
>>> Signed-off-by: Janosch Frank 
>>> ---
>> [...]
>>> +++ b/hw/s390x/pv.c
>>> @@ -0,0 +1,118 @@
>>> +/*
>>> + * Secure execution functions
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s):
>>> + *  Janosch Frank 
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +#include "qemu/osdep.h"
>>> +#include 
>>> +
>>> +#include 
>>> +
>>> +#include "qemu/error-report.h"
>>> +#include "sysemu/kvm.h"
>>> +#include "pv.h"
>>> +
>>> +static int s390_pv_cmd(uint32_t cmd, void *data)
>>> +{
>>> +int rc;
>>> +struct kvm_pv_cmd pv_cmd = {
>>> +.cmd = cmd,
>>> +.data = (uint64_t)data,
>>> +};
>>> +
>>> +rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
>>> +if (rc) {
>>> +error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
>>> +exit(1);
>>> +}
>>> +return rc;
>>> +}
>>> +
>>> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
>>> +{
>>> +int rc;
>>> +struct kvm_pv_cmd pv_cmd = {
>>> +.cmd = cmd,
>>> +.data = (uint64_t)data,
>>> +};
>>> +
>>> +rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
>>> +if (rc) {
>>> +error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
>>> +exit(1);
>>> +}
>>> +return rc;
>>> +}
>>> +
>>> +int s390_pv_vm_create(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
>>> +}
>>> +
>>> +int s390_pv_vm_destroy(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
>>> +}
>>> +
>>> +int s390_pv_vcpu_create(CPUState *cs)
>>> +{
>>> +return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
>>> +}
>>> +
>>> +int s390_pv_vcpu_destroy(CPUState *cs)
>>> +{
>>> +S390CPU *cpu = S390_CPU(cs);
>>> +CPUS390XState *env = &cpu->env;
>>> +int rc;
>>> +
>>> +rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
>>> +if (!rc) {
>>> +env->pv = false;
>>> +}
>>> +return rc;
>>> +}
>>> +
>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>>> +{
>>> +struct kvm_s390_pv_sec_parm args = {
>>> +.origin = origin,
>>> +.length = length,
>>> +};
>>> +
>>> +return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>>> +}
>>> +
>>> +/*
>>> + * Called for each component in the SE type IPL parameter block 0.
>>> + */
>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>>> +{
>>> +struct kvm_s390_pv_unp args = {
>>> +.addr = addr,
>>> +.size = size,
>>> +.tweak = tweak,
>>> +};
>>> +
>>> +return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
>>> +}
>>> +
>>> +int s390_pv_perf_clear_reset(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
>>> +}
>>> +
>>> +int s390_pv_verify(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>>> +}
>>> +
>>> +int s390_pv_unshare(void)
>>> +{
>>> +return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
>>> +}
>>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>>> new file mode 100644
>>> index 00..eb074e4bc9
>>> --- /dev/null
>>> +++ b/hw/s390x/pv.h
>>> @@ -0,0 +1,26 @@
>>> +/*
>>> + * Protected Virtualization header
>>> + *
>>> + * Copyright IBM Corp. 2019
>>> + * Author(s):
>>> + *  Janosch Frank 
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>>> + * your option) any later version. See the COPYING file in the top-level
>>> + * directory.
>>> + */
>>> +
>>> +#ifndef HW_S390_PV_H
>>> +#define HW_S390_PV_H
>>> +
>>> +int s390_pv_vm_create(void);
>>> +int s390_pv_vm_destroy(void);
>>> +int s390_pv_vcpu_destroy(CPUState *cs);
>>> +int s390_pv_vcpu_create(CPUState *cs);
>>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>>> +int s390_pv_perf_clear_reset(void);
>>> +int s390_pv_verify(void);
>>> +int s390_pv_unshare(void);
>>
>> I still think you should make all those functions returning "void"
>> instead of "int" - since errors results in an exit() in s390_pv_cmd()
>> and s390_pv_cmd_vcpu() anyway...
> 

Re: [PATCH v4 12/40] target/arm: Rename ARMMMUIdx*_S1E3 to ARMMMUIdx*_SE3

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The EL3 regime only has a single stage translation, and
> is always secure.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  4 ++--
>  target/arm/internals.h |  2 +-
>  target/arm/helper.c| 14 +++---
>  target/arm/translate.c |  2 +-
>  4 files changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index e8ee316e05..f307de561a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2867,7 +2867,7 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
>  ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
> +ARMMMUIdx_SE3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE0 = 4 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE1 = 5 | ARM_MMU_IDX_A,
>  ARMMMUIdx_Stage2 = 6 | ARM_MMU_IDX_A,
> @@ -2893,7 +2893,7 @@ typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_EL10_0 = 1 << 0,
>  ARMMMUIdxBit_EL10_1 = 1 << 1,
>  ARMMMUIdxBit_S1E2 = 1 << 2,
> -ARMMMUIdxBit_S1E3 = 1 << 3,
> +ARMMMUIdxBit_SE3 = 1 << 3,
>  ARMMMUIdxBit_SE0 = 1 << 4,
>  ARMMMUIdxBit_SE1 = 1 << 5,
>  ARMMMUIdxBit_Stage2 = 1 << 6,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3600bf9122..50d258b0e1 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -819,7 +819,7 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_MPriv:
>  case ARMMMUIdx_MUser:
>  return false;
> -case ARMMMUIdx_S1E3:
> +case ARMMMUIdx_SE3:
>  case ARMMMUIdx_SE0:
>  case ARMMMUIdx_SE1:
>  case ARMMMUIdx_MSPrivNegPri:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 377825431a..98d00b4549 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3138,7 +3138,7 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  /* stage 1 current state PL1: ATS1CPR, ATS1CPW */
>  switch (el) {
>  case 3:
> -mmu_idx = ARMMMUIdx_S1E3;
> +mmu_idx = ARMMMUIdx_SE3;
>  break;
>  case 2:
>  mmu_idx = ARMMMUIdx_Stage1_E1;
> @@ -3220,7 +3220,7 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  mmu_idx = ARMMMUIdx_S1E2;
>  break;
>  case 6: /* AT S1E3R, AT S1E3W */
> -mmu_idx = ARMMMUIdx_S1E3;
> +mmu_idx = ARMMMUIdx_SE3;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3963,7 +3963,7 @@ static void tlbi_aa64_alle3_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  ARMCPU *cpu = env_archcpu(env);
>  CPUState *cs = CPU(cpu);
>  
> -tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_S1E3);
> +tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_alle1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -3988,7 +3988,7 @@ static void tlbi_aa64_alle3is_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_S1E3);
> +tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_vae2_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -4016,7 +4016,7 @@ static void tlbi_aa64_vae3_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  CPUState *cs = CPU(cpu);
>  uint64_t pageaddr = sextract64(value << 12, 0, 56);
>  
> -tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_S1E3);
> +tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_vae1is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -4065,7 +4065,7 @@ static void tlbi_aa64_vae3is_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  uint64_t pageaddr = sextract64(value << 12, 0, 56);
>  
>  tlb_flush_page_by_mmuidx_all_cpus_synced(cs, pageaddr,
> - ARMMMUIdxBit_S1E3);
> + ARMMMUIdxBit_SE3);
>  }
>  
>  static void tlbi_aa64_ipas2e1_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -8567,7 +8567,7 @@ static inline uint32_t regime_el(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_Stage2:
>  case ARMMMUIdx_S1E2:
>  return 2;
> -case ARMMMUIdx_S1E3:
> +case ARMMMUIdx_SE3:
>  return 3;
>  case ARMMMUIdx_SE0:
>  return arm_el_is_aa64(env, 3) ? 1 : 3;
> diff --git a/target/arm/translate.c b/target/arm/translate.c
> index 787e34f258..6cf2fe2806 100644
> --- a/target/arm/translate.c
> +++ b/target/arm/translate.c
> @@ -156,7 +156,7 @@ static inline int get_a32_user_mem_index(DisasContext *s)
>  case ARMMMUIdx_EL10_0:
>  case ARMMMUIdx_EL10_1:
>  return arm_to_core_mmu_idx(ARMMMUIdx_EL10_0);
> - 

Re: [PATCH v4 10/40] target/arm: Rename ARMMMUIdx_S1NSE* to ARMMMUIdx_Stage1_E*

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The EL1&0 regime is the only one that uses 2-stage translation.
> Spelling out Stage avoids confusion with Secure.
>
> Signed-off-by: Richard Henderson 

> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 97677f8482..a34accec20 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -2992,7 +2992,8 @@ static uint64_t do_ats_write(CPUARMState *env, uint64_t 
> value,
>  bool take_exc = false;
>  
>  if (fi.s1ptw && current_el == 1 && !arm_is_secure(env)
> -&& (mmu_idx == ARMMMUIdx_S1NSE1 || mmu_idx == ARMMMUIdx_S1NSE0)) 
> {
> +&& (mmu_idx == ARMMMUIdx_Stage1_E1
> +|| mmu_idx == ARMMMUIdx_Stage1_E0)) {

Personal nit: I think ||\nfoo == scans more nicely as it lines up but
maybe that's just me.

Reviewed-by: Alex Bennée 

-- 
Alex Bennée

Re: [PATCH v6 5/9] qdev-clock: introduce an init array to ease the device construction

[Damien Hedde]

On 12/2/19 4:13 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Introduce a function and macro helpers to setup several clocks
>> in a device from a static array description.
>>
>> An element of the array describes the clock (name and direction) as
>> well as the related callback and an optional offset to store the
>> created object pointer in the device state structure.
>>
>> The array must be terminated by a special element QDEV_CLOCK_END.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> ---
>>  hw/core/qdev-clock.c| 26 
>>  include/hw/qdev-clock.h | 67 +
>>  2 files changed, 93 insertions(+)
>>
>> diff --git a/hw/core/qdev-clock.c b/hw/core/qdev-clock.c
>> index bebdd8fa15..32ad45c061 100644
>> --- a/hw/core/qdev-clock.c
>> +++ b/hw/core/qdev-clock.c
>> @@ -153,3 +153,29 @@ void qdev_connect_clock_out(DeviceState *dev, const 
>> char *name, ClockIn *clk,
>>
>>  clock_connect(clk, clkout);
>>  }
>> +
>> +void qdev_init_clocks(DeviceState *dev, const ClockPortInitArray clocks)
>> +{
>> +const struct ClockPortInitElem *elem;
>> +
>> +assert(dev);
>> +assert(clocks);
> 
> More unnecessary asserts, I think.
> 
> 
> 
>> +/**
>> + * ClockInitElem:
>> + * @name: name of the clock (can't be NULL)
>> + * @is_output: indicates whether the clock is input or output
>> + * @callback: for inputs, optional callback to be called on clock's update
>> + * with device as opaque
>> + * @offset: optional offset to store the ClockIn or ClockOut pointer in 
>> device
>> + * state structure (0 means unused)
>> + */
>> +struct ClockPortInitElem {
>> +const char *name;
>> +bool is_output;
>> +ClockCallback *callback;
>> +size_t offset;
>> +};
>> +
>> +#define clock_offset_value(_type, _devstate, _field) \
>> +(offsetof(_devstate, _field) + \
>> + type_check(_type *, typeof_field(_devstate, _field)))
> 
> Avoid leading underscores, please.
> 
>> +
>> +#define QDEV_CLOCK(_is_output, _type, _devstate, _field, _callback) { \
>> +.name = (stringify(_field)), \
>> +.is_output = _is_output, \
>> +.callback = _callback, \
>> +.offset = clock_offset_value(_type, _devstate, _field), \
>> +}
>> +
>> +/**
>> + * QDEV_CLOCK_(IN|OUT):
>> + * @_devstate: structure type. @dev argument of qdev_init_clocks below must 
>> be
>> + * a pointer to that same type.
> 
> It's a bit unclear what "below" here is referring to. Maybe
> just have this be "@devstate: name of a C struct type"
> and then explain below...
> 
>> + * @_field: a field in @_devstate (must be ClockIn* or ClockOut*)
>> + * @_callback: (for input only) callback (or NULL) to be called with the 
>> device
>> + * state as argument
>> + *
> 
> ...here, where we can have a paragraph giving the purpose
> of the macro:
> 
> "Define an entry in a ClockPortInitArray which is intended
> to be passed to qdev_init_clocks(), which should be called
> with an @dev argument which is a pointer to the @devstate
> struct type."

Sounds good.

> 
>> + * The name of the clock will be derived from @_field
> 
> Derived how? Guessing from the stringify(_field) above that it
> will be the same as the field name ?

yes.

> 
> It makes sense to hardcode the opaque pointer for the callback to be
> the device pointer.
> 
> 
>> + */
>> +#define QDEV_CLOCK_IN(_devstate, _field, _callback) \
>> +QDEV_CLOCK(false, ClockIn, _devstate, _field, _callback)
>> +
>> +#define QDEV_CLOCK_OUT(_devstate, _field) \
>> +QDEV_CLOCK(true, ClockOut, _devstate, _field, NULL)
>> +
>> +/**
>> + * QDEV_CLOCK_IN_NOFIELD:
>> + * @_name: name of the clock
>> + * @_callback: callback (or NULL) to be called with the device state as 
>> argument
>> + */
>> +#define QDEV_CLOCK_IN_NOFIELD(_name, _callback) { \
>> +.name = _name, \
>> +.is_output = false, \
>> +.callback = _callback, \
>> +.offset = 0, \
>> +}
> 
> When would we want to use this one ?

If the callback interaction is enough, we don't need to access the clock
object directly. So we don't need the field in the device state
structure. I can remove this macro for sake of simplicity.

--
Damien

Re: [PATCH v4 11/40] target/arm: Rename ARMMMUIdx_S1SE* to ARMMMUIdx_SE*

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The Secure regimes all have a single stage translation;
> there is no point in pointing out that the idx is for stage1.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  8 
>  target/arm/internals.h |  4 ++--
>  target/arm/translate.h |  2 +-
>  target/arm/helper.c| 26 +-
>  target/arm/translate-a64.c |  4 ++--
>  target/arm/translate.c |  6 +++---
>  6 files changed, 25 insertions(+), 25 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 0714c52176..e8ee316e05 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2868,8 +2868,8 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
>  ARMMMUIdx_S1E3 = 3 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1SE0 = 4 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1SE1 = 5 | ARM_MMU_IDX_A,
> +ARMMMUIdx_SE0 = 4 | ARM_MMU_IDX_A,
> +ARMMMUIdx_SE1 = 5 | ARM_MMU_IDX_A,
>  ARMMMUIdx_Stage2 = 6 | ARM_MMU_IDX_A,
>  ARMMMUIdx_MUser = 0 | ARM_MMU_IDX_M,
>  ARMMMUIdx_MPriv = 1 | ARM_MMU_IDX_M,
> @@ -2894,8 +2894,8 @@ typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_EL10_1 = 1 << 1,
>  ARMMMUIdxBit_S1E2 = 1 << 2,
>  ARMMMUIdxBit_S1E3 = 1 << 3,
> -ARMMMUIdxBit_S1SE0 = 1 << 4,
> -ARMMMUIdxBit_S1SE1 = 1 << 5,
> +ARMMMUIdxBit_SE0 = 1 << 4,
> +ARMMMUIdxBit_SE1 = 1 << 5,
>  ARMMMUIdxBit_Stage2 = 1 << 6,
>  ARMMMUIdxBit_MUser = 1 << 0,
>  ARMMMUIdxBit_MPriv = 1 << 1,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 3fd1518f3b..3600bf9122 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -820,8 +820,8 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_MUser:
>  return false;
>  case ARMMMUIdx_S1E3:
> -case ARMMMUIdx_S1SE0:
> -case ARMMMUIdx_S1SE1:
> +case ARMMMUIdx_SE0:
> +case ARMMMUIdx_SE1:
>  case ARMMMUIdx_MSPrivNegPri:
>  case ARMMMUIdx_MSUserNegPri:
>  case ARMMMUIdx_MSPriv:
> diff --git a/target/arm/translate.h b/target/arm/translate.h
> index dd24f91f26..3760159661 100644
> --- a/target/arm/translate.h
> +++ b/target/arm/translate.h
> @@ -124,7 +124,7 @@ static inline int default_exception_el(DisasContext *s)
>   * exceptions can only be routed to ELs above 1, so we target the higher 
> of
>   * 1 or the current EL.
>   */
> -return (s->mmu_idx == ARMMMUIdx_S1SE0 && s->secure_routed_to_el3)
> +return (s->mmu_idx == ARMMMUIdx_SE0 && s->secure_routed_to_el3)
>  ? 3 : MAX(1, s->current_el);
>  }
>  
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index a34accec20..377825431a 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3144,7 +3144,7 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  mmu_idx = ARMMMUIdx_Stage1_E1;
>  break;
>  case 1:
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_Stage1_E1;
> +mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_Stage1_E1;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3154,13 +3154,13 @@ static void ats_write(CPUARMState *env, const 
> ARMCPRegInfo *ri, uint64_t value)
>  /* stage 1 current state PL0: ATS1CUR, ATS1CUW */
>  switch (el) {
>  case 3:
> -mmu_idx = ARMMMUIdx_S1SE0;
> +mmu_idx = ARMMMUIdx_SE0;
>  break;
>  case 2:
>  mmu_idx = ARMMMUIdx_Stage1_E0;
>  break;
>  case 1:
> -mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_Stage1_E0;
> +mmu_idx = secure ? ARMMMUIdx_SE0 : ARMMMUIdx_Stage1_E0;
>  break;
>  default:
>  g_assert_not_reached();
> @@ -3214,7 +3214,7 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  case 0:
>  switch (ri->opc1) {
>  case 0: /* AT S1E1R, AT S1E1W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_Stage1_E1;
> +mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_Stage1_E1;
>  break;
>  case 4: /* AT S1E2R, AT S1E2W */
>  mmu_idx = ARMMMUIdx_S1E2;
> @@ -3227,13 +3227,13 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  }
>  break;
>  case 2: /* AT S1E0R, AT S1E0W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE0 : ARMMMUIdx_Stage1_E0;
> +mmu_idx = secure ? ARMMMUIdx_SE0 : ARMMMUIdx_Stage1_E0;
>  break;
>  case 4: /* AT S12E1R, AT S12E1W */
> -mmu_idx = secure ? ARMMMUIdx_S1SE1 : ARMMMUIdx_EL10_1;
> +mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_EL10_1;
>  break;
>  case 6: /* AT S12E0R, AT S12E0W */
> -mmu_idx = secure ? ARMMM

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Thomas Huth]

On 29/11/2019 10.47, Janosch Frank wrote:
> When a guest has saved a ipib of type 5 and call diagnose308 with
> subcode 10, we have to setup the protected processing environment via
> Ultravisor calls. The calls are done by KVM and are exposed via an API.
> 
> The following steps are necessary:
> 1. Create a VM (register it with the Ultravisor)
> 2. Create secure CPUs for all of our current cpus
> 3. Forward the secure header to the Ultravisor (has all information on
> how to decrypt the image and VM information)
> 4. Protect image pages from the host and decrypt them
> 5. Verify the image integrity
> 
> Only after step 5 a protected VM is allowed to run.
> 
> Signed-off-by: Janosch Frank 
> ---
[...]
> +++ b/hw/s390x/pv.c
> @@ -0,0 +1,118 @@
> +/*
> + * Secure execution functions
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s):
> + *  Janosch Frank 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +#include "qemu/osdep.h"
> +#include 
> +
> +#include 
> +
> +#include "qemu/error-report.h"
> +#include "sysemu/kvm.h"
> +#include "pv.h"
> +
> +static int s390_pv_cmd(uint32_t cmd, void *data)
> +{
> +int rc;
> +struct kvm_pv_cmd pv_cmd = {
> +.cmd = cmd,
> +.data = (uint64_t)data,
> +};
> +
> +rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
> +if (rc) {
> +error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
> +exit(1);
> +}
> +return rc;
> +}
> +
> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
> +{
> +int rc;
> +struct kvm_pv_cmd pv_cmd = {
> +.cmd = cmd,
> +.data = (uint64_t)data,
> +};
> +
> +rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
> +if (rc) {
> +error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
> +exit(1);
> +}
> +return rc;
> +}
> +
> +int s390_pv_vm_create(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
> +}
> +
> +int s390_pv_vm_destroy(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
> +}
> +
> +int s390_pv_vcpu_create(CPUState *cs)
> +{
> +return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
> +}
> +
> +int s390_pv_vcpu_destroy(CPUState *cs)
> +{
> +S390CPU *cpu = S390_CPU(cs);
> +CPUS390XState *env = &cpu->env;
> +int rc;
> +
> +rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
> +if (!rc) {
> +env->pv = false;
> +}
> +return rc;
> +}
> +
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
> +{
> +struct kvm_s390_pv_sec_parm args = {
> +.origin = origin,
> +.length = length,
> +};
> +
> +return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
> +}
> +
> +/*
> + * Called for each component in the SE type IPL parameter block 0.
> + */
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
> +{
> +struct kvm_s390_pv_unp args = {
> +.addr = addr,
> +.size = size,
> +.tweak = tweak,
> +};
> +
> +return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
> +}
> +
> +int s390_pv_perf_clear_reset(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
> +}
> +
> +int s390_pv_verify(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
> +}
> +
> +int s390_pv_unshare(void)
> +{
> +return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
> +}
> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
> new file mode 100644
> index 00..eb074e4bc9
> --- /dev/null
> +++ b/hw/s390x/pv.h
> @@ -0,0 +1,26 @@
> +/*
> + * Protected Virtualization header
> + *
> + * Copyright IBM Corp. 2019
> + * Author(s):
> + *  Janosch Frank 
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> + * your option) any later version. See the COPYING file in the top-level
> + * directory.
> + */
> +
> +#ifndef HW_S390_PV_H
> +#define HW_S390_PV_H
> +
> +int s390_pv_vm_create(void);
> +int s390_pv_vm_destroy(void);
> +int s390_pv_vcpu_destroy(CPUState *cs);
> +int s390_pv_vcpu_create(CPUState *cs);
> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
> +int s390_pv_perf_clear_reset(void);
> +int s390_pv_verify(void);
> +int s390_pv_unshare(void);

I still think you should make all those functions returning "void"
instead of "int" - since errors results in an exit() in s390_pv_cmd()
and s390_pv_cmd_vcpu() anyway...

> +
> +#endif /* HW_S390_PV_H */
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index c1d1440272..f9481ccace 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -41,6 +41,7 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/s390x/tod.h"
>  #include "sysemu/sysemu.h"
> +#include "hw/s390x/pv.h"
>  
>  S390CPU *s390_cpu_addr2state(uint16_t cpu_addr)
>  {
> @@ -322,6 +323,7

Re: [PATCH v4 13/40] target/arm: Rename ARMMMUIdx_S1E2 to ARMMMUIdx_E2

[Alex Bennée]

Richard Henderson  writes:

> This is part of a reorganization to the set of mmu_idx.
> The non-secure EL2 regime only has a single stage translation;
> there is no point in pointing out that the idx is for stage1.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h   |  4 ++--
>  target/arm/internals.h |  2 +-
>  target/arm/helper.c| 22 +++---
>  target/arm/translate.c |  2 +-
>  4 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index f307de561a..28259be733 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2866,7 +2866,7 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>  typedef enum ARMMMUIdx {
>  ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
>  ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
> -ARMMMUIdx_S1E2 = 2 | ARM_MMU_IDX_A,
> +ARMMMUIdx_E2 = 2 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE3 = 3 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE0 = 4 | ARM_MMU_IDX_A,
>  ARMMMUIdx_SE1 = 5 | ARM_MMU_IDX_A,
> @@ -2892,7 +2892,7 @@ typedef enum ARMMMUIdx {
>  typedef enum ARMMMUIdxBit {
>  ARMMMUIdxBit_EL10_0 = 1 << 0,
>  ARMMMUIdxBit_EL10_1 = 1 << 1,
> -ARMMMUIdxBit_S1E2 = 1 << 2,
> +ARMMMUIdxBit_E2 = 1 << 2,
>  ARMMMUIdxBit_SE3 = 1 << 3,
>  ARMMMUIdxBit_SE0 = 1 << 4,
>  ARMMMUIdxBit_SE1 = 1 << 5,
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index 50d258b0e1..aee54dc105 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -812,7 +812,7 @@ static inline bool regime_is_secure(CPUARMState *env, 
> ARMMMUIdx mmu_idx)
>  case ARMMMUIdx_EL10_1:
>  case ARMMMUIdx_Stage1_E0:
>  case ARMMMUIdx_Stage1_E1:
> -case ARMMMUIdx_S1E2:
> +case ARMMMUIdx_E2:
>  case ARMMMUIdx_Stage2:
>  case ARMMMUIdx_MPrivNegPri:
>  case ARMMMUIdx_MUserNegPri:
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 98d00b4549..5172843667 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -728,7 +728,7 @@ static void tlbiall_hyp_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbiall_hyp_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -736,7 +736,7 @@ static void tlbiall_hyp_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbimva_hyp_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -745,7 +745,7 @@ static void tlbimva_hyp_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  CPUState *cs = env_cpu(env);
>  uint64_t pageaddr = value & ~MAKE_64BIT_MASK(0, 12);
>  
> -tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_S1E2);
> +tlb_flush_page_by_mmuidx(cs, pageaddr, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbimva_hyp_is_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -755,7 +755,7 @@ static void tlbimva_hyp_is_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  uint64_t pageaddr = value & ~MAKE_64BIT_MASK(0, 12);
>  
>  tlb_flush_page_by_mmuidx_all_cpus_synced(cs, pageaddr,
> - ARMMMUIdxBit_S1E2);
> + ARMMMUIdxBit_E2);
>  }
>  
>  static const ARMCPRegInfo cp_reginfo[] = {
> @@ -3189,7 +3189,7 @@ static void ats1h_write(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  MMUAccessType access_type = ri->opc2 & 1 ? MMU_DATA_STORE : 
> MMU_DATA_LOAD;
>  uint64_t par64;
>  
> -par64 = do_ats_write(env, value, access_type, ARMMMUIdx_S1E2);
> +par64 = do_ats_write(env, value, access_type, ARMMMUIdx_E2);
>  
>  A32_BANKED_CURRENT_REG_SET(env, par, par64);
>  }
> @@ -3217,7 +3217,7 @@ static void ats_write64(CPUARMState *env, const 
> ARMCPRegInfo *ri,
>  mmu_idx = secure ? ARMMMUIdx_SE1 : ARMMMUIdx_Stage1_E1;
>  break;
>  case 4: /* AT S1E2R, AT S1E2W */
> -mmu_idx = ARMMMUIdx_S1E2;
> +mmu_idx = ARMMMUIdx_E2;
>  break;
>  case 6: /* AT S1E3R, AT S1E3W */
>  mmu_idx = ARMMMUIdx_SE3;
> @@ -3954,7 +3954,7 @@ static void tlbi_aa64_alle2_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  ARMCPU *cpu = env_archcpu(env);
>  CPUState *cs = CPU(cpu);
>  
> -tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_mmuidx(cs, ARMMMUIdxBit_E2);
>  }
>  
>  static void tlbi_aa64_alle3_write(CPUARMState *env, const ARMCPRegInfo *ri,
> @@ -3980,7 +3980,7 @@ static void tlbi_aa64_alle2is_write(CPUARMState *env, 
> const ARMCPRegInfo *ri,
>  {
>  CPUState *cs = env_cpu(env);
>  
> -tlb_flush_by_mmuidx_all_cpus_synced(cs, ARMMMUIdxBit_S1E2);
> +tlb_flush_by_

Re: [PATCH v4 14/40] target/arm: Recover 4 bits from TBFLAGs

[Alex Bennée]

Richard Henderson  writes:

> We had completely run out of TBFLAG bits.
> Split A- and M-profile bits into two overlapping buckets.
> This results in 4 free bits.
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/cpu.h   | 52 ---
>  target/arm/helper.c| 17 ++---
>  target/arm/translate.c | 56 +++---
>  3 files changed, 70 insertions(+), 55 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 28259be733..ae9fc1ded3 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3188,38 +3188,50 @@ FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
>   */
>  FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)

I'm not sure if this visual aid helps but here you go:

 *  31  20 1916 15 10 90
 * +--++-+--+
 * |  ||   TBFLAG_A64   |
 * |  | +--+-+--+
 * | TBFLAG_ANY   | |   TBFLAG_A32   |  |
 * |  | +-+--+  TBFLAG_AM32 |
 * |  |   |TBFLAG_M32|  |
 * +--+---+--+--+

>  
> -/* Bit usage when in AArch32 state: */
> -FIELD(TBFLAG_A32, THUMB, 0, 1)  /* Not cached. */
> -FIELD(TBFLAG_A32, VECLEN, 1, 3) /* Not cached. */
> -FIELD(TBFLAG_A32, VECSTRIDE, 4, 2)  /* Not cached. */
> +/*
> + * Bit usage when in AArch32 state, both A- and M-profile.
> + */
> +FIELD(TBFLAG_AM32, CONDEXEC, 0, 8)  /* Not cached. */
> +FIELD(TBFLAG_AM32, THUMB, 8, 1) /* Not cached. */
> +
> +/*
> + * Bit usage when in AArch32 state, for A-profile only.
> + */
> +FIELD(TBFLAG_A32, VECLEN, 9, 3) /* Not cached. */
> +FIELD(TBFLAG_A32, VECSTRIDE, 12, 2) /* Not cached. */
>  /*
>   * We store the bottom two bits of the CPAR as TB flags and handle
>   * checks on the other bits at runtime. This shares the same bits as
>   * VECSTRIDE, which is OK as no XScale CPU has VFP.
>   * Not cached, because VECLEN+VECSTRIDE are not cached.
>   */
> -FIELD(TBFLAG_A32, XSCALE_CPAR, 4, 2)
> +FIELD(TBFLAG_A32, XSCALE_CPAR, 12, 2)
> +FIELD(TBFLAG_A32, VFPEN, 14, 1) /* Partially cached, minus FPEXC. */
> +FIELD(TBFLAG_A32, SCTLR_B, 15, 1)
>  /*
>   * Indicates whether cp register reads and writes by guest code should access
>   * the secure or nonsecure bank of banked registers; note that this is not
>   * the same thing as the current security state of the processor!
>   */
> -FIELD(TBFLAG_A32, NS, 6, 1)
> -FIELD(TBFLAG_A32, VFPEN, 7, 1)  /* Partially cached, minus FPEXC. */
> -FIELD(TBFLAG_A32, CONDEXEC, 8, 8)   /* Not cached. */
> -FIELD(TBFLAG_A32, SCTLR_B, 16, 1)
> -/* For M profile only, set if FPCCR.LSPACT is set */
> -FIELD(TBFLAG_A32, LSPACT, 18, 1)/* Not cached. */
> -/* For M profile only, set if we must create a new FP context */
> -FIELD(TBFLAG_A32, NEW_FP_CTXT_NEEDED, 19, 1) /* Not cached. */
> -/* For M profile only, set if FPCCR.S does not match current security state 
> */
> -FIELD(TBFLAG_A32, FPCCR_S_WRONG, 20, 1) /* Not cached. */
> -/* For M profile only, Handler (ie not Thread) mode */
> -FIELD(TBFLAG_A32, HANDLER, 21, 1)
> -/* For M profile only, whether we should generate stack-limit checks */
> -FIELD(TBFLAG_A32, STACKCHECK, 22, 1)
> +FIELD(TBFLAG_A32, NS, 16, 1)
>  
> -/* Bit usage when in AArch64 state */
> +/*
> + * Bit usage when in AArch32 state, for M-profile only.
> + */
> +/* Handler (ie not Thread) mode */
> +FIELD(TBFLAG_M32, HANDLER, 9, 1)
> +/* Whether we should generate stack-limit checks */
> +FIELD(TBFLAG_M32, STACKCHECK, 10, 1)
> +/* Set if FPCCR.LSPACT is set */
> +FIELD(TBFLAG_M32, LSPACT, 11, 1) /* Not cached. */
> +/* Set if we must create a new FP context */
> +FIELD(TBFLAG_M32, NEW_FP_CTXT_NEEDED, 12, 1) /* Not cached. */
> +/* Set if FPCCR.S does not match current security state */
> +FIELD(TBFLAG_M32, FPCCR_S_WRONG, 13, 1)  /* Not cached. */
> +
> +/*
> + * Bit usage when in AArch64 state
> + */
>  FIELD(TBFLAG_A64, TBII, 0, 2)
>  FIELD(TBFLAG_A64, SVEEXC_EL, 2, 2)
>  FIELD(TBFLAG_A64, ZCR_LEN, 4, 4)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 5172843667..ec5c7fa325 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -11207,11 +11207,8 @@ static uint32_t rebuild_hflags_m32(CPUARMState *env, 
> int fp_el,
>  {
>  uint32_t flags = 0;
>  
> -/* v8M always enables the fpu.  */
> -flags = FIELD_DP32(flags, TBFLAG_A32, VFPEN, 1);
> -
>  if (arm_v7m_is_handler_mode(env)) {
> -flags = FIELD_DP32(flags, TBFLAG_A32, HANDLER, 1);
> +flags = FIELD_DP32(flags, TBFLAG_M32, HANDLER, 1);
>  }
>  
>  /*
> @@ -11222,7 +11219,7 @@ static uint32_t rebuild_hflags_m32(CPUARMState *env, 
> int fp_el,
>  if (arm_feature(env, ARM_FEATURE_V8) &&
>  

Re: [PATCH v4 15/40] target/arm: Expand TBFLAG_ANY.MMUIDX to 4 bits

[Alex Bennée]

Richard Henderson  writes:

> We are about to expand the number of mmuidx to 10, and so need 4 bits.
> For the benefit of reading the number out of -d exec, align it to the
> penultimate nibble.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index ae9fc1ded3..5f295c7e60 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -3176,17 +3176,17 @@ typedef ARMCPU ArchCPU;
>   * Unless otherwise noted, these bits are cached in env->hflags.
>   */
>  FIELD(TBFLAG_ANY, AARCH64_STATE, 31, 1)
> -FIELD(TBFLAG_ANY, MMUIDX, 28, 3)
> -FIELD(TBFLAG_ANY, SS_ACTIVE, 27, 1)
> -FIELD(TBFLAG_ANY, PSTATE_SS, 26, 1) /* Not cached. */
> +FIELD(TBFLAG_ANY, SS_ACTIVE, 30, 1)
> +FIELD(TBFLAG_ANY, PSTATE_SS, 29, 1) /* Not cached. */
> +FIELD(TBFLAG_ANY, BE_DATA, 28, 1)
> +FIELD(TBFLAG_ANY, MMUIDX, 24, 4)
>  /* Target EL if we take a floating-point-disabled exception */
> -FIELD(TBFLAG_ANY, FPEXC_EL, 24, 2)
> -FIELD(TBFLAG_ANY, BE_DATA, 23, 1)
> +FIELD(TBFLAG_ANY, FPEXC_EL, 22, 2)
>  /*
>   * For A-profile only, target EL for debug exceptions.
>   * Note that this overlaps with the M-profile-only HANDLER and STACKCHECK 
> bits.
>   */
> -FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 21, 2)
> +FIELD(TBFLAG_ANY, DEBUG_TARGET_EL, 20, 2)
>  
>  /*
>   * Bit usage when in AArch32 state, both A- and M-profile.


-- 
Alex Bennée

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Janosch Frank]

On 12/4/19 12:34 PM, Thomas Huth wrote:
> On 04/12/2019 12.32, Janosch Frank wrote:
>> On 12/4/19 11:48 AM, Thomas Huth wrote:
>>> On 29/11/2019 10.47, Janosch Frank wrote:
 When a guest has saved a ipib of type 5 and call diagnose308 with
 subcode 10, we have to setup the protected processing environment via
 Ultravisor calls. The calls are done by KVM and are exposed via an API.

 The following steps are necessary:
 1. Create a VM (register it with the Ultravisor)
 2. Create secure CPUs for all of our current cpus
 3. Forward the secure header to the Ultravisor (has all information on
 how to decrypt the image and VM information)
 4. Protect image pages from the host and decrypt them
 5. Verify the image integrity

 Only after step 5 a protected VM is allowed to run.

 Signed-off-by: Janosch Frank 
 ---
>>> [...]
 +++ b/hw/s390x/pv.c
 @@ -0,0 +1,118 @@
 +/*
 + * Secure execution functions
 + *
 + * Copyright IBM Corp. 2019
 + * Author(s):
 + *  Janosch Frank 
 + *
 + * This work is licensed under the terms of the GNU GPL, version 2 or (at
 + * your option) any later version. See the COPYING file in the top-level
 + * directory.
 + */
 +#include "qemu/osdep.h"
 +#include 
 +
 +#include 
 +
 +#include "qemu/error-report.h"
 +#include "sysemu/kvm.h"
 +#include "pv.h"
 +
 +static int s390_pv_cmd(uint32_t cmd, void *data)
 +{
 +int rc;
 +struct kvm_pv_cmd pv_cmd = {
 +.cmd = cmd,
 +.data = (uint64_t)data,
 +};
 +
 +rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
 +if (rc) {
 +error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
 +exit(1);
 +}
 +return rc;
 +}
 +
 +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
 +{
 +int rc;
 +struct kvm_pv_cmd pv_cmd = {
 +.cmd = cmd,
 +.data = (uint64_t)data,
 +};
 +
 +rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
 +if (rc) {
 +error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, 
 rc);
 +exit(1);
 +}
 +return rc;
 +}
 +
 +int s390_pv_vm_create(void)
 +{
 +return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
 +}
 +
 +int s390_pv_vm_destroy(void)
 +{
 +return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
 +}
 +
 +int s390_pv_vcpu_create(CPUState *cs)
 +{
 +return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
 +}
 +
 +int s390_pv_vcpu_destroy(CPUState *cs)
 +{
 +S390CPU *cpu = S390_CPU(cs);
 +CPUS390XState *env = &cpu->env;
 +int rc;
 +
 +rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
 +if (!rc) {
 +env->pv = false;
 +}
 +return rc;
 +}
 +
 +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
 +{
 +struct kvm_s390_pv_sec_parm args = {
 +.origin = origin,
 +.length = length,
 +};
 +
 +return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
 +}
 +
 +/*
 + * Called for each component in the SE type IPL parameter block 0.
 + */
 +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
 +{
 +struct kvm_s390_pv_unp args = {
 +.addr = addr,
 +.size = size,
 +.tweak = tweak,
 +};
 +
 +return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
 +}
 +
 +int s390_pv_perf_clear_reset(void)
 +{
 +return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
 +}
 +
 +int s390_pv_verify(void)
 +{
 +return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
 +}
 +
 +int s390_pv_unshare(void)
 +{
 +return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
 +}
 diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
 new file mode 100644
 index 00..eb074e4bc9
 --- /dev/null
 +++ b/hw/s390x/pv.h
 @@ -0,0 +1,26 @@
 +/*
 + * Protected Virtualization header
 + *
 + * Copyright IBM Corp. 2019
 + * Author(s):
 + *  Janosch Frank 
 + *
 + * This work is licensed under the terms of the GNU GPL, version 2 or (at
 + * your option) any later version. See the COPYING file in the top-level
 + * directory.
 + */
 +
 +#ifndef HW_S390_PV_H
 +#define HW_S390_PV_H
 +
 +int s390_pv_vm_create(void);
 +int s390_pv_vm_destroy(void);
 +int s390_pv_vcpu_destroy(CPUState *cs);
 +int s390_pv_vcpu_create(CPUState *cs);
 +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
 +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
 +int s390_pv_perf_clear_reset(void);
 +int s390_pv_verify(void);
>

Re: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191204085628.2892-1-h...@redhat.com/



Hi,

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

Subject: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"
Type: series
Message-id: 20191204085628.2892-1-h...@redhat.com

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
6803338 Revert "qemu-options.hx: Update for reboot-timeout parameter"

=== OUTPUT BEGIN ===
ERROR: Missing Signed-off-by: line(s)

total: 1 errors, 0 warnings, 10 lines checked

Commit 680333822026 (Revert "qemu-options.hx: Update for reboot-timeout 
parameter") has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH] target/i386: relax assert when old host kernels don't include msrs

[Paolo Bonzini]

On 04/12/19 09:50, Catherine Ho wrote:
> Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
> add vmx msr entry although older host kernels don't include them.
> 
> But old host kernel + newest qemu will cause a qemu crash as follows:
> qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
> target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
> cpu->kvm_msr_buf->nmsrs' failed.
> 
> This fixes it by relaxing the condition.

This is intentional.  The VMX MSR entries should not have been added.
What combination of host kernel/QEMU are you using, and what QEMU
command line?

Paolo

Re: [PATCH v4 16/40] target/arm: Rearrange ARMMMUIdxBit

[Alex Bennée]

Richard Henderson  writes:

> Define via macro expansion, so that renumbering of the base ARMMMUIdx
> symbols is automatically reflexed in the bit definitions.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu.h | 39 +++
>  1 file changed, 23 insertions(+), 16 deletions(-)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 5f295c7e60..6ba5126852 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2886,27 +2886,34 @@ typedef enum ARMMMUIdx {
>  ARMMMUIdx_Stage1_E1 = 1 | ARM_MMU_IDX_NOTLB,
>  } ARMMMUIdx;
>  
> -/* Bit macros for the core-mmu-index values for each index,
> +/*
> + * Bit macros for the core-mmu-index values for each index,
>   * for use when calling tlb_flush_by_mmuidx() and friends.
>   */
> +#define TO_CORE_BIT(NAME) \
> +ARMMMUIdxBit_##NAME = 1 << (ARMMMUIdx_##NAME & ARM_MMU_IDX_COREIDX_MASK)
> +
>  typedef enum ARMMMUIdxBit {
> -ARMMMUIdxBit_EL10_0 = 1 << 0,
> -ARMMMUIdxBit_EL10_1 = 1 << 1,
> -ARMMMUIdxBit_E2 = 1 << 2,
> -ARMMMUIdxBit_SE3 = 1 << 3,
> -ARMMMUIdxBit_SE0 = 1 << 4,
> -ARMMMUIdxBit_SE1 = 1 << 5,
> -ARMMMUIdxBit_Stage2 = 1 << 6,
> -ARMMMUIdxBit_MUser = 1 << 0,
> -ARMMMUIdxBit_MPriv = 1 << 1,
> -ARMMMUIdxBit_MUserNegPri = 1 << 2,
> -ARMMMUIdxBit_MPrivNegPri = 1 << 3,
> -ARMMMUIdxBit_MSUser = 1 << 4,
> -ARMMMUIdxBit_MSPriv = 1 << 5,
> -ARMMMUIdxBit_MSUserNegPri = 1 << 6,
> -ARMMMUIdxBit_MSPrivNegPri = 1 << 7,
> +TO_CORE_BIT(EL10_0),
> +TO_CORE_BIT(EL10_1),
> +TO_CORE_BIT(E2),
> +TO_CORE_BIT(SE0),
> +TO_CORE_BIT(SE1),
> +TO_CORE_BIT(SE3),
> +TO_CORE_BIT(Stage2),
> +
> +TO_CORE_BIT(MUser),
> +TO_CORE_BIT(MPriv),
> +TO_CORE_BIT(MUserNegPri),
> +TO_CORE_BIT(MPrivNegPri),
> +TO_CORE_BIT(MSUser),
> +TO_CORE_BIT(MSPriv),
> +TO_CORE_BIT(MSUserNegPri),
> +TO_CORE_BIT(MSPrivNegPri),
>  } ARMMMUIdxBit;
>  
> +#undef TO_CORE_BIT
> +
>  #define MMU_USER_IDX 0
>  
>  static inline int arm_to_core_mmu_idx(ARMMMUIdx mmu_idx)


-- 
Alex Bennée

Re: [PATCH v2 03/13] s390x: protvirt: Support unpack facility

[Janosch Frank]

On 12/4/19 11:48 AM, Thomas Huth wrote:
> On 29/11/2019 10.47, Janosch Frank wrote:
>> When a guest has saved a ipib of type 5 and call diagnose308 with
>> subcode 10, we have to setup the protected processing environment via
>> Ultravisor calls. The calls are done by KVM and are exposed via an API.
>>
>> The following steps are necessary:
>> 1. Create a VM (register it with the Ultravisor)
>> 2. Create secure CPUs for all of our current cpus
>> 3. Forward the secure header to the Ultravisor (has all information on
>> how to decrypt the image and VM information)
>> 4. Protect image pages from the host and decrypt them
>> 5. Verify the image integrity
>>
>> Only after step 5 a protected VM is allowed to run.
>>
>> Signed-off-by: Janosch Frank 
>> ---
> [...]
>> +++ b/hw/s390x/pv.c
>> @@ -0,0 +1,118 @@
>> +/*
>> + * Secure execution functions
>> + *
>> + * Copyright IBM Corp. 2019
>> + * Author(s):
>> + *  Janosch Frank 
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +#include "qemu/osdep.h"
>> +#include 
>> +
>> +#include 
>> +
>> +#include "qemu/error-report.h"
>> +#include "sysemu/kvm.h"
>> +#include "pv.h"
>> +
>> +static int s390_pv_cmd(uint32_t cmd, void *data)
>> +{
>> +int rc;
>> +struct kvm_pv_cmd pv_cmd = {
>> +.cmd = cmd,
>> +.data = (uint64_t)data,
>> +};
>> +
>> +rc = kvm_vm_ioctl(kvm_state, KVM_S390_PV_COMMAND, &pv_cmd);
>> +if (rc) {
>> +error_report("KVM PV command failed cmd: %d rc: %d", cmd, rc);
>> +exit(1);
>> +}
>> +return rc;
>> +}
>> +
>> +static int s390_pv_cmd_vcpu(CPUState *cs, uint32_t cmd, void *data)
>> +{
>> +int rc;
>> +struct kvm_pv_cmd pv_cmd = {
>> +.cmd = cmd,
>> +.data = (uint64_t)data,
>> +};
>> +
>> +rc = kvm_vcpu_ioctl(cs, KVM_S390_PV_COMMAND_VCPU, &pv_cmd);
>> +if (rc) {
>> +error_report("KVM PV VCPU command failed cmd: %d rc: %d", cmd, rc);
>> +exit(1);
>> +}
>> +return rc;
>> +}
>> +
>> +int s390_pv_vm_create(void)
>> +{
>> +return s390_pv_cmd(KVM_PV_VM_CREATE, NULL);
>> +}
>> +
>> +int s390_pv_vm_destroy(void)
>> +{
>> +return s390_pv_cmd(KVM_PV_VM_DESTROY, NULL);
>> +}
>> +
>> +int s390_pv_vcpu_create(CPUState *cs)
>> +{
>> +return s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_CREATE, NULL);
>> +}
>> +
>> +int s390_pv_vcpu_destroy(CPUState *cs)
>> +{
>> +S390CPU *cpu = S390_CPU(cs);
>> +CPUS390XState *env = &cpu->env;
>> +int rc;
>> +
>> +rc = s390_pv_cmd_vcpu(cs, KVM_PV_VCPU_DESTROY, NULL);
>> +if (!rc) {
>> +env->pv = false;
>> +}
>> +return rc;
>> +}
>> +
>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length)
>> +{
>> +struct kvm_s390_pv_sec_parm args = {
>> +.origin = origin,
>> +.length = length,
>> +};
>> +
>> +return s390_pv_cmd(KVM_PV_VM_SET_SEC_PARMS, &args);
>> +}
>> +
>> +/*
>> + * Called for each component in the SE type IPL parameter block 0.
>> + */
>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak)
>> +{
>> +struct kvm_s390_pv_unp args = {
>> +.addr = addr,
>> +.size = size,
>> +.tweak = tweak,
>> +};
>> +
>> +return s390_pv_cmd(KVM_PV_VM_UNPACK, &args);
>> +}
>> +
>> +int s390_pv_perf_clear_reset(void)
>> +{
>> +return s390_pv_cmd(KVM_PV_VM_PERF_CLEAR_RESET, NULL);
>> +}
>> +
>> +int s390_pv_verify(void)
>> +{
>> +return s390_pv_cmd(KVM_PV_VM_VERIFY, NULL);
>> +}
>> +
>> +int s390_pv_unshare(void)
>> +{
>> +return s390_pv_cmd(KVM_PV_VM_UNSHARE, NULL);
>> +}
>> diff --git a/hw/s390x/pv.h b/hw/s390x/pv.h
>> new file mode 100644
>> index 00..eb074e4bc9
>> --- /dev/null
>> +++ b/hw/s390x/pv.h
>> @@ -0,0 +1,26 @@
>> +/*
>> + * Protected Virtualization header
>> + *
>> + * Copyright IBM Corp. 2019
>> + * Author(s):
>> + *  Janosch Frank 
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or (at
>> + * your option) any later version. See the COPYING file in the top-level
>> + * directory.
>> + */
>> +
>> +#ifndef HW_S390_PV_H
>> +#define HW_S390_PV_H
>> +
>> +int s390_pv_vm_create(void);
>> +int s390_pv_vm_destroy(void);
>> +int s390_pv_vcpu_destroy(CPUState *cs);
>> +int s390_pv_vcpu_create(CPUState *cs);
>> +int s390_pv_set_sec_parms(uint64_t origin, uint64_t length);
>> +int s390_pv_unpack(uint64_t addr, uint64_t size, uint64_t tweak);
>> +int s390_pv_perf_clear_reset(void);
>> +int s390_pv_verify(void);
>> +int s390_pv_unshare(void);
> 
> I still think you should make all those functions returning "void"
> instead of "int" - since errors results in an exit() in s390_pv_cmd()
> and s390_pv_cmd_vcpu() anyway...

Hey Thomas,

I have requested an error code for diag 308 subcode 10 that tells the
VM, that we didn't succeed starting a secure guest. Once that is in
place, I'll need to check the return codes.

Although I'm a bit un

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Thomas Huth]

On 29/11/2019 10.48, Janosch Frank wrote:
> CPU resets for protected guests need to be done via Ultravisor
> calls. Hence we need a way to issue these calls for each reset.
> 
> As we formerly had only one reset function and it was called for
> initial, as well as for the clear reset, we now need a new interface.
> 
> Signed-off-by: Janosch Frank 
> ---
[...]
> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
> index b802d02ff5..5b1ed3acb4 100644
> --- a/target/s390x/kvm.c
> +++ b/target/s390x/kvm.c
> @@ -154,6 +154,7 @@ static int cap_ri;
>  static int cap_gs;
>  static int cap_hpage_1m;
>  static int cap_protvirt;
> +static int cap_vcpu_resets;
>  
>  static int active_cmma;
>  
> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>  cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>  cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>  cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
> +cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>  
>  if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>  || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>  return 0;
>  }
>  
> -void kvm_s390_reset_vcpu(S390CPU *cpu)
> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>  {
>  CPUState *cs = CPU(cpu);
>  
> -/* The initial reset call is needed here to reset in-kernel
> - * vcpu data that we can't access directly from QEMU
> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
> - * Before this ioctl cpu_synchronize_state() is called in common kvm
> - * code (kvm-all) */
> +/*
> + * The reset call is needed here to reset in-kernel vcpu data that
> + * we can't access directly from QEMU (i.e. with older kernels
> + * which don't support sync_regs/ONE_REG).  Before this ioctl
> + * cpu_synchronize_state() is called in common kvm code
> + * (kvm-all).
> + */
> +if (cap_vcpu_resets) {
> +if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
> +error_report("CPU reset type %ld failed on CPU %i",
> + type, cs->cpu_index);
> +}
> +return;
> +}
>  if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>  error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>  }

Don't you want to check for type != KVM_S390_VCPU_RESET_NORMAL before
doing the INITIAL_RESET here?

>  }
>  
> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
> +{
> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
> +}
> +
> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
> +{
> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
> +}
> +
> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
> +{
> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
> +}

... otherwise this might reset more things than expected?

Or do I miss something here?

 Thomas

Re: virtiofsd: Where should it live?

[Dr. David Alan Gilbert]

* Markus Armbruster (arm...@redhat.com) wrote:
> Daniel P. Berrangé  writes:
> 
> > On Tue, Dec 03, 2019 at 11:06:44AM +, Peter Maydell wrote:
> >> On Tue, 3 Dec 2019 at 10:53, Dr. David Alan Gilbert  
> >> wrote:
> >> >
> >> > We seem to be coming to the conclusion something that:
> >> >
> >> >   a) It should live in the qemu tree
> >> >   b) It shouldn't live under contrib
> >> >   c) We'll create a new top level, i.e. 'daemons'
> >> >   d) virtiofsd will be daemons/virtiofsd
> >> >
> >> > Now, somethings I'm less clear on:
> >> >   e) What else would move into daemons?  It was suggested
> >> > that if we've got virtiofsd in there, then we should move
> >> > libvhost-user - which I understand, but then it's not a
> >> > 'daemons'.
> >> > Are there any otehr daemons that should move?
> >> 
> >> I like the idea of a new top level directory, but I think
> >> 'daemons' is a bit too specific -- for instance it seems to
> >> me that qemu-img would be sensible to move out of the root,
> >> and that's not a daemon.
> >
> > Do we really need an extra directory level ?
> 
> +1
> 
> > IIUC, the main point against having $GIT_ROOT/virtiofsd is that
> > the root of our repo is quite cluttered already.
> >
> > Rather than trying to create a multi-level hierarchy which adds
> > a debate around naming, why not address the clutter by moving
> > *ALL* the .c/.h files out of the root so that we have a flatter
> > tree:
> >
> >   $GITROOT
> > +- qemu-system
> > |   +- vl.c
> > |   +- ...most other files...
> 
> Sounds good to me.
> 
> > +- qemu-img
> > |   +- qemu-img.c
> 
> Perhaps this one can all go into existing block/, similar to how
> pr-manager-helper.c is in scsi/, and virtfs-proxy-helper.c is in fsdev/.
> Up to the block maintainers, of course.
> 
> > +- qemu-nbd
> > |   +- qemu-nbd.c
> 
> block/ or nbd/?
> 
> > +- qemu-io
> > |   +- qemu-io.c
> > |   +- qemu-io-cmds.c
> 
> block/?
> 
> > +- qemu-bridge-helper
> 
> net/?
> 
> > |   ...
> > +- qemu-edid
> 
> Has its own MAINTAINERS section, together with hw/display/edit* and
> include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
> idea.  Gerd?
> 
> > +- qemu-keymap
> 
> Not covered by MAINTAINERS.  scripts/get_maintainer.pl --git-blame
> points to Gerd.
> 
> > +- qga  (already exists)
> 
> Yes.
> 
> > Then we can add virtiofsd and other programs at the root with no big
> > issue.
> 
> We don't *have* to put each program into its own directory.  Simple ones
> could also share one.  We just need a directory name.

So what do you think of Paolo's suggestion of putting virtiofsd in 
fsdev (mkdir fsdev/9p && mv fsdev/* fsdev/9p && mkdir fsdev/virtiofsd )

Dave

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

Re: [PATCH v6 3/9] qdev: add clock input&output support to devices.

[Damien Hedde]

On 12/4/19 10:53 AM, Philippe Mathieu-Daudé wrote:
> On 12/4/19 10:05 AM, Damien Hedde wrote:
>> On 12/2/19 3:34 PM, Peter Maydell wrote:
>>> On Wed, 4 Sep 2019 at 13:56, Damien Hedde
>>>  wrote:

> [...]
 +/**
 + * qdev_pass_clock:
 + * @dev: the device to forward the clock to
 + * @name: the name of the clock to be added (can't be NULL)
 + * @container: the device which already has the clock
 + * @cont_name: the name of the clock in the container device
 + *
 + * Add a clock @name to @dev which forward to the clock @cont_name
 in @container
 + */
>>>
>>> 'container' seems odd terminology here, because I would expect
>>> the usual use of this function to be when a 'container' object
>>> like an SoC wants to forward a clock to one of its components;
>>> in that case the 'container' SoC would be @dev, wouldn't it?
>>
>> Yes. I agree it is confusing.
>> This function just allow a a device 'A' to exhibit a clock from another
>> device 'B' (typically a composing sub-device, inside a soc like you
>> said). The device A is not supposed to interact with the clock itself.
>> The original sub-device is the true
>> owner/controller of the clock and is the only one which should use the
>> clock API: setting a callback on it (input clock); or updating the clock
>> frequency (output clock).
>> Basically, it just add the clock into the device clock namespace without
>> interfering with it.
>>
>>> We should get this to be the same way round as qdev_pass_gpios(),
>>> which takes "DeviceState *dev, DeviceState *container", and
>>> passes the gpios that exist on 'dev' over to 'container' so that
>>> 'container' now has gpios which it did not before.
>>
>> Ok, I'll invert the arguments.
>>
>>>
>>> Also, your use of 'forward to' is inconsistent: in the 'dev'
>>> documentation you say we're forwarding the clock to 'dev',
>>> but in the body of the documentation you say we're forwarding
>>> the clock to the clock in 'container'.
>>
>> I'll try to clarify this and make the documentation more consistent with
>> the comments here.
>>
>>>
>>> I think the way to resolve this is to stick to the terminology
>>> in the function name itself:
>>>   @dev: the device which has the clock
>>>   @name: the name of the clock on @dev
>>>   @container: the name of the device which the clock should
>>>    be passed to
>>>   @cont_name: the name to use for the clock on @container
>>
>> I think container is confusing because depending on how we reason (clock
>> in a device; device in another device), we end up thinking the opposite.
>>
>> Maybe we can use "exhibit" instead of "container" in the 2nd pair of
>> parameters, or prefix use "origin" or "owner" as a prefix for the first
>> pair of parameters.
> 
> @sink vs @source?
> 
>>> Q: if you pass a clock to another device with this function,
>>> does it still exist to be used directly on the original
>>> device? For qdev_pass_gpios it does not (I think), but
>>> this is more accident of implementation than anything else.
>>
>> It depends what we mean by "used by".
>> Original device can:
>> + set the callback in case it is an input
>> + update the frequency in case it is an output
> 
> Hmm here you use @input vs @output...

Not really. What I meant here is that the device which originally
creates the clock is the only device which can interact with the clock.
And it will never gives this right to another device even if
qdev_pass_clock() is used.

There are 2 interactions, depending on the clock direction (input or
output). If it is an input clock, it can register a callback on
frequency changes; and if it is an output clock, it can updates the
frequency of the clock.

> 
>> But since an input clock can only be connected once,
>> I think the logic here is that any connection should now go through the
>> new device. But this is not checked and using one or the other is
>> exactly the same.
>>
>> Maybe I misunderstood the meaning of qdev_pass_gpios().
> [...]
> 

Re: Custom logic gates on user space emulation

[burak sarac]

Hello Alex,
 Thank you for your response, if I am on the right path I want to add
hadamard or one of pauli gate to gnu assembler then I want to run this
extended GAS via qemu using user space emulation. i.e.
https://en.wikipedia.org/wiki/Quantum_logic_gate#Hadamard_(H)_gate.
The idea is there are many quantum computer emulators i.e. here
https://www.quantiki.org/wiki/list-qc-simulators , yet I couldnt find
any one of them uses qemu. For beginning I wanted to try to port
libquantum hadamard impl.  There is https://github.com/Qiskit/openqasm
but source is not present.

Burak

burak sarac  writes:

> Hello All,
>  Currently I am studying qemu and I want to figure out how I can use
> custom logic gates on user space emulation.  I am searching very basic
> 'hello world' kind of tutorial or some resources to i.e. adding left
> or LOR : 1 | 0 = 1 but 0 | 1 = 0 to existing x86 arch
> ((https://en.wikibooks.org/wiki/X86_Assembly/Logic) ?).

It's not clear what you want to do. Are you looking to extend an
existing instruction set with additional custom instructions? Can you
explain why you want to do this?

> What I want to
> try is run this extended x86 version with qemu user space emulation.
> Do I need a custom toolchain also for this? I found this book:
> https://subscription.packtpub.com/book/hardware_and_creative/9781783289455/1/ch01lvl1sec15/generating-a-custom-toolchain-become-an-expert

For testing you don't need a custom toolchain - you can use inline
assembly with data statements to insert your custom instructions into a
program. Again it depends on what your eventual aim is here.

>
> Sorry for my ignorance in case it is totally irrelevant and I would
> appreciate any guidance! Or pseudo kind of road map for me!
>
> Thank you & have a nice day
> Burak


-- 
Alex Bennée

NBD reconnect on open

[Vladimir Sementsov-Ogievskiy]

Hi Eric!

There is a question to discuss.

We need to make an option to allow nbd-reconnect loop on nbd-open.
For example, add optional nbd blockdev option open-reconnect-delay, to
make it possible to start qemu with specified nbd connection, when nbd
server is down, and make several tries to connect before starting the
guest.

So, we need it for nbd opened from commandline arguments, and this case
seems OK.

But adding option to QAPI, we also allow it for qmp blockdev-add, and
reconnecting in context of qmp command execution is a wrong thing..

I can add new option only to options in block/nbd.c, but this way
-blockdev command line option will not work, it needs QAPI definition.

What do you think about it?

I can detect somehow in nbd_open that we are in qmp monitor context, and
return error if open-reconnect-delay specified.. Is it OK? Is there a
way to do it?


-- 
Best regards,
Vladimir

Re: [PATCH v6 6/9] docs/clocks: add device's clock documentation

[Damien Hedde]

On 12/2/19 4:17 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Add the documentation about the clock inputs and outputs in devices.
>>
>> This is based on the original work of Frederic Konrad.
>>
>> Signed-off-by: Damien Hedde 
>> ---
>>  docs/devel/clock.txt | 246 +++
>>  1 file changed, 246 insertions(+)
>>  create mode 100644 docs/devel/clock.txt
> 
> Could you convert this to rst format, please?Yes.
> 
> 
> 
>> +Changing a clock output
>> +===
>> +
>> +A device can change its outputs using the clock_set_frequency function. It
>> +will trigger updates on every connected inputs.
> 
> "input"
> 
>> +
>> +For example, let's say that we have an output clock "clkout" and we have a
>> +pointer to it in the device state because we did the following in init 
>> phase:
>> +dev->clkout = qdev_init_clock_out(DEVICE(dev), "clkout");
>> +
>> +Then at any time (apart from the cases listed below), it is possible to
>> +change the clock value by doing:
>> +clock_set_frequency(dev->clkout, 1000 * 1000 * 1000); /* 1Ghz */
>> +This operation must be done while holding the qemu io lock.
>> +
>> +One can change clocks only when it is allowed to have side effects on other
>> +objects. In consequence, it is forbidden:
>> ++ during migration,
>> ++ and in the init phase of reset.
>> +
>> +Forwarding clocks
>> +=
>> +
>> +Sometimes, one needs to forward, or inherit, a clock from another device.
>> +Typically, when doing device composition, a device might expose a 
>> sub-device's
>> +clock without interfering with it.
>> +The function qdev_pass_clock() can be used to achieve this behaviour. Note, 
>> that
> 
> "Note that"
> 
>> +it is possible to expose the clock under a different name. This works for 
>> both
>> +inputs or outputs.
> 
> "inputs and outputs"
> 
> 
>> +Migration
>> +=
>> +
>> +Only the ClockIn object has a state. ClockOut is not concerned by migration.
> 
> "has any state".
> 
> "ClockOut has no state and does not need special handling for migration."
> 
>> +
>> +In case the frequency of in input clock is needed for a device's migration,
>> +this state must be migrated.
> 
> Are you trying to say that if an input clock is known to be a
> fixed frequency we don't need to migrate anything? I wonder
> if we need to worry about that or if we could/should just say that
> input clocks should always be migrated.

What I wanted to say is that there are indeed probably cases where
migrating the frequency is unnecessary. For example if we only use the
callback and never fetch the frequency outside it: if the frequency is
only used to compute something which is already saved/loaded during
migration.

But yes we could just do as you say. It's probably less confusing.

> 
>> The VMSTATE_CLOCKIN macro defines an entry to
>> +be added in a vmstate description.
>> +
>> +For example, if a device has a clock input and the device state looks like:
>> +MyDeviceState {
>> +DeviceState parent_obj;
>> +ClockIn *clk;
>> +};
>> +
>> +Then, to add the clock frequency to the device's migrated state, the vmstate
>> +description is:
>> +VMStateDescription my_device_vmstate = {
>> +.name = "my_device",
>> +.fields = (VMStateField[]) {
>> +VMSTATE_CLOCKIN(clk, MyDeviceState),
>> +VMSTATE_END_OF_LIST()
>> +}
>> +};
>> +
>> +When adding a input clock support to an existing device, you must care about
>> +migration compatibility. To this end, you can use the clock_init_frequency 
>> in
>> +a pre_load function to setup a default value in case the source vm does not
>> +migrate the frequency.
> 
> thanks
> -- PMM
> 

Re: [PATCH v2 12/13] s390x: protvirt: Disable address checks for PV guest IO emulation

[Thomas Huth]

On 29/11/2019 10.48, Janosch Frank wrote:
> IO instruction data is routed through SIDAD for protected guests, so
> adresses do not need to be checked, as this is kernel memory.
> 
> Signed-off-by: Janosch Frank 
> ---
>  target/s390x/ioinst.c | 26 +++---
>  1 file changed, 19 insertions(+), 7 deletions(-)

Reviewed-by: Thomas Huth 

Re: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[Markus Armbruster]

Han Han  writes:

> This reverts commit bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8.
>
> In 20a1922032 we allowed reboot-timeout=-1 again, so update the doc
> accordingly.
> ---
>  qemu-options.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 65c9473b73..e14d88e9b2 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -327,8 +327,8 @@ format(true color). The resolution should be supported by 
> the SVGA mode, so
>  the recommended is 320x240, 640x480, 800x640.
>  
>  A timeout could be passed to bios, guest will pause for @var{rb_timeout} ms
> -when boot failed, then reboot. If @option{reboot-timeout} is not set,
> -guest will not reboot by default. Currently Seabios for X86
> +when boot failed, then reboot. If @var{rb_timeout} is '-1', guest will not
> +reboot, qemu passes '-1' to bios by default. Currently Seabios for X86
>  system support it.
>  
>  Do strict boot via @option{strict=on} as far as firmware/BIOS

Reviewed-by: Markus Armbruster 

Re: [PATCH v2 08/13] s390x: protvirt: Add new VCPU reset functions

[Janosch Frank]

On 12/4/19 12:58 PM, Thomas Huth wrote:
> On 29/11/2019 10.48, Janosch Frank wrote:
>> CPU resets for protected guests need to be done via Ultravisor
>> calls. Hence we need a way to issue these calls for each reset.
>>
>> As we formerly had only one reset function and it was called for
>> initial, as well as for the clear reset, we now need a new interface.
>>
>> Signed-off-by: Janosch Frank 
>> ---
> [...]
>> diff --git a/target/s390x/kvm.c b/target/s390x/kvm.c
>> index b802d02ff5..5b1ed3acb4 100644
>> --- a/target/s390x/kvm.c
>> +++ b/target/s390x/kvm.c
>> @@ -154,6 +154,7 @@ static int cap_ri;
>>  static int cap_gs;
>>  static int cap_hpage_1m;
>>  static int cap_protvirt;
>> +static int cap_vcpu_resets;
>>  
>>  static int active_cmma;
>>  
>> @@ -346,6 +347,7 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>>  cap_mem_op = kvm_check_extension(s, KVM_CAP_S390_MEM_OP);
>>  cap_s390_irq = kvm_check_extension(s, KVM_CAP_S390_INJECT_IRQ);
>>  cap_protvirt = kvm_check_extension(s, KVM_CAP_S390_PROTECTED);
>> +cap_vcpu_resets = kvm_check_extension(s, KVM_CAP_S390_VCPU_RESETS);
>>  
>>  if (!kvm_check_extension(s, KVM_CAP_S390_GMAP)
>>  || !kvm_check_extension(s, KVM_CAP_S390_COW)) {
>> @@ -407,20 +409,44 @@ int kvm_arch_destroy_vcpu(CPUState *cs)
>>  return 0;
>>  }
>>  
>> -void kvm_s390_reset_vcpu(S390CPU *cpu)
>> +static void kvm_s390_reset_vcpu(S390CPU *cpu, unsigned long type)
>>  {
>>  CPUState *cs = CPU(cpu);
>>  
>> -/* The initial reset call is needed here to reset in-kernel
>> - * vcpu data that we can't access directly from QEMU
>> - * (i.e. with older kernels which don't support sync_regs/ONE_REG).
>> - * Before this ioctl cpu_synchronize_state() is called in common kvm
>> - * code (kvm-all) */
>> +/*
>> + * The reset call is needed here to reset in-kernel vcpu data that
>> + * we can't access directly from QEMU (i.e. with older kernels
>> + * which don't support sync_regs/ONE_REG).  Before this ioctl
>> + * cpu_synchronize_state() is called in common kvm code
>> + * (kvm-all).
>> + */
>> +if (cap_vcpu_resets) {
>> +if (kvm_vcpu_ioctl(cs, KVM_S390_VCPU_RESET, type)) {
>> +error_report("CPU reset type %ld failed on CPU %i",
>> + type, cs->cpu_index);
>> +}
>> +return;
>> +}
>>  if (kvm_vcpu_ioctl(cs, KVM_S390_INITIAL_RESET, NULL)) {
>>  error_report("Initial CPU reset failed on CPU %i", cs->cpu_index);
>>  }
> 
> Don't you want to check for type != KVM_S390_VCPU_RESET_NORMAL before
> doing the INITIAL_RESET here?
> 
>>  }
>>  
>> +void kvm_s390_reset_vcpu_initial(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_INITIAL);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_clear(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_CLEAR);
>> +}
>> +
>> +void kvm_s390_reset_vcpu_normal(S390CPU *cpu)
>> +{
>> +kvm_s390_reset_vcpu(cpu, KVM_S390_VCPU_RESET_NORMAL);
>> +}
> 
> ... otherwise this might reset more things than expected?
> 
> Or do I miss something here?

Hey Thomas I split out this patch into the "architectural compliance"
patch series. Have a look into there, this is an old version.

> 
>  Thomas
> 




signature.asc
Description: OpenPGP digital signature

Re: [PATCH] Revert "qemu-options.hx: Update for reboot-timeout parameter"

[Dr. David Alan Gilbert]

* Han Han (h...@redhat.com) wrote:
> This reverts commit bbd9e6985ff342cbe15b9cb7eb30e842796fbbe8.

Patchew spotted you're missing the signed-off-by; please send one.

Dave

> In 20a1922032 we allowed reboot-timeout=-1 again, so update the doc
> accordingly.
> ---
>  qemu-options.hx | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 65c9473b73..e14d88e9b2 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -327,8 +327,8 @@ format(true color). The resolution should be supported by 
> the SVGA mode, so
>  the recommended is 320x240, 640x480, 800x640.
>  
>  A timeout could be passed to bios, guest will pause for @var{rb_timeout} ms
> -when boot failed, then reboot. If @option{reboot-timeout} is not set,
> -guest will not reboot by default. Currently Seabios for X86
> +when boot failed, then reboot. If @var{rb_timeout} is '-1', guest will not
> +reboot, qemu passes '-1' to bios by default. Currently Seabios for X86
>  system support it.
>  
>  Do strict boot via @option{strict=on} as far as firmware/BIOS
> -- 
> 2.24.0.rc1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v6 7/9] hw/misc/zynq_slcr: add clock generation for uarts

[Damien Hedde]

On 12/2/19 4:20 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Switch the slcr to multi-phase reset and add some clocks:
>> + the main input clock (ps_clk)
>> + the reference clock outputs for each uart (uart0 & 1)
>>
>> The clock frequencies are computed using the internal pll & uart 
>> configuration
>> registers and the ps_clk frequency.
>>
>> Signed-off-by: Damien Hedde 
> 
> Review of this and the following two patches by some Xilinx
> person would be nice. I've just looked them over for general
> issues, and haven't checked against the hardware specs.
> 
>> ---
> 
> 
>> +/*
>> + * return the output frequency of a clock given:
>> + * + the frequencies in an array corresponding to mux's indexes
>> + * + the register xxx_CLK_CTRL value
>> + * + enable bit index in ctrl register
>> + *
>> + * This function make the assumption that ctrl_reg value is organized as 
>> follow:
> 
> "makes"; "that the"; "follows"
> 
>> + * + bits[13:8] clock divisor
>> + * + bits[5:4]  clock mux selector (index in array)
>> + * + bits[index] clock enable
>> + */
>> +static uint64_t zynq_slcr_compute_clock(const uint64_t mux[],
>> +uint32_t ctrl_reg,
>> +unsigned index)
>> +{
>> +uint32_t srcsel = extract32(ctrl_reg, 4, 2); /* bits [5:4] */
>> +uint32_t divisor = extract32(ctrl_reg, 8, 6); /* bits [13:8] */
>> +
>> +/* first, check if clock is enabled */
>> +if (((ctrl_reg >> index) & 1u) == 0) {
>> +return 0;
>> +}
>> +
>> +/*
>> + * according to the Zynq technical ref. manual UG585 v1.12.2 in
>> + * "Clocks" chapter, section 25.10.1 page 705" the range of the divisor
>> + * is [1;63].
> 
> Is this the range notation the spec doc uses?

The exact terms is:
"The 6-bit divider provides a divide range of 1 to 63"
At the time, I checked also the kernel sources, and this is the behavior
implemented in the driver as well (1 based timer and allowing 0 special
value for bypass). The bypass is undocumented as far as I can tell.

> 
>> + * So divide the source while avoiding division-by-zero.
>> + */
>> +return mux[srcsel] / (divisor ? divisor : 1u);
>> +}
>> +
> 
>> +static const ClockPortInitArray zynq_slcr_clocks = {
>> +QDEV_CLOCK_IN(ZynqSLCRState, ps_clk, zynq_slcr_ps_clk_callback),
>> +QDEV_CLOCK_OUT(ZynqSLCRState, uart0_ref_clk),
>> +QDEV_CLOCK_OUT(ZynqSLCRState, uart1_ref_clk),
>> +QDEV_CLOCK_END
>> +};
>> +
>>  static void zynq_slcr_init(Object *obj)
>>  {
>>  ZynqSLCRState *s = ZYNQ_SLCR(obj);
>> @@ -425,6 +559,8 @@ static void zynq_slcr_init(Object *obj)
>>  memory_region_init_io(&s->iomem, obj, &slcr_ops, s, "slcr",
>>ZYNQ_SLCR_MMIO_SIZE);
>>  sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->iomem);
>> +
>> +qdev_init_clocks(DEVICE(obj), zynq_slcr_clocks);
>>  }
>>
>>  static const VMStateDescription vmstate_zynq_slcr = {
>> @@ -440,9 +576,12 @@ static const VMStateDescription vmstate_zynq_slcr = {
>>  static void zynq_slcr_class_init(ObjectClass *klass, void *data)
>>  {
>>  DeviceClass *dc = DEVICE_CLASS(klass);
>> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>>
>>  dc->vmsd = &vmstate_zynq_slcr;
>> -dc->reset = zynq_slcr_reset;
>> +rc->phases.init = zynq_slcr_reset_init;
>> +rc->phases.hold = zynq_slcr_reset_hold;
>> +rc->phases.exit = zynq_slcr_reset_exit;
>>  }
> 
> We're adding an input clock, so doesn't the migration
> state struct need to be updated to migrate it ?
Yes, we can. Although this input clock is really not expected to change.

> 
> thanks
> -- PMM
> 

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Eric Blake]

On 12/4/19 12:56 AM, Markus Armbruster wrote:


+++ b/Makefile
@@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
 generated-files-y += $(GENERATED_QAPI_FILES)
   +GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
+GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
+GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
+GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
+GENERATED_QGA_FILES += qga-qapi-doc.texi
+GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, $(GENERATED_QGA_FILES))


Would it be worth using two separate variable names (maybe
GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
arcane knowledge that consecutive use of := causes GNU make to rewrite
an existing variable with new contents?


Our rules.mak relies on this already.  It's full of magic, which
admittedly diminishes its suitability to serve as a good example.

Your worry might be rooted in old "=" burns.  "=" makes the variable
recursively expanded, and

 GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, 
$(GENERATED_QGA_FILES))


Indeed, but I have to refer to the manual to remind myself of whether = 
or := is what I want in a given situation.




would be an infinite loop.  ":=" makes it simply expanded; there's not
even a loop, let alone an infinite one.  The GNU Make manual explains
this clearly at
https://www.gnu.org/software/make/manual/html_node/Flavors.html

Aside: there's a reason one of the two flavors is called "simple".  It
could additionally be called "not as slow".  One of my pet makefile
peeves: unthinking use of recursively expanded variables, complicating
semantics and slowing down builds.

Back to this patch.  I had started to write the thing in longhand, but
got tired of repeating qga/qapi-generated/, so I factored that out.
Would longhand be easier to understand?


It's more verbose.  My suggestion was more:

GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h
GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h
...
GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, 
$(GENERATED_QGA_BASENAMES))


to avoid the reassignment-to-self issue altogether, while still 
remaining concise compared to longhand.


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

Re: [PATCH v6] error: rename errp to errp_in where it is IN-argument

[Markus Armbruster]

Markus Armbruster  writes:

> Vladimir Sementsov-Ogievskiy  writes:
>
>> 29.11.2019 17:35, Markus Armbruster wrote:
[...]
>>> I pushed my fixups to git://repo.or.cz/qemu/armbru.git branch error-prep
>>> for your convenience.  The commit messages of the fixed up commits need
>>> rephrasing, of course.
>>> 
>>
>> Looked through fixups, looks OK for me, thanks! What next?
>
> Let me finish my fixing incorrect dereferences of errp, and then we
> figure out what to include in v7.

Your v6 with my fixups does not conflict with my "[PATCH v2 00/18] Error
handling fixes", except for "hw/core/loader-fit: fix freeing errp in
fit_load_fdt", which my "[PATCH v2 07/18] hw/core: Fix fit_load_fdt()
error handling violations" supersedes.

Suggest you work in the fixups and post as v7.  I'll merge that in my
tree, to give you a base for the remainder of your "auto propagated
local_err" work.  While you work on that, I'll work on getting the base
merged into master.  Sounds like a plan?

Re: virtiofsd: Where should it live?

[Markus Armbruster]

"Dr. David Alan Gilbert"  writes:

> So what do you think of Paolo's suggestion of putting virtiofsd in 
> fsdev (mkdir fsdev/9p && mv fsdev/* fsdev/9p && mkdir fsdev/virtiofsd )

No objections.

Flatter: fsdev-9p/ and fsdev-virtio/.  Matter of taste.

Re: [PATCH 7/6] Makefile: Make Makefile depend on generated qga files, too

[Markus Armbruster]

Eric Blake  writes:

> On 12/4/19 12:56 AM, Markus Armbruster wrote:
>
 +++ b/Makefile
 @@ -130,6 +130,15 @@ GENERATED_QAPI_FILES += qapi/qapi-doc.texi
  generated-files-y += $(GENERATED_QAPI_FILES)
+GENERATED_QGA_FILES := qga-qapi-types.c qga-qapi-types.h
 +GENERATED_QGA_FILES += qga-qapi-visit.c qga-qapi-visit.h
 +GENERATED_QGA_FILES += qga-qapi-commands.h qga-qapi-commands.c
 +GENERATED_QGA_FILES += qga-qapi-init-commands.h qga-qapi-init-commands.c
 +GENERATED_QGA_FILES += qga-qapi-doc.texi
 +GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/, 
 $(GENERATED_QGA_FILES))
>>>
>>> Would it be worth using two separate variable names (maybe
>>> GENERATED_QGA_BASEFILES for the first list) rather than exploiting the
>>> arcane knowledge that consecutive use of := causes GNU make to rewrite
>>> an existing variable with new contents?
>>
>> Our rules.mak relies on this already.  It's full of magic, which
>> admittedly diminishes its suitability to serve as a good example.
>>
>> Your worry might be rooted in old "=" burns.  "=" makes the variable
>> recursively expanded, and
>>
>>  GENERATED_QGA_FILES = $(addprefix qga/qapi-generated/, 
>> $(GENERATED_QGA_FILES))
>
> Indeed, but I have to refer to the manual to remind myself of whether
> = or := is what I want in a given situation.

Trust me, you're either sure you want "=", or you want ":=".

On a green field, I recommend a hard rule "no = without a comment
explaining why".

>> would be an infinite loop.  ":=" makes it simply expanded; there's not
>> even a loop, let alone an infinite one.  The GNU Make manual explains
>> this clearly at
>> https://www.gnu.org/software/make/manual/html_node/Flavors.html
>>
>> Aside: there's a reason one of the two flavors is called "simple".  It
>> could additionally be called "not as slow".  One of my pet makefile
>> peeves: unthinking use of recursively expanded variables, complicating
>> semantics and slowing down builds.
>>
>> Back to this patch.  I had started to write the thing in longhand, but
>> got tired of repeating qga/qapi-generated/, so I factored that out.
>> Would longhand be easier to understand?
>
> It's more verbose.  My suggestion was more:
>
> GENERATED_QGA_BASENAMES := qga-qapi-types.c qga-qapi-types.h
> GENERATED_QGA_BASENAMES += qga-qapi-visit.c qga-qapi-visit.h
> ...
> GENERATED_QGA_FILES := $(addprefix qga/qapi-generated/,
> $(GENERATED_QGA_BASENAMES))
>
> to avoid the reassignment-to-self issue altogether, while still
> remaining concise compared to longhand.

Either way, we use multiple assignments to build GENERATED_QGA_FILES.
The only difference is that the version using two variables would also
work with recursive expansion, due to the magic of +=.

Re: NBD reconnect on open

[Kevin Wolf]

Am 04.12.2019 um 13:18 hat Vladimir Sementsov-Ogievskiy geschrieben:
> There is a question to discuss.
> 
> We need to make an option to allow nbd-reconnect loop on nbd-open.
> For example, add optional nbd blockdev option open-reconnect-delay, to
> make it possible to start qemu with specified nbd connection, when nbd
> server is down, and make several tries to connect before starting the
> guest.
> 
> So, we need it for nbd opened from commandline arguments, and this case
> seems OK.
> 
> But adding option to QAPI, we also allow it for qmp blockdev-add, and
> reconnecting in context of qmp command execution is a wrong thing..
> 
> I can add new option only to options in block/nbd.c, but this way
> -blockdev command line option will not work, it needs QAPI definition.
> 
> What do you think about it?

I think there is a more general problem here actually. bdrv_open() is a
blocking operation and it shouldn't be. BlockDriver should probably have
a .bdrv_co_open instead, and I think this wouldn't be too hard to do.

However, that's only half of the solution: QMP still takes the BQL while
it's executing a command, so even if we used a coroutine, it would be of
no use if then the QMP command implementation would just call
BDRV_POLL_WHILE() to wait for the completion of the coroutine while
holding the BQL.

This is going in direction of async commands that Marc-André was working
on. I didn't follow this closely, so I'm not sure what the status there
is, but he and Markus should be able to tell more.

> I can detect somehow in nbd_open that we are in qmp monitor context, and
> return error if open-reconnect-delay specified.. Is it OK? Is there a
> way to do it?

The whole point of -blockdev is that it's a direct mapping of
blockdev-add to the command line, so making things behave differently
between them sounds like a bad idea.

Kevin

Re: virtiofsd: Where should it live?

[Kevin Wolf]

Am 04.12.2019 um 09:17 hat Gerd Hoffmann geschrieben:
>   Hi,
> 
> > > |   ...
> > > +- qemu-edid
> > 
> > Has its own MAINTAINERS section, together with hw/display/edit* and
> > include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
> > idea.  Gerd?
> 
> Sort-of makes sense.  My personal preference would be a tools/ directory
> for all those small utilities though.

I think I would like that better than throwing tools into block/ where
currently mostly just block drivers live.

Or, if we want to move the tools there, we'd need another directory
level inside block/ to keep things reasonably organised.

Kevin

Re: virtiofsd: Where should it live?

[Thomas Huth]

On 04/12/2019 14.28, Kevin Wolf wrote:
> Am 04.12.2019 um 09:17 hat Gerd Hoffmann geschrieben:
>>   Hi,
>>
 |   ...
 +- qemu-edid
>>>
>>> Has its own MAINTAINERS section, together with hw/display/edit* and
>>> include/hw/display/edid.h.  I'm not sure moving it hw/display/ is a good
>>> idea.  Gerd?
>>
>> Sort-of makes sense.  My personal preference would be a tools/ directory
>> for all those small utilities though.
> 
> I think I would like that better than throwing tools into block/ where
> currently mostly just block drivers live.

+1 for tools/

 Thomas

Re: [PATCH] target/i386: relax assert when old host kernels don't include msrs

[Catherine Ho]

Hi Paolo
[sorry to resend it, seems to reply it incorrectly]

On Wed, 4 Dec 2019 at 19:23, Paolo Bonzini  wrote:

> On 04/12/19 09:50, Catherine Ho wrote:
> > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
> > add vmx msr entry although older host kernels don't include them.
> >
> > But old host kernel + newest qemu will cause a qemu crash as follows:
> > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
> > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
> > cpu->kvm_msr_buf->nmsrs' failed.
> >
> > This fixes it by relaxing the condition.
>
> This is intentional.  The VMX MSR entries should not have been added.
> What combination of host kernel/QEMU are you using, and what QEMU
> command line?
>
>
> Host kernel: 4.15.0 (ubuntu 18.04)
Qemu: https://gitlab.com/virtio-fs/qemu/tree/virtio-fs-dev
cmdline: qemu-system-x86_64 -M pc -cpu host --enable-kvm -smp 8 \
  -m 4G,maxmem=4G

But before 20a78b02d315, the older kernel + latest qemu can boot guest
successfully.

Best Regards,
Catherine

Re: [PATCH v6 8/9] hw/char/cadence_uart: add clock support

[Damien Hedde]

On 12/2/19 4:24 PM, Peter Maydell wrote:
> On Wed, 4 Sep 2019 at 13:56, Damien Hedde  wrote:
>>
>> Switch the cadence uart to multi-phase reset and add the
>> reference clock input.
>>
>> The input clock frequency is added to the migration structure.
>>
>> The reference clock controls the baudrate generation. If it disabled,
>> any input characters and events are ignored.
>>
>> If this clock remains unconnected, the uart behaves as before
>> (it default to a 50MHz ref clock).
>>
>> Signed-off-by: Damien Hedde 
> 
>>  static void uart_parameters_setup(CadenceUARTState *s)
>>  {
>>  QEMUSerialSetParams ssp;
>> -unsigned int baud_rate, packet_size;
>> +unsigned int baud_rate, packet_size, input_clk;
>> +input_clk = clock_get_frequency(s->refclk);
>>
>> -baud_rate = (s->r[R_MR] & UART_MR_CLKS) ?
>> -UART_INPUT_CLK / 8 : UART_INPUT_CLK;
>> +baud_rate = (s->r[R_MR] & UART_MR_CLKS) ? input_clk / 8 : input_clk;
>> +baud_rate /= (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>> +trace_cadence_uart_baudrate(baud_rate);
>> +
>> +ssp.speed = baud_rate;
>>
>> -ssp.speed = baud_rate / (s->r[R_BRGR] * (s->r[R_BDIV] + 1));
>>  packet_size = 1;
>>
>>  switch (s->r[R_MR] & UART_MR_PAR) {
>> @@ -215,6 +220,13 @@ static void uart_parameters_setup(CadenceUARTState *s)
>>  }
>>
>>  packet_size += ssp.data_bits + ssp.stop_bits;
>> +if (ssp.speed == 0) {
>> +/*
>> + * Avoid division-by-zero below.
>> + * TODO: find something better
>> + */
> 
> Any ideas what might be better? :-)

Well maybe the comment is misplaced. Because it is probably a good thing
to round up the ssp.speed in case it becomes 0 (which is very unlikely
apart from the case where the input clock is 0/disabled).

The problem is what should we do when the clock is disabled ?
Right now we:
+ set a minimal baudrate
+ ignore input characters/events
+ still forward output characters... (I just checked)

I suppose we could at least fix the last point: we can drop any output
characters. But if this happen, there is definitely a problem somewhere
(a firmware should not try to send characters to an unclocked uart). Is
there a qemu way of reporting this kind of situation ?

It would be best to somehow tell the backend we're not handling anything
anymore. So I could put that in the comment instead.

I really don't know if/how we can do that. When I looked I did not see
any way to do the opposite of qemu_chr_fe_accept_input() which is done
to start receiving stuff.

--
Damien

Re: [PATCH v3] travis.yml: Run tcg tests with tci

[Richard Henderson]

On 12/4/19 12:31 AM, Thomas Huth wrote:
> -# We manually include builds which we disable "make check" for
> +# Check the TCG interpreter (TCI)
>  - env:
> -- CONFIG="--enable-debug --enable-tcg-interpreter"
> -- TEST_CMD=""
> +- CONFIG="--enable-debug --enable-tcg-interpreter --disable-kvm

While we're changing things, the interpreter will go much faster with
optimization enabled.  We can change this to --enable-debug-tcg, which leaves
the asserts enabled, but compiles with -O2.


r~

Re: [PATCH v4 18/40] target/arm: Reorganize ARMMMUIdx

[Alex Bennée]

Richard Henderson  writes:

> Prepare for, but do not yet implement, the EL2&0 regime.
> This involves adding the new MMUIdx enumerators and adjusting
> some of the MMUIdx related predicates to match.
>
> Signed-off-by: Richard Henderson 

Reviewed-by: Alex Bennée 

> ---
>  target/arm/cpu-param.h |   2 +-
>  target/arm/cpu.h   | 128 ++---
>  target/arm/internals.h |  37 +++-
>  target/arm/helper.c|  66 ++---
>  target/arm/translate.c |   1 -
>  5 files changed, 150 insertions(+), 84 deletions(-)
>
> diff --git a/target/arm/cpu-param.h b/target/arm/cpu-param.h
> index 6e6948e960..18ac562346 100644
> --- a/target/arm/cpu-param.h
> +++ b/target/arm/cpu-param.h
> @@ -29,6 +29,6 @@
>  # define TARGET_PAGE_BITS_MIN  10
>  #endif
>  
> -#define NB_MMU_MODES 8
> +#define NB_MMU_MODES 9
>  
>  #endif
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 015301e93a..bf8eb57e3a 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -2778,7 +2778,9 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   *  + NonSecure EL1 & 0 stage 1
>   *  + NonSecure EL1 & 0 stage 2
>   *  + NonSecure EL2
> - *  + Secure EL1 & EL0
> + *  + NonSecure EL2 & 0   (ARMv8.1-VHE)
> + *  + Secure EL0
> + *  + Secure EL1
>   *  + Secure EL3
>   * If EL3 is 32-bit:
>   *  + NonSecure PL1 & 0 stage 1
> @@ -2788,8 +2790,9 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   * (reminder: for 32 bit EL3, Secure PL1 is *EL3*, not EL1.)
>   *
>   * For QEMU, an mmu_idx is not quite the same as a translation regime 
> because:
> - *  1. we need to split the "EL1 & 0" regimes into two mmu_idxes, because 
> they
> - * may differ in access permissions even if the VA->PA map is the same
> + *  1. we need to split the "EL1 & 0" and "EL2 & 0" regimes into two 
> mmu_idxes,
> + * because they may differ in access permissions even if the VA->PA map 
> is
> + * the same
>   *  2. we want to cache in our TLB the full VA->IPA->PA lookup for a stage 
> 1+2
>   * translation, which means that we have one mmu_idx that deals with two
>   * concatenated translation regimes [this sort of combined s1+2 TLB is
> @@ -2801,19 +2804,23 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   *  4. we can also safely fold together the "32 bit EL3" and "64 bit EL3"
>   * translation regimes, because they map reasonably well to each other
>   * and they can't both be active at the same time.
> - * This gives us the following list of mmu_idx values:
> + *  5. we want to be able to use the TLB for accesses done as part of a
> + * stage1 page table walk, rather than having to walk the stage2 page
> + * table over and over.
>   *
> - * NS EL0 (aka NS PL0) stage 1+2
> - * NS EL1 (aka NS PL1) stage 1+2
> + * This gives us the following list of cases:
> + *
> + * NS EL0 (aka NS PL0) EL1&0 stage 1+2
> + * NS EL1 (aka NS PL1) EL1&0 stage 1+2
> + * NS EL0 EL2&0
> + * NS EL2 EL2&0
>   * NS EL2 (aka NS PL2)
> - * S EL3 (aka S PL1)
>   * S EL0 (aka S PL0)
>   * S EL1 (not used if EL3 is 32 bit)
> - * NS EL0+1 stage 2
> + * S EL3 (aka S PL1)
> + * NS EL0&1 stage 2
>   *
> - * (The last of these is an mmu_idx because we want to be able to use the TLB
> - * for the accesses done as part of a stage 1 page table walk, rather than
> - * having to walk the stage 2 page table over and over.)
> + * for a total of 9 different mmu_idx.
>   *
>   * R profile CPUs have an MPU, but can use the same set of MMU indexes
>   * as A profile. They only need to distinguish NS EL0 and NS EL1 (and
> @@ -2851,26 +2858,47 @@ static inline bool arm_excp_unmasked(CPUState *cs, 
> unsigned int excp_idx,
>   * For M profile we arrange them to have a bit for priv, a bit for negpri
>   * and a bit for secure.
>   */
> -#define ARM_MMU_IDX_A 0x10 /* A profile */
> -#define ARM_MMU_IDX_NOTLB 0x20 /* does not have a TLB */
> -#define ARM_MMU_IDX_M 0x40 /* M profile */
> +#define ARM_MMU_IDX_A 0x10  /* A profile */
> +#define ARM_MMU_IDX_NOTLB 0x20  /* does not have a TLB */
> +#define ARM_MMU_IDX_M 0x40  /* M profile */
>  
> -/* meanings of the bits for M profile mmu idx values */
> -#define ARM_MMU_IDX_M_PRIV 0x1
> +/* Meanings of the bits for M profile mmu idx values */
> +#define ARM_MMU_IDX_M_PRIV   0x1
>  #define ARM_MMU_IDX_M_NEGPRI 0x2
> -#define ARM_MMU_IDX_M_S 0x4
> +#define ARM_MMU_IDX_M_S  0x4  /* Secure */
>  
> -#define ARM_MMU_IDX_TYPE_MASK (~0x7)
> -#define ARM_MMU_IDX_COREIDX_MASK 0x7
> +#define ARM_MMU_IDX_TYPE_MASK \
> +(ARM_MMU_IDX_A | ARM_MMU_IDX_M | ARM_MMU_IDX_NOTLB)
> +#define ARM_MMU_IDX_COREIDX_MASK 0xf
>  
>  typedef enum ARMMMUIdx {
> +/*
> + * A-profile.
> + */
>  ARMMMUIdx_EL10_0 = 0 | ARM_MMU_IDX_A,
> -ARMMMUIdx_EL10_1 = 1 | ARM_MMU_IDX_A,
> -ARMMMUIdx_E2 = 2 | ARM_MMU_IDX_A,
> -ARMMMUIdx_SE3 = 3 | ARM_MMU_IDX_A,
> -ARMMMUIdx_SE0 = 4 | ARM_M

Re: [PATCH v3] travis.yml: Run tcg tests with tci

[Alex Bennée]

Thomas Huth  writes:

> So far we only have compile coverage for tci. But since commit
> 2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
> for INDEX_op_ld16u_i64") has been included now, we can also run the
> "tcg" and "qtest" tests with tci, so let's enable them in Travis now.
> Since we don't gain much additional test coverage by compiling all
> targets, and TCI is broken e.g. with the Sparc targets, we also limit
> the target list to a reasonable subset now (which should still get us
> test coverage by tests/boot-serial-test for example).

Queued to testing/next, thanks.
>
> Tested-by: Stefan Weil 
> Reviewed-by: Philippe Mathieu-Daudé 
> Signed-off-by: Thomas Huth 
> ---
>  v3:
>  - Add --disable-kvm option since we're only interested in TCG here
>
>  .travis.yml | 7 ---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 445b0646c1..d73e2fb744 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -215,10 +215,11 @@ matrix:
>  - TEST_CMD=""
>  
>  
> -# We manually include builds which we disable "make check" for
> +# Check the TCG interpreter (TCI)
>  - env:
> -- CONFIG="--enable-debug --enable-tcg-interpreter"
> -- TEST_CMD=""
> +- CONFIG="--enable-debug --enable-tcg-interpreter --disable-kvm 
> --disable-containers
> +
> --target-list=alpha-softmmu,arm-softmmu,hppa-softmmu,m68k-softmmu,microblaze-softmmu,moxie-softmmu,ppc-softmmu,s390x-softmmu,x86_64-softmmu"
> +- TEST_CMD="make check-qtest check-tcg V=1"
>  
>  
>  # We don't need to exercise every backend with every front-end


-- 
Alex Bennée

Re: [PATCH] travis.yml: Drop libcap-dev

[Alex Bennée]

Greg Kurz  writes:

> Commit b1553ab12fe0 converted virtfs-proxy-helper to using libcap-ng. There
> aren't any users of libcap anymore. No need to install libcap-dev.
>
> Signed-off-by: Greg Kurz 
> ---
>
> Yet another follow-up to Paolo's patch to use libcap-ng instead of libcap.
> Like with the docker and the gitlab CI patches, if I get an ack from Alex
> I'll gladly merge this in the 9p tree and send a PR as soon as 5.0 dev
> begins. I'll make sure the SHA1 for Paolo's patch remains the same.

Acked-by: Alex Bennée 

>
>  .travis.yml |1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/.travis.yml b/.travis.yml
> index 445b0646c18a..6cb8af6fa599 100644
> --- a/.travis.yml
> +++ b/.travis.yml
> @@ -26,7 +26,6 @@ addons:
>- libaio-dev
>- libattr1-dev
>- libbrlapi-dev
> -  - libcap-dev
>- libcap-ng-dev
>- libgcc-4.8-dev
>- libgnutls28-dev


-- 
Alex Bennée

Re: [PATCH v3] travis.yml: Run tcg tests with tci

[Thomas Huth]

On 04/12/2019 14.48, Alex Bennée wrote:
> 
> Thomas Huth  writes:
> 
>> So far we only have compile coverage for tci. But since commit
>> 2f160e0f9797c7522bfd0d09218d0c9340a5137c ("tci: Add implementation
>> for INDEX_op_ld16u_i64") has been included now, we can also run the
>> "tcg" and "qtest" tests with tci, so let's enable them in Travis now.
>> Since we don't gain much additional test coverage by compiling all
>> targets, and TCI is broken e.g. with the Sparc targets, we also limit
>> the target list to a reasonable subset now (which should still get us
>> test coverage by tests/boot-serial-test for example).
> 
> Queued to testing/next, thanks.

Thanks! But could you maybe s/--enable-debug/--enable-debug-tcg/ as
Richard just suggested in his mail? Or want me to send a v4?

 Thomas

Re: [PATCH] target/i386: relax assert when old host kernels don't include msrs

[Paolo Bonzini]

On 04/12/19 14:33, Catherine Ho wrote:
> Hi Paolo
> [sorry to resend it, seems to reply it incorrectly]
> 
> On Wed, 4 Dec 2019 at 19:23, Paolo Bonzini  > wrote:
> 
> On 04/12/19 09:50, Catherine Ho wrote:
> > Commit 20a78b02d315 ("target/i386: add VMX features") unconditionally
> > add vmx msr entry although older host kernels don't include them.
> >
> > But old host kernel + newest qemu will cause a qemu crash as follows:
> > qemu-system-x86_64: error: failed to set MSR 0x480 to 0x0
> > target/i386/kvm.c:2932: kvm_put_msrs: Assertion `ret ==
> > cpu->kvm_msr_buf->nmsrs' failed.
> >
> > This fixes it by relaxing the condition.
> 
> This is intentional.  The VMX MSR entries should not have been added.
> What combination of host kernel/QEMU are you using, and what QEMU
> command line?
> 
> 
> Host kernel: 4.15.0 (ubuntu 18.04)
> Qemu: https://gitlab.com/virtio-fs/qemu/tree/virtio-fs-dev
> cmdline: qemu-system-x86_64 -M pc -cpu host --enable-kvm -smp 8 \
>                   -m 4G,maxmem=4G
> 
> But before 20a78b02d315, the older kernel + latest qemu can boot guest
> successfully.

Ok, so the problem is that some MSR didn't exist in that version.  Which
one it is?  Can you make it conditional, similar to MSR_IA32_VMX_VMFUNC?

Thanks,

Paolo