Re: [Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> The set_console() method is intended to ease higher level use cases
> that require a console device.
> 
> The amount of inteligence is limited on purpose, requiring either the
> device type explicitly, or the existence of a machine (pattern)
> definition.
> 
> Because of the console device type selection criteria (by machine
> type), users should also be able to define that.  It'll then be used
> for both '-machine' and for the console device type selection.
> 
> Users of the set_console() method will certainly be interested in
> accessing the console device, and for that a console_socket property
> has been added.
> 
> Signed-off-by: Cleber Rosa 
> ---
>  scripts/qemu.py  |  97 +++-
>  scripts/test_qemu.py | 176 +++
>  2 files changed, 272 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/test_qemu.py
> 
> diff --git a/scripts/qemu.py b/scripts/qemu.py
> index 7813dd45ad..89db5bc6b2 100644
> --- a/scripts/qemu.py
> +++ b/scripts/qemu.py
> @@ -17,19 +17,41 @@ import logging
>  import os
>  import subprocess
>  import qmp.qmp
> +import re
>  import shutil
> +import socket
>  import tempfile
>  
>  
>  LOG = logging.getLogger(__name__)
>  
>  
> +#: Maps machine types to the preferred console device types
> +CONSOLE_DEV_TYPES = {
> +r'^clipper$': 'isa-serial',
> +r'^malta': 'isa-serial',
> +r'^(pc.*|q35.*|isapc)$': 'isa-serial',
> +r'^(40p|powernv|prep)$': 'isa-serial',
> +r'^pseries.*': 'spapr-vty',
> +r'^s390-ccw-virtio.*': 'sclpconsole',
> +}
> +
> +
>  class QEMUMachineError(Exception):
>  """
>  Exception called when an error in QEMUMachine happens.
>  """
>  
>  
> +class QEMUMachineAddDeviceError(QEMUMachineError):
> +"""
> +Exception raised when a request to add a device can not be fulfilled
> +
> +The failures are caused by limitations, lack of information or 
> conflicting
> +requests on the QEMUMachine methods.  This exception does not represent
> +failures reported by the QEMU binary itself.
> +"""
> +
>  class MonitorResponseError(qmp.qmp.QMPError):
>  '''
>  Represents erroneous QMP monitor reply
> @@ -91,6 +113,10 @@ class QEMUMachine(object):
>  self._test_dir = test_dir
>  self._temp_dir = None
>  self._launched = False
> +self._machine = None
> +self._console_device_type = None
> +self._console_address = None
> +self._console_socket = None
>  
>  # just in case logging wasn't configured by the main script:
>  logging.basicConfig()
> @@ -175,9 +201,19 @@ class QEMUMachine(object):
>  self._monitor_address[1])
>  else:
>  moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
> -return ['-chardev', moncdev,
> +args = ['-chardev', moncdev,
>  '-mon', 'chardev=mon,mode=control',
>  '-display', 'none', '-vga', 'none']
> +if self._machine is not None:
> +args.extend(['-machine', self._machine])
> +if self._console_device_type is not None:
> +self._console_address = os.path.join(self._temp_dir,
> + self._name + 
> "-console.sock")
> +chardev = ('socket,id=console,path=%s,server,nowait' %
> +   self._console_address)
> +device = '%s,chardev=console' % self._console_device_type
> +args.extend(['-chardev', chardev, '-device', device])
> +return args
>  
>  def _pre_launch(self):
>  self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
> @@ -202,6 +238,10 @@ class QEMUMachine(object):
>  
>  self._qemu_log_path = None
>  
> +if self._console_socket is not None:
> +self._console_socket.close()
> +self._console_socket = None
> +
>  if self._temp_dir is not None:
>  shutil.rmtree(self._temp_dir)
>  self._temp_dir = None
> @@ -365,3 +405,58 @@ class QEMUMachine(object):
>  Adds to the list of extra arguments to be given to the QEMU binary
>  '''
>  return self._args.extend(args)
> +
> +def set_machine(self, machine_type):
> +'''
> +Sets the machine type
> +
> +If set, the machine type will be added to the base arguments
> +of the resulting QEMU command line.
> +'''
> +self._machine = machine_type
> +
> +def set_console(self, device_type=None):
> +'''
> +Sets the device type for a console device
> +
> +If set, the console device and a backing character device will
> +be added to the base arguments of the resulting QEMU command
> +line.
> +
> +This is a convenience method that will either use the provided
> +device type, of if not given, it will used the device type set
> +on 

Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable

[Markus Armbruster]

Fam Zheng  writes:

> On Thu, 05/24 19:16, Paolo Bonzini wrote:
>> On 21/05/2018 08:35, Fam Zheng wrote:
>> > Coverity doesn't like the tests under fail label (report CID 1385847).
>> > Reset the fields so the clean up order is more apparent.
>> > 
>> > Signed-off-by: Fam Zheng 
>> > ---
>> >  block/nvme.c | 7 +++
>> >  1 file changed, 7 insertions(+)
>> > 
>> > diff --git a/block/nvme.c b/block/nvme.c
>> > index 6f71122bf5..8239b920c8 100644
>> > --- a/block/nvme.c
>> > +++ b/block/nvme.c
>> > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char 
>> > *device, int namespace,
>> >  qemu_co_queue_init(>dma_flush_queue);
>> >  s->nsid = namespace;
>> >  s->aio_context = bdrv_get_aio_context(bs);
>> > +
>> > +/* Fields we've not touched should be zero-initialized by block layer
>> > + * already, but reset them anyway to make the error handling code 
>> > easier to
>> > + * reason. */
>> > +s->regs = NULL;
>> > +s->vfio = NULL;
>> > +
>> >  ret = event_notifier_init(>irq_notifier, 0);
>> >  if (ret) {
>> >  error_setg(errp, "Failed to init event notifier");
>> > 
>> 
>> I think we should just mark it as a false positive or do something like
>> 
>> fail_regs:
>> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
>> fail_vfio:
>> qemu_vfio_close(s->vfio);
>> fail:
>> g_free(s->queues);
>> event_notifier_cleanup(>irq_notifier);
>> return ret;
>> 
>> even though it's a larger patch.
>
> And that makes five labels in total, I'm not sure I like it:
>
> fail_handler:
> aio_set_event_notifier(bdrv_get_aio_context(bs), >irq_notifier,
>false, NULL, NULL);
> fail_queue:
> nvme_free_queue_pair(bs, s->queues[0]);
> fail_regs:
> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
> fail_vfio:
> qemu_vfio_close(s->vfio);
> fail:
> g_free(s->queues);
> event_notifier_cleanup(>irq_notifier);
> return ret;

Doesn't look materially worse to me :)

With nice cleanup functions that detect "hasn't been set up" and do
nothing then, like free(NULL), you can use just one label.  Sadly,
cleanup functions are often not nice that way.

> Maybe we just mark it as false positive then?
>
> Fam

Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks

[Markus Armbruster]

"Michael S. Tsirkin"  writes:

> On Thu, May 24, 2018 at 01:16:24PM -0500, Eric Blake wrote:
>> On 05/24/2018 11:01 AM, Michael S. Tsirkin wrote:
>> > On Thu, May 24, 2018 at 11:00:19AM -0500, Eric Blake wrote:
>> > > On 05/24/2018 10:52 AM, Eric Blake wrote:
>> > > 
>> > > > Also, since waitpid() can only return either s->qemu_pid or -1 as we
>> > > > aren't using WNOHANG, it may also be worth asserting that if pid == -1,
>> > > > we either have EAGAIN (but why aren't we looping in that case?) or
>> > > > ECHILD.
>> > > 
>> > > I meant EINTR, not EAGAIN.  But in general, using waitpid() to collect
>> > > process status without doing it in a loop is risky.
>> > 
>> > Interesting. Risky how?
>> 
>> If your process has any signal handler installed, then an EINTR failure
>> means you interpret a transient failure to grab process status (because your
>> check was interrupted by something else) as a permanent failure, unless you
>> go back to another waitpid() in a loop.
>
> I don't think we have a handler installed, though.

That's a nasty assumption to make for a library.

Re: [Qemu-devel] [PATCH 3/5] Acceptance tests: add quick VNC tests

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> This patch adds a few simple behavior tests for VNC.  These tests
> introduce manipulation of the QEMUMachine arguments, by setting
> the arguments, instead of adding to the existing ones.

I'm confused by this. The code uses 'add_args', so it does add to the arguments,
no?

> 
> Signed-off-by: Cleber Rosa 
> ---
>  tests/acceptance/test_vnc.py | 50 
>  1 file changed, 50 insertions(+)
>  create mode 100644 tests/acceptance/test_vnc.py
> 
> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
> new file mode 100644
> index 00..9d9a35cf55
> --- /dev/null
> +++ b/tests/acceptance/test_vnc.py
> @@ -0,0 +1,50 @@

Copyright header is missing here too.

Fam

> +from avocado_qemu import Test
> +
> +
> +class Vnc(Test):

Should VncTest be a better class name?

> +"""
> +:avocado: enable
> +:avocado: tags=vnc,quick
> +"""
> +def test_no_vnc(self):
> +self.vm.add_args('-nodefaults', '-S')
> +self.vm.launch()
> +self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
> +
> +def test_no_vnc_change_password(self):
> +self.vm.add_args('-nodefaults', '-S')
> +self.vm.launch()
> +self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
> +set_password_response = self.vm.qmp('change',
> +device='vnc',
> +target='password',
> +arg='new_password')
> +self.assertIn('error', set_password_response)
> +self.assertEqual(set_password_response['error']['class'],
> + 'GenericError')
> +self.assertEqual(set_password_response['error']['desc'],
> + 'Could not set password')
> +
> +def test_vnc_change_password_requires_a_password(self):
> +self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
> +self.vm.launch()
> +self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
> +set_password_response = self.vm.qmp('change',
> +device='vnc',
> +target='password',
> +arg='new_password')
> +self.assertIn('error', set_password_response)
> +self.assertEqual(set_password_response['error']['class'],
> + 'GenericError')
> +self.assertEqual(set_password_response['error']['desc'],
> + 'Could not set password')
> +
> +def test_vnc_change_password(self):
> +self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password')
> +self.vm.launch()
> +self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
> +set_password_response = self.vm.qmp('change',
> +device='vnc',
> +target='password',
> +arg='new_password')
> +self.assertEqual(set_password_response['return'], {})
> -- 
> 2.17.0
> 

Fam

Re: [Qemu-devel] [PATCH 1/5] Add functional/acceptance tests infrastructure

[Fam Zheng]

On Thu, 05/24 20:58, Cleber Rosa wrote:
> This patch adds the very minimum infrastructure necessary for writing
> and running functional/acceptance tests, including:
> 
>  * Documentation
>  * The avocado_qemu.Test base test class
>  * One example tests (test_version.py)
> 
> Additional functionality is expected to be added along the tests that
> require them.
> 
> Signed-off-by: Cleber Rosa 
> ---
>  tests/acceptance/README.rst   | 141 ++

Could you add the actual doc text to docs/devel/testing.rst and reference it
here?

>  tests/acceptance/avocado_qemu/__init__.py |  45 +++
>  tests/acceptance/test_version.py  |  13 ++
>  3 files changed, 199 insertions(+)
>  create mode 100644 tests/acceptance/README.rst
>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>  create mode 100644 tests/acceptance/test_version.py
> 
> diff --git a/tests/acceptance/README.rst b/tests/acceptance/README.rst
> new file mode 100644
> index 00..f1434177da
> --- /dev/null
> +++ b/tests/acceptance/README.rst
> @@ -0,0 +1,141 @@
> +==
> + Acceptance tests using the Avocado Framework
> +==
> +
> +This directory hosts functional tests, also known as acceptance level
> +tests.  They're usually higher level, and may interact with external
> +resources and with various guest operating systems.
> +
> +The tests are written using the Avocado Testing Framework, in
> +conjunction with a the ``avocado_qemu.Test`` class, distributed here.
> +
> +Installation
> +
> +
> +To install Avocado and its dependencies, run::
> +
> +  pip install --user avocado-framework
> +
> +Alternatively, follow the instructions on this link:
> +
> +  
> http://avocado-framework.readthedocs.io/en/latest/GetStartedGuide.html#installing-avocado
> +
> +Overview
> +
> +
> +This directory provides the ``avocado_qemu`` Python module, containing
> +the ``avocado_qemu.Test`` class.  Here's a simple usage example::
> +
> +  from avocado_qemu import Test
> +
> +
> +  class Version(Test):
> +  """
> +  :avocado: enable
> +  :avocado: tags=quick
> +  """
> +  def test_qmp_human_info_version(self):
> +  self.vm.launch()
> +  res = self.vm.command('human-monitor-command',
> +command_line='info version')
> +  self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
> +
> +To execute your test, run::
> +
> +  avocado run test_version.py
> +
> +To run all tests in the current directory, tagged in a particular way,
> +run::
> +
> +  avocado run -t  .
> +
> +The ``avocado_qemu.Test`` base test class
> +=
> +
> +The ``avocado_qemu.Test`` class has a number of characteristics that
> +are worth being mentioned right away.
> +
> +First of all, it attempts to give each test a ready to use QEMUMachine
> +instance, available at ``self.vm``.  Because many tests will tweak the
> +QEMU command line, launching the QEMUMachine (by using ``self.vm.launch()``)
> +is left to the test writer.
> +
> +At test "tear down", ``avocado_qemu.Test`` handles the QEMUMachine
> +shutdown.
> +
> +QEMUMachine
> +---
> +
> +The QEMUMachine API should be somewhat familiar to QEMU hackers.  It's
> +used in the Python iotests, device-crash-test and other Python scripts.
> +
> +QEMU binary selection
> +-
> +
> +The QEMU binary used for the ``self.vm`` QEMUMachine instance will
> +primarily depend on the value of the ``qemu_bin`` parameter.  If it's
> +not explicitly set, its default value will be the result of a dynamic
> +probe in the same source tree.  A suitable binary will be one that
> +targets the architecture matching host machine.
> +
> +Based on this description, test writers will usually rely on one of
> +the following approaches:
> +
> +1) Set ``qemu_bin``, and use the given binary
> +
> +2) Do not set ``qemu_bin``, and use a QEMU binary named like
> +   "${arch}-softmmu/qemu-system-${arch}", either in the current
> +   working directory, or in the current source tree.
> +
> +The resulting ``qemu_bin`` value will be preserved in the
> +``avocado_qemu.Test`` as an attribute with the same name.
> +
> +Attribute reference
> +===
> +
> +Besides the attributes and methods that are part of the base
> +``avocado.Test`` class, the following attributes are available on any
> +``avocado_qemu.Test`` instance.
> +
> +vm
> +--
> +
> +A QEMUMachine instance, initially configured according to the given
> +``qemu_bin`` parameter.
> +
> +qemu_bin
> +
> +
> +The preserved value of the ``qemu_bin`` parameter or the result of the
> +dynamic probe for a QEMU binary in the current working directory or
> +source tree.
> +
> +Parameter reference
> +===
> +
> +To understand how Avocado parameters are accessed by tests, and how
> +they can be passed to tests, please refer 

Re: [Qemu-devel] Questions about vNVDIMM on qemu/KVM

[Yasunori Goto]

> >
> > But ,I would like understand one more thing.
> > In the following mail, it seems that e820 bus will be used for fake DAX.
> >
> > https://lists.01.org/pipermail/linux-nvdimm/2018-January/013926.html
> >
> > Could you tell me what is relationship between "fake DAX" in this mail
> > and Guest DAX?
> > Why e820 is necessary for this case?
> >
> 
> It was proposed as a starting template for writing a new nvdimm bus
> driver. All we need is a way to communicate both the address range and
> the flush interface. This could be done with a new SPA Range GUID with
> the NFIT, or a custom virtio-pci device that registers a special
> nvdimm region with this property. My preference is whichever approach
> minimizes the code duplication, because the pmem driver should be
> re-used as much as possible.

Ok, I see.
Thank you very much for your explanation.

Bye,
---
Yasunori Goto

[Qemu-devel] [PATCH] bochs-display: add missing break

[Gerd Hoffmann]

Fixes: CID 1391291
Signed-off-by: Gerd Hoffmann 
---
 hw/display/bochs-display.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/display/bochs-display.c b/hw/display/bochs-display.c
index c33524b558..1187d77576 100644
--- a/hw/display/bochs-display.c
+++ b/hw/display/bochs-display.c
@@ -158,6 +158,7 @@ static int bochs_display_get_mode(BochsDisplayState *s,
 /* best effort: support native endianess only */
 mode->format = PIXMAN_r5g6b5;
 mode->bytepp = 2;
+break;
 case 32:
 mode->format = s->big_endian_fb
 ? PIXMAN_BE_x8r8g8b8
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 1/1] sandbox: disable -sandbox if CONFIG_SECCOMP undefined

[Yi Min Zhao]

在 2018/5/24 下午9:40, Paolo Bonzini 写道:

On 24/05/2018 09:53, Eduardo Otubo wrote:

Thanks! But I have not got response from Paolo.  I have added him to
CC list.


  I'll just wait one more ACK and will send a pull request on the
seccomp queue. Thanks for the contribution.



So... what I should do is wait?


Yes, even though I think we're safe to proceed without his explicit ack.

The patch is okay; however, as a follow-up, you could consider moving
all the CONFIG_SECCOMP code to qemu-seccomp.c.

This way, the only #ifdef remains the one around qemu_opts_foreach.

Paolo


Thanks for your comment! Indeed, moving to the single C file is much 
more clear.

I will do this after this patch.

@Otubo, what about next step?

Re: [Qemu-devel] [PATCH] gdbstub: Prevent fd leakage

[Thomas Huth]

On 25.05.2018 00:34, Philippe Mathieu-Daudé wrote:
> Since 2f652224f7, we now check if socket_set_nodelay() errored,
> but forgot to close the socket before reporting an error.
> 
> Fixes: Coverity CID 1391290 (RESOURCE_LEAK)
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  gdbstub.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index e4ece2f5bc..9c860cd81c 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1836,6 +1836,7 @@ static bool gdb_accept(void)
>  /* set short latency */
>  if (socket_set_nodelay(fd)) {
>  perror("setsockopt");
> +close(fd);
>  return false;
>  }

Reviewed-by: Thomas Huth 

Re: [Qemu-devel] [PATCH v7 4/4] monitor: add lock to protect mon_fdsets

[Peter Xu]

On Thu, May 24, 2018 at 10:28:37AM +0100, Stefan Hajnoczi wrote:
> On Thu, May 24, 2018 at 12:39:52PM +0800, Peter Xu wrote:
> >  int monitor_fdset_get_fd(int64_t fdset_id, int flags)
> >  {
> > -#ifndef _WIN32
> > +#ifdef _WIN32
> > +return -ENOENT;
> 
> stubs/fdset.c:monitor_fdset_get_fd() should return -ENOENT instead of -1
> now.

Yes that's intended.  That's actually a suggestion from Markus since
changing the return code will simplify the code.  I mentioned it in
the commit message too:

"""
The monitor_fdset_get_fd() handling is a bit tricky: now we need to call
qemu_mutex_unlock() which might pollute errno, so we need to make sure
the correct errno be passed up to the callers.  To make things simpler,
we let monitor_fdset_get_fd() return the -errno directly when error
happens, then in qemu_open() we translate it back into errno.
"""

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 14/27] iommu: Add IOMMU index concept to IOMMU API

[Peter Xu]

On Thu, May 24, 2018 at 11:54:17AM +0100, Peter Maydell wrote:
> On 24 May 2018 at 07:23, Peter Xu  wrote:
> > On Wed, May 23, 2018 at 12:47:16PM +0100, Peter Maydell wrote:
> >> On 23 May 2018 at 02:06, Peter Xu  wrote:
> >> > Could you elaborate a bit more on why IOMMU notifier failed to
> >> > corporate when passing in MemTxAttrs?  I am not sure I caught the idea
> >> > here, but can we copy the MemTxAttrs into IOMMUTLBEntry when
> >> > translating, then in IOMMU notifiers we can know the attrs (if that is
> >> > what MPC wants)?
> >>
> >> (1) The notifier API lets you register a notifier before you've
> >> called the translate API
> >
> > Yes.
> >
> >> (2) An IOMMUTLBEntry can be valid for more than just the txattrs
> >> that it was passed in (for instance, if an IOMMU doesn't care
> >> about txattrs at all, then the resulting TLB entry is valid for
> >> any txattrs; or if the IOMMU only cares about attrs.secure the
> >> resulting TLB entries are valid for both attrs.user=0 and
> >> attrs.user=1).
> >
> > [1]
> >
> > Yes exactly, that's why I thought copying the txattrs into IOTLB
> > should work.
> 
> I'm a bit confused about why the IOMMUTLBEntry is relevant here.
> That's the thing returned from the translate method, so there's
> no point in copying txattrs into it, because the caller by definition
> already had them. At the point where the IOMMU notices a guest
> changed the config, it doesn't have an IOMMUTLBEntry or a set of
> tx attrs.

Yes the txattrs is not for the translate() callers, but for IOMMU
notifiers only.  Thanks for the pointer below [1], I think it's very
similar to what Paolo mentioned there at [1], the major difference is
that Paolo suggested to put txattrs into both IOMMUNotify and
memory_region_notify_one(), while my above suggestion is that we can
directly copy it into IOMMUTLBEntry - note that both IOMMUNotify and
memory_region_notify_one will have a IOMMUTLBEntry parameter.  Though
I am not 100% clear on Paolo's suggestion on why to add two txattrs
parameters for each function (since I thought all the values in
txattrs to be passed to either IOMMUNotify or memory_region_notify_one
should be valid, so I am not sure why we need to "indicate which
attributes matter (0 = indifferent, 1 = matter)").

> 
> >> (3) when the IOMMU calls the notifier because the guest config
> >> changed it doesn't have tx attributes, so it would have to
> >> fabricate some; and the guest config will invalidate transactions
> >> with some combinations of tx attributes and not others.
> >
> > IMHO it doesn't directly matter with what we are discussing now.  That
> > IOMMU_NOTIFIER_[UN]MAP flag tells what kind of message would the
> > notifier be interested in from "what kind of mapping it is".  IMHO
> > it's not really related to some other attributes when translation
> > happens - in our case, it does not directly related to what txattrs
> > value is.  Here as mentioned at [1] above IMHO we can still check this
> > against txattrs in the notifier handler, then we ignore messages that
> > we don't care about.  Actually the IOMMU_NOTIFIER_[UN]MAP flags can be
> > removed and we can just do similar things (e.g., we can skip MAP
> > messages if we only care about UNMAP messages), but since it's a
> > general concept and easy to be generalized, so we provided these
> > MAP/UNMAP flags to ease the notifier hooks.
> >
> > In other words, I think we can also add more flags for SECURE or not.
> > However I still don't see a reason (from above three points) on why we
> > can't pass in txattrs directly into translate(), and at the same time
> > we copy the txattrs into IOTLB so that IOMMUTLBEntry can contain some
> > context information. [2]
> 
> I'm afraid I really don't understand the design you're proposing
> here. But overall I think the point of divergence is that
> the mapping from "transaction attributes" to "translation contexts"
> (ie, effectively different page tables) is not 1:1. So for instance:
> 
> Our current IOMMUs which don't care about txattrs:
> 
>   [any txattr at all] -> the one and only translation context
> 
> An IOMMU which cares about attrs.secure, and also treats
> attrs.unspecified like secure:
>   [any txattr with attrs.secure = 1]  \-> 'secure' context'
>   MEMATTRS_UNSPECIFIED/
> 
>   [txattrs with secure = 1] -> 'nonsecure' context

(This part is a bit interesting - the "secure" bit is actually flipped
 between txattrs and the context...)

> 
> An IOMMU which cares about attrs.secure and attrs.user:
>   [secure=1,user=1]   -> 'secure user' context
>   [secure=0,user=1]   -> 'ns user' context
>   [secure=1,user=0]   -> 's priv' context
>   [secure=0,user=0]   -> 'ns priv' context
> 
> The IOMMU index captures this idea that there is not a 1:1
> mapping, so we have a way to think about and refer to the
> actual set of translation contexts that the IOMMU has.

Yes.  I'd say I am not really against this iommu_idx idea.  My only

Re: [Qemu-devel] [PULL 1/2] Implement .hex file loader

[Su Hang]

You are right. I'm shocked by my mistakes...

> The hex format is used mainly by Cortex-M boards. This code doesn't
> support them, because armv7m uses its own load function. Could you add
> support for armv7m?
> 
> Best regards, Julia Suvorova.

Julia indeed have mentioned me about this. But at that time, I was thinking
about "get this patch merged first, then add support for armv7m". I am wrong.

SU Hang


> -Original Messages-
> From: "Peter Maydell" 
> Sent Time: 2018-05-24 22:40:04 (Thursday)
> To: "Su Hang" 
> Cc: "Stefan Hajnoczi" , j...@groklearning.com, "Joel 
> Stanley" , "QEMU Developers" 
> Subject: Re: [Qemu-devel] [PULL 1/2] Implement .hex file loader
> 
> On 24 May 2018 at 12:28, Su Hang  wrote:
> > This patch adds Intel Hexadecimal Object File format support to
> > the loader.  The file format specification is available here:
> > http://www.piclist.com/techref/fileext/hex/intel.htm
> >
> > The file format is mainly intended for embedded systems
> > and microcontrollers, such as Micro:bit Arduino, ARM, STM32, etc.
> 
> Could we have some more rationale here, please? For instance,
> we could mention who ships binaries in this format, what other
> boot loaders handle this, and so on. The idea is to explain
> why it's worth our having this code, rather than just having
> users convert their hex files to a format we already handle
> (ie why there is a significant body of users with hex format
> images they want to load).
> 
> > diff --git a/hw/arm/boot.c b/hw/arm/boot.c
> > index 9496f331a8fa..17fe1a8c287b 100644
> > --- a/hw/arm/boot.c
> > +++ b/hw/arm/boot.c
> > @@ -1038,12 +1038,17 @@ void arm_load_kernel(ARMCPU *cpu, struct 
> > arm_boot_info *info)
> >  kernel_size = load_uimage_as(info->kernel_filename, , NULL,
> >   _linux, NULL, NULL, as);
> >  }
> > +if (kernel_size < 0) {
> > +/* 32-bit ARM Intel HEX file */
> > +entry = 0;
> > +kernel_size = load_targphys_hex_as(info->kernel_filename, , 
> > as);
> > +}
> >  if (arm_feature(>env, ARM_FEATURE_AARCH64) && kernel_size < 0) {
> >  kernel_size = load_aarch64_image(info->kernel_filename,
> >   info->loader_start, , as);
> >  is_linux = 1;
> >  } else if (kernel_size < 0) {
> > -/* 32-bit ARM */
> > +/* 32-bit ARM binary file */
> >  entry = info->loader_start + KERNEL_LOAD_ADDR;
> >  kernel_size = load_image_targphys_as(info->kernel_filename, entry,
> >   info->ram_size - 
> > KERNEL_LOAD_ADDR,
> 
> I don't think it makes sense to add support for this format here.
> Who is using hex files to provide A-class Linux kernels?
> 
> (Note that hw/arm/boot.c does *not* handle -kernel for M-class cores.)
> 
> There might be an argument for wiring up hex file support
> in the "generic loader" hw/misc/generic-loader.c
> (documentation in docs/generic-loader.txt).
> 
> It looks like your implementation calls rom_add_blob_fixed_as()
> as it goes along to add chunks of data to guest memory, but
> it doesn't do anything to undo that if it detects an error
> in the input halfway through and returns a failure code.
> That matters if you want to put it in a chain of "try this
> format? if that didn't work, try this other format; last
> resort, load as binary" calls like you have here.
> 
> It's probably worth splitting the "add load_targphys_hex_as()"
> patch from the one that wires it up for a specific loader.
> 
> thanks
> -- PMM

Re: [Qemu-devel] [PULL 2/2] Add QTest testcase for the Intel Hexadecimal

[Su Hang]

Sure, I will list other involved files. Thanks for you suggestion.

SU Hang

"Eric Blake" wrote:
> On 05/24/2018 06:29 AM, Su Hang wrote:
> > 'test.hex' file is a bare metal ARM software stored in Hexadecimal
> > Object Format. When it's loaded by QEMU, it will print "Hello world!\n"
> > on console.
> > 
> > `pre_store` array in 'hexloader-test.c' file, stores the binary format
> > of 'test.hex' file, which is used to verify correctness.
> > 
> > Reviewed-by: Stefan Hajnoczi 
> > Suggested-by: Steffen Gortz 
> > Suggested-by: Stefan Hajnoczi 
> > Signed-off-by: Su Hang 
> > ---
> >   MAINTAINERS  |  6 
> >   configure|  4 +++
> >   tests/Makefile.include   |  2 ++
> >   tests/hex-loader-check-data/test.hex | 12 
> >   tests/hexloader-test.c   | 56 
> > 
> 
> The previous patch also touched:
> 
>   hw/arm/boot.c   |   7 +-
>   hw/core/loader.c| 246 
> 
>   include/hw/loader.h |  12 +++
> 
> > +++ b/MAINTAINERS
> > @@ -1291,6 +1291,12 @@ F: hw/core/generic-loader.c
> >   F: include/hw/core/generic-loader.h
> >   F: docs/generic-loader.txt
> > 
> > +Intel Hexadecimal Object File Loader
> > +M: Su Hang 
> > +S: Maintained
> > +F: tests/hexloader-test.c
> > +F: tests/hex-loader-check-data/test.hex
> > +
> 
> It looks odd having a maintainer that claims only test files; do you 
> want to also list some of the other files touched by this patch so that 
> you get notification if one of the implementation files has subsequent 
> patches (rather than just the test files)?
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable

[Fam Zheng]

On Thu, 05/24 19:16, Paolo Bonzini wrote:
> On 21/05/2018 08:35, Fam Zheng wrote:
> > Coverity doesn't like the tests under fail label (report CID 1385847).
> > Reset the fields so the clean up order is more apparent.
> > 
> > Signed-off-by: Fam Zheng 
> > ---
> >  block/nvme.c | 7 +++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/block/nvme.c b/block/nvme.c
> > index 6f71122bf5..8239b920c8 100644
> > --- a/block/nvme.c
> > +++ b/block/nvme.c
> > @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char 
> > *device, int namespace,
> >  qemu_co_queue_init(>dma_flush_queue);
> >  s->nsid = namespace;
> >  s->aio_context = bdrv_get_aio_context(bs);
> > +
> > +/* Fields we've not touched should be zero-initialized by block layer
> > + * already, but reset them anyway to make the error handling code 
> > easier to
> > + * reason. */
> > +s->regs = NULL;
> > +s->vfio = NULL;
> > +
> >  ret = event_notifier_init(>irq_notifier, 0);
> >  if (ret) {
> >  error_setg(errp, "Failed to init event notifier");
> > 
> 
> I think we should just mark it as a false positive or do something like
> 
> fail_regs:
> qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
> fail_vfio:
> qemu_vfio_close(s->vfio);
> fail:
> g_free(s->queues);
> event_notifier_cleanup(>irq_notifier);
> return ret;
> 
> even though it's a larger patch.

And that makes five labels in total, I'm not sure I like it:

fail_handler:
aio_set_event_notifier(bdrv_get_aio_context(bs), >irq_notifier,
   false, NULL, NULL);
fail_queue:
nvme_free_queue_pair(bs, s->queues[0]);
fail_regs:
qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
fail_vfio:
qemu_vfio_close(s->vfio);
fail:
g_free(s->queues);
event_notifier_cleanup(>irq_notifier);
return ret;

Maybe we just mark it as false positive then?

Fam

[Qemu-devel] [PATCH] migration: use g_free for ram load bitmap

[Peter Xu]

Buffers allocated with bitmap_new() should be freed with g_free().

Both reported by Coverity:

*** CID 1391300:  API usage errors  (ALLOC_FREE_MISMATCH)
/migration/ram.c: 3517 in ram_dirty_bitmap_reload()
3511  * the last one to sync, we need to notify the main send thread.
3512  */
3513 ram_dirty_bitmap_reload_notify(s);
3514
3515 ret = 0;
3516 out:
>>> CID 1391300:  API usage errors  (ALLOC_FREE_MISMATCH)
>>> Calling "free" frees "le_bitmap" using "free" but it should have been 
>>> freed using "g_free".
3517 free(le_bitmap);
3518 return ret;
3519 }
3520
3521 static int ram_resume_prepare(MigrationState *s, void *opaque)
3522 {

*** CID 1391292:  API usage errors  (ALLOC_FREE_MISMATCH)
/migration/ram.c: 249 in ramblock_recv_bitmap_send()
243  * Mark as an end, in case the middle part is screwed up due to
244  * some "misterious" reason.
245  */
246 qemu_put_be64(file, RAMBLOCK_RECV_BITMAP_ENDING);
247 qemu_fflush(file);
248
>>> CID 1391292:  API usage errors  (ALLOC_FREE_MISMATCH)
>>> Calling "free" frees "le_bitmap" using "free" but it should have been 
>>> freed using "g_free".
249 free(le_bitmap);
250
251 if (qemu_file_get_error(file)) {
252 return qemu_file_get_error(file);
253 }
254

Signed-off-by: Peter Xu 
---
 migration/ram.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 5bcbf7a9f9..c53e8369a3 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -246,7 +246,7 @@ int64_t ramblock_recv_bitmap_send(QEMUFile *file,
 qemu_put_be64(file, RAMBLOCK_RECV_BITMAP_ENDING);
 qemu_fflush(file);
 
-free(le_bitmap);
+g_free(le_bitmap);
 
 if (qemu_file_get_error(file)) {
 return qemu_file_get_error(file);
@@ -3514,7 +3514,7 @@ int ram_dirty_bitmap_reload(MigrationState *s, RAMBlock 
*block)
 
 ret = 0;
 out:
-free(le_bitmap);
+g_free(le_bitmap);
 return ret;
 }
 
-- 
2.17.0

[Qemu-devel] [Bug 1769189] Re: Issue with qemu 2.12.0 + SATA

[John Snow]

I tried bisecting as well, and I wound up at:

1a423896 -- five out of five boot attempts succeeded.
d759c951 -- five out of five boot attempts failed.


d759c951f3287fad04210a52f2dc93f94cf58c7f is the first bad commit
commit d759c951f3287fad04210a52f2dc93f94cf58c7f
Author: Alex Bennée 
Date:   Tue Feb 27 12:52:48 2018 +0300

replay: push replay_mutex_lock up the call tree



My methodology was to boot QEMU like this:

./x86_64-softmmu/qemu-system-x86_64 -m 4096 -cpu host -M q35 -enable-kvm
-smp 4 -drive id=sda,if=none,file=/home/bos/jhuston/windows_10.qcow
-device ide-hd,drive=sda -qmp tcp::,server,nowait

and run it three times with -snapshot and see if it hung during boot; if
it did, I marked the commit bad. If it did not, I booted and attempted
to log in and run CrystalDiskMark. If it froze before I even launched
CDM, I marked it bad.

Interestingly enough, on a subsequent (presumably bad) commit (6dc0f529)
which hangs fairly reliably on bootup (66%) I can occasionally get into
Windows 10 and run CDM -- and that unfortunately does not seem to
trigger the error again, so CDM doesn't look like a reliable way to
trigger the hangs.



Anyway, d759c951 definitely appears to change the odds of AHCI locking up 
during boot for me, and I suppose it might have something to do with how it is 
changing the BQL acquisition/release in main-loop.c, but I am not sure why/what 
yet.

Before this patch, we only lock the iothread and re-lock it if there was
a timeout, and after this patch we *always* lock and unlock the
iothread. This is probably just exposing some latent bug in the AHCI
emulator that has always existed, but now the odds of seeing it are much
higher.

I'll have to dig as to what the race is -- I'm not sure just yet.


If those of you who are seeing this bug too could confirm for me that d759c951 
appears to be the guilty party, that probably wouldn't hurt.

Thanks!
--js

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

Title:
  Issue with qemu 2.12.0 + SATA

Status in QEMU:
  New

Bug description:
  [EDIT: I first thought that OVMF was the issue, but it turns out to be
  SATA]

  I had a Windows 10 VM running perfectly fine with a SATA drive, since
  I upgraded to qemu 2.12, the guests hangs for a couple of minutes,
  works for a few seconds, and hangs again, etc. By "hang" I mean it
  doesn't freeze, but it looks like it's waiting on IO or something, I
  can move the mouse but everything needing disk access is unresponsive.

  What doesn't work: qemu 2.12 with SATA
  What works: using VirIO-SCSI with qemu 2.12 or downgrading qemu to 2.11.1 and 
keep using SATA.

  Platform is arch linux 4.16.7 on skylake and Haswell, I have attached
  the vm xml file.

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

Re: [Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Cleber Rosa]

On 05/24/2018 08:58 PM, Cleber Rosa wrote:
> TL;DR
> =
> 
> Another version, with a minimalist approach, to the acceptance tests
> infrastructure for QEMU, based on the Avocado Testing Framework.
> 
> Background
> ==
> 
> The previous version, still considered an RFC, was sent to the list by
> Eduardo[1] was based on the work held in Amador's branch[2].  After
> reviewing in under a different light, including the experiences
> done and reported by Philippe[3].
> 

(major sigh for killing a line, writing a non-sense sentence)

... it was clear to me that a different approach would be better.

- Cleber.

> Differences from previous versions
> ==
> 
> The main difference is that this series include only the minimal
> changes deemed necessary to have a starting point.  I like to think
> that it's better connected to the QEMU community and project needs,
> and will hopefully allow for a more organic growth.
> 
> Since this version has less features than the previous versions,
> provided it's accepted, these are the next probable development tasks:
> 
>  * Provide a simple variants mechanism to allow the same tests to be
>run under different targets, machine models and devices (present on
>the previous versions as a "YAML to Mux" file with architecture
>definitions)
>  * Implement QEMUMachine migration support (present on the previous
>version in the "avocado_qemu.test._VM" class)
>  * Implement Guest OS image selection and download (mostly an Avocado
>feature, paired with a parameter convention and cloud-init support
>code)
>  * Implement interactive support for Guest OS sessions (present on
>the previous versions, supported by the aexpect Python module)
> 
> Even though this version shares very little (if any) code with the
> previous versions, the following is a list of noteworthy changes:
> 
>  * Tests directory is now "tests/acceptance" (was "tests/avocado")
>  * Base test class is now "avocado_qemu.Test" (was
>"avocado_qemu.test.QemuTest")
>  * Base test class is now hosted in "avocado_qemu/__init__.py" (was
>"avocado_qemu/test.py")
>  * Direct use of "qemu.QEMUMachine", that is, the
>avocado_qemu.test._VM class is gone
>  * avocado_qemu.Test won't search for QEMU binaries on $PATH.  To use
>QEMU binary on a custom system location it's necessary to use the
>"qemu_bin" parameter
>  * Example test in README.rst is distributed as a real test
>("test_version.py")
>  * A new "Linux boot console" test, loosely modeled after Phillipe's
>use case
> 
> Commit summary
> ==
> 
> Cleber Rosa (5):
>   Add functional/acceptance tests infrastructure
>   scripts/qemu.py: allow adding to the list of extra arguments
>   Acceptance tests: add quick VNC tests
>   scripts/qemu.py: introduce set_console() method
>   Acceptance tests: add Linux kernel boot and console checking test
> 
>  scripts/qemu.py | 103 +++-
>  scripts/test_qemu.py| 176 +++
>  tests/acceptance/README.rst | 141 +
>  tests/acceptance/avocado_qemu/__init__.py   |  45 +++
>  tests/acceptance/test_boot_linux_console.py |  37 ++
>  tests/acceptance/test_version.py|  13 ++
>  tests/acceptance/test_vnc.py|  50 
>  7 files changed, 564 insertions(+), 1 deletion(-)
>  create mode 100644 scripts/test_qemu.py
>  create mode 100644 tests/acceptance/README.rst
>  create mode 100644 tests/acceptance/avocado_qemu/__init__.py
>  create mode 100644 tests/acceptance/test_boot_linux_console.py
>  create mode 100644 tests/acceptance/test_version.py
>  create mode 100644 tests/acceptance/test_vnc.py
> 
> ---
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03443.html
> [2] https://github.com/apahim/qemu/commits/avocado_qemu
> [3] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03076.html
> 
> 

[Qemu-devel] [PATCH 5/5] Acceptance tests: add Linux kernel boot and console checking test

[Cleber Rosa]

This test boots a Linux kernel, and checks that the given command
line was effective in two ways:

 * It makes the kernel use the set "console device" as a console
 * The kernel records the command line as expected in the console

Given that way too many error conditions may occur, and detecting the
kernel boot progress status may not be trivial, this test relies on a
timeout to handle unexpected situations.  Also, it's *not* tagged as a
quick test for obvious reasons.

It may be useful, while interactively running/debugging this test, or
tests similar to this one, to show some of the logging channels.
Example:

 $ avocado --show=QMP,console run test_boot_linux_console.py

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/test_boot_linux_console.py | 37 +
 1 file changed, 37 insertions(+)
 create mode 100644 tests/acceptance/test_boot_linux_console.py

diff --git a/tests/acceptance/test_boot_linux_console.py 
b/tests/acceptance/test_boot_linux_console.py
new file mode 100644
index 00..52811bfe06
--- /dev/null
+++ b/tests/acceptance/test_boot_linux_console.py
@@ -0,0 +1,37 @@
+import logging
+
+from avocado_qemu import Test
+
+
+class BootLinuxConsole(Test):
+"""
+Boots a x86_64 Linux kernel and checks that the console is operational
+and the kernel command line is properly passed from QEMU to the kernel
+
+:avocado: enable
+:avocado: tags=x86_64
+"""
+
+timeout = 60
+
+def test(self):
+kernel_url = ('https://mirrors.kernel.org/fedora/releases/28/'
+  'Everything/x86_64/os/images/pxeboot/vmlinuz')
+kernel_hash = '238e083e114c48200f80d889f7e32eeb2793e02a'
+kernel_path = self.fetch_asset(kernel_url, asset_hash=kernel_hash)
+
+self.vm.set_machine('pc')
+self.vm.set_console()
+kernel_command_line = 'console=ttyS0'
+self.vm.add_args('-kernel', kernel_path,
+ '-append', kernel_command_line)
+self.vm.launch()
+console = self.vm.console_socket.makefile()
+console_logger = logging.getLogger('console')
+while True:
+msg = console.readline()
+console_logger.debug(msg.strip())
+if 'Kernel command line: %s' % kernel_command_line in msg:
+break
+if 'Kernel panic - not syncing' in msg:
+self.fail("Kernel panic reached")
-- 
2.17.0

[Qemu-devel] [PATCH 2/5] scripts/qemu.py: allow adding to the list of extra arguments

[Cleber Rosa]

Tests will often need to add extra arguments to QEMU command
line arguments.

Signed-off-by: Cleber Rosa 
---
 scripts/qemu.py | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 08a3e9af5a..7813dd45ad 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -359,3 +359,9 @@ class QEMUMachine(object):
 of the qemu process.
 '''
 return self._iolog
+
+def add_args(self, *args):
+'''
+Adds to the list of extra arguments to be given to the QEMU binary
+'''
+return self._args.extend(args)
-- 
2.17.0

[Qemu-devel] [PATCH 4/5] scripts/qemu.py: introduce set_console() method

[Cleber Rosa]

The set_console() method is intended to ease higher level use cases
that require a console device.

The amount of inteligence is limited on purpose, requiring either the
device type explicitly, or the existence of a machine (pattern)
definition.

Because of the console device type selection criteria (by machine
type), users should also be able to define that.  It'll then be used
for both '-machine' and for the console device type selection.

Users of the set_console() method will certainly be interested in
accessing the console device, and for that a console_socket property
has been added.

Signed-off-by: Cleber Rosa 
---
 scripts/qemu.py  |  97 +++-
 scripts/test_qemu.py | 176 +++
 2 files changed, 272 insertions(+), 1 deletion(-)
 create mode 100644 scripts/test_qemu.py

diff --git a/scripts/qemu.py b/scripts/qemu.py
index 7813dd45ad..89db5bc6b2 100644
--- a/scripts/qemu.py
+++ b/scripts/qemu.py
@@ -17,19 +17,41 @@ import logging
 import os
 import subprocess
 import qmp.qmp
+import re
 import shutil
+import socket
 import tempfile
 
 
 LOG = logging.getLogger(__name__)
 
 
+#: Maps machine types to the preferred console device types
+CONSOLE_DEV_TYPES = {
+r'^clipper$': 'isa-serial',
+r'^malta': 'isa-serial',
+r'^(pc.*|q35.*|isapc)$': 'isa-serial',
+r'^(40p|powernv|prep)$': 'isa-serial',
+r'^pseries.*': 'spapr-vty',
+r'^s390-ccw-virtio.*': 'sclpconsole',
+}
+
+
 class QEMUMachineError(Exception):
 """
 Exception called when an error in QEMUMachine happens.
 """
 
 
+class QEMUMachineAddDeviceError(QEMUMachineError):
+"""
+Exception raised when a request to add a device can not be fulfilled
+
+The failures are caused by limitations, lack of information or conflicting
+requests on the QEMUMachine methods.  This exception does not represent
+failures reported by the QEMU binary itself.
+"""
+
 class MonitorResponseError(qmp.qmp.QMPError):
 '''
 Represents erroneous QMP monitor reply
@@ -91,6 +113,10 @@ class QEMUMachine(object):
 self._test_dir = test_dir
 self._temp_dir = None
 self._launched = False
+self._machine = None
+self._console_device_type = None
+self._console_address = None
+self._console_socket = None
 
 # just in case logging wasn't configured by the main script:
 logging.basicConfig()
@@ -175,9 +201,19 @@ class QEMUMachine(object):
 self._monitor_address[1])
 else:
 moncdev = 'socket,id=mon,path=%s' % self._vm_monitor
-return ['-chardev', moncdev,
+args = ['-chardev', moncdev,
 '-mon', 'chardev=mon,mode=control',
 '-display', 'none', '-vga', 'none']
+if self._machine is not None:
+args.extend(['-machine', self._machine])
+if self._console_device_type is not None:
+self._console_address = os.path.join(self._temp_dir,
+ self._name + "-console.sock")
+chardev = ('socket,id=console,path=%s,server,nowait' %
+   self._console_address)
+device = '%s,chardev=console' % self._console_device_type
+args.extend(['-chardev', chardev, '-device', device])
+return args
 
 def _pre_launch(self):
 self._temp_dir = tempfile.mkdtemp(dir=self._test_dir)
@@ -202,6 +238,10 @@ class QEMUMachine(object):
 
 self._qemu_log_path = None
 
+if self._console_socket is not None:
+self._console_socket.close()
+self._console_socket = None
+
 if self._temp_dir is not None:
 shutil.rmtree(self._temp_dir)
 self._temp_dir = None
@@ -365,3 +405,58 @@ class QEMUMachine(object):
 Adds to the list of extra arguments to be given to the QEMU binary
 '''
 return self._args.extend(args)
+
+def set_machine(self, machine_type):
+'''
+Sets the machine type
+
+If set, the machine type will be added to the base arguments
+of the resulting QEMU command line.
+'''
+self._machine = machine_type
+
+def set_console(self, device_type=None):
+'''
+Sets the device type for a console device
+
+If set, the console device and a backing character device will
+be added to the base arguments of the resulting QEMU command
+line.
+
+This is a convenience method that will either use the provided
+device type, of if not given, it will used the device type set
+on CONSOLE_DEV_TYPES.
+
+The actual setting of command line arguments will be be done at
+machine launch time, as it depends on the temporary directory
+to be created.
+
+@param device_type: the device type, such as "isa-serial"
+@raises: QEMUMachineAddDeviceError if the device type is 

[Qemu-devel] [PATCH 3/5] Acceptance tests: add quick VNC tests

[Cleber Rosa]

This patch adds a few simple behavior tests for VNC.  These tests
introduce manipulation of the QEMUMachine arguments, by setting
the arguments, instead of adding to the existing ones.

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/test_vnc.py | 50 
 1 file changed, 50 insertions(+)
 create mode 100644 tests/acceptance/test_vnc.py

diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py
new file mode 100644
index 00..9d9a35cf55
--- /dev/null
+++ b/tests/acceptance/test_vnc.py
@@ -0,0 +1,50 @@
+from avocado_qemu import Test
+
+
+class Vnc(Test):
+"""
+:avocado: enable
+:avocado: tags=vnc,quick
+"""
+def test_no_vnc(self):
+self.vm.add_args('-nodefaults', '-S')
+self.vm.launch()
+self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
+
+def test_no_vnc_change_password(self):
+self.vm.add_args('-nodefaults', '-S')
+self.vm.launch()
+self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled'])
+set_password_response = self.vm.qmp('change',
+device='vnc',
+target='password',
+arg='new_password')
+self.assertIn('error', set_password_response)
+self.assertEqual(set_password_response['error']['class'],
+ 'GenericError')
+self.assertEqual(set_password_response['error']['desc'],
+ 'Could not set password')
+
+def test_vnc_change_password_requires_a_password(self):
+self.vm.add_args('-nodefaults', '-S', '-vnc', ':0')
+self.vm.launch()
+self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
+set_password_response = self.vm.qmp('change',
+device='vnc',
+target='password',
+arg='new_password')
+self.assertIn('error', set_password_response)
+self.assertEqual(set_password_response['error']['class'],
+ 'GenericError')
+self.assertEqual(set_password_response['error']['desc'],
+ 'Could not set password')
+
+def test_vnc_change_password(self):
+self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password')
+self.vm.launch()
+self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled'])
+set_password_response = self.vm.qmp('change',
+device='vnc',
+target='password',
+arg='new_password')
+self.assertEqual(set_password_response['return'], {})
-- 
2.17.0

[Qemu-devel] [PATCH 1/5] Add functional/acceptance tests infrastructure

[Cleber Rosa]

This patch adds the very minimum infrastructure necessary for writing
and running functional/acceptance tests, including:

 * Documentation
 * The avocado_qemu.Test base test class
 * One example tests (test_version.py)

Additional functionality is expected to be added along the tests that
require them.

Signed-off-by: Cleber Rosa 
---
 tests/acceptance/README.rst   | 141 ++
 tests/acceptance/avocado_qemu/__init__.py |  45 +++
 tests/acceptance/test_version.py  |  13 ++
 3 files changed, 199 insertions(+)
 create mode 100644 tests/acceptance/README.rst
 create mode 100644 tests/acceptance/avocado_qemu/__init__.py
 create mode 100644 tests/acceptance/test_version.py

diff --git a/tests/acceptance/README.rst b/tests/acceptance/README.rst
new file mode 100644
index 00..f1434177da
--- /dev/null
+++ b/tests/acceptance/README.rst
@@ -0,0 +1,141 @@
+==
+ Acceptance tests using the Avocado Framework
+==
+
+This directory hosts functional tests, also known as acceptance level
+tests.  They're usually higher level, and may interact with external
+resources and with various guest operating systems.
+
+The tests are written using the Avocado Testing Framework, in
+conjunction with a the ``avocado_qemu.Test`` class, distributed here.
+
+Installation
+
+
+To install Avocado and its dependencies, run::
+
+  pip install --user avocado-framework
+
+Alternatively, follow the instructions on this link:
+
+  
http://avocado-framework.readthedocs.io/en/latest/GetStartedGuide.html#installing-avocado
+
+Overview
+
+
+This directory provides the ``avocado_qemu`` Python module, containing
+the ``avocado_qemu.Test`` class.  Here's a simple usage example::
+
+  from avocado_qemu import Test
+
+
+  class Version(Test):
+  """
+  :avocado: enable
+  :avocado: tags=quick
+  """
+  def test_qmp_human_info_version(self):
+  self.vm.launch()
+  res = self.vm.command('human-monitor-command',
+command_line='info version')
+  self.assertRegexpMatches(res, r'^(\d+\.\d+\.\d)')
+
+To execute your test, run::
+
+  avocado run test_version.py
+
+To run all tests in the current directory, tagged in a particular way,
+run::
+
+  avocado run -t  .
+
+The ``avocado_qemu.Test`` base test class
+=
+
+The ``avocado_qemu.Test`` class has a number of characteristics that
+are worth being mentioned right away.
+
+First of all, it attempts to give each test a ready to use QEMUMachine
+instance, available at ``self.vm``.  Because many tests will tweak the
+QEMU command line, launching the QEMUMachine (by using ``self.vm.launch()``)
+is left to the test writer.
+
+At test "tear down", ``avocado_qemu.Test`` handles the QEMUMachine
+shutdown.
+
+QEMUMachine
+---
+
+The QEMUMachine API should be somewhat familiar to QEMU hackers.  It's
+used in the Python iotests, device-crash-test and other Python scripts.
+
+QEMU binary selection
+-
+
+The QEMU binary used for the ``self.vm`` QEMUMachine instance will
+primarily depend on the value of the ``qemu_bin`` parameter.  If it's
+not explicitly set, its default value will be the result of a dynamic
+probe in the same source tree.  A suitable binary will be one that
+targets the architecture matching host machine.
+
+Based on this description, test writers will usually rely on one of
+the following approaches:
+
+1) Set ``qemu_bin``, and use the given binary
+
+2) Do not set ``qemu_bin``, and use a QEMU binary named like
+   "${arch}-softmmu/qemu-system-${arch}", either in the current
+   working directory, or in the current source tree.
+
+The resulting ``qemu_bin`` value will be preserved in the
+``avocado_qemu.Test`` as an attribute with the same name.
+
+Attribute reference
+===
+
+Besides the attributes and methods that are part of the base
+``avocado.Test`` class, the following attributes are available on any
+``avocado_qemu.Test`` instance.
+
+vm
+--
+
+A QEMUMachine instance, initially configured according to the given
+``qemu_bin`` parameter.
+
+qemu_bin
+
+
+The preserved value of the ``qemu_bin`` parameter or the result of the
+dynamic probe for a QEMU binary in the current working directory or
+source tree.
+
+Parameter reference
+===
+
+To understand how Avocado parameters are accessed by tests, and how
+they can be passed to tests, please refer to::
+
+  
http://avocado-framework.readthedocs.io/en/latest/WritingTests.html#accessing-test-parameters
+
+Parameter values can be easily seen in the log files, and will look
+like the following::
+
+  PARAMS (key=qemu_bin, path=*, default=x86_64-softmmu/qemu-system-x86_64) => 
'x86_64-softmmu/qemu-system-x86_64
+
+qemu_bin
+
+
+The exact QEMU binary to be used on QEMUMachine.
+
+Uninstalling Avocado

[Qemu-devel] [PATCH 0/5] Acceptance/functional tests

[Cleber Rosa]

TL;DR
=

Another version, with a minimalist approach, to the acceptance tests
infrastructure for QEMU, based on the Avocado Testing Framework.

Background
==

The previous version, still considered an RFC, was sent to the list by
Eduardo[1] was based on the work held in Amador's branch[2].  After
reviewing in under a different light, including the experiences
done and reported by Philippe[3].

Differences from previous versions
==

The main difference is that this series include only the minimal
changes deemed necessary to have a starting point.  I like to think
that it's better connected to the QEMU community and project needs,
and will hopefully allow for a more organic growth.

Since this version has less features than the previous versions,
provided it's accepted, these are the next probable development tasks:

 * Provide a simple variants mechanism to allow the same tests to be
   run under different targets, machine models and devices (present on
   the previous versions as a "YAML to Mux" file with architecture
   definitions)
 * Implement QEMUMachine migration support (present on the previous
   version in the "avocado_qemu.test._VM" class)
 * Implement Guest OS image selection and download (mostly an Avocado
   feature, paired with a parameter convention and cloud-init support
   code)
 * Implement interactive support for Guest OS sessions (present on
   the previous versions, supported by the aexpect Python module)

Even though this version shares very little (if any) code with the
previous versions, the following is a list of noteworthy changes:

 * Tests directory is now "tests/acceptance" (was "tests/avocado")
 * Base test class is now "avocado_qemu.Test" (was
   "avocado_qemu.test.QemuTest")
 * Base test class is now hosted in "avocado_qemu/__init__.py" (was
   "avocado_qemu/test.py")
 * Direct use of "qemu.QEMUMachine", that is, the
   avocado_qemu.test._VM class is gone
 * avocado_qemu.Test won't search for QEMU binaries on $PATH.  To use
   QEMU binary on a custom system location it's necessary to use the
   "qemu_bin" parameter
 * Example test in README.rst is distributed as a real test
   ("test_version.py")
 * A new "Linux boot console" test, loosely modeled after Phillipe's
   use case

Commit summary
==

Cleber Rosa (5):
  Add functional/acceptance tests infrastructure
  scripts/qemu.py: allow adding to the list of extra arguments
  Acceptance tests: add quick VNC tests
  scripts/qemu.py: introduce set_console() method
  Acceptance tests: add Linux kernel boot and console checking test

 scripts/qemu.py | 103 +++-
 scripts/test_qemu.py| 176 +++
 tests/acceptance/README.rst | 141 +
 tests/acceptance/avocado_qemu/__init__.py   |  45 +++
 tests/acceptance/test_boot_linux_console.py |  37 ++
 tests/acceptance/test_version.py|  13 ++
 tests/acceptance/test_vnc.py|  50 
 7 files changed, 564 insertions(+), 1 deletion(-)
 create mode 100644 scripts/test_qemu.py
 create mode 100644 tests/acceptance/README.rst
 create mode 100644 tests/acceptance/avocado_qemu/__init__.py
 create mode 100644 tests/acceptance/test_boot_linux_console.py
 create mode 100644 tests/acceptance/test_version.py
 create mode 100644 tests/acceptance/test_vnc.py

---

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03443.html
[2] https://github.com/apahim/qemu/commits/avocado_qemu
[3] https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg03076.html

Re: [Qemu-devel] QEMU: Multiple PCI domains for x86 PCI Express Machine (Q35)

[Zihan Yang]

2018-05-24 19:54 GMT+08:00 Marcel Apfelbaum :
> Hi Aaron
>
> On 05/24/2018 12:31 PM, Wei, Aaron wrote:
>>
>>
>> Hi,  Marcel
>>
>> It's my pleasure to write to you on behalf of a team who is developing the
>> Intel VMD virtualization base on QEMU in Dell EMC.
>>
>> From https://www.outreachy.org. we find that you are working on the
>> Multiple PCI domain feature which we think may be  very helpful to our
>> current work.
>>
>
> We are glad you are considering using the project.
> The project got delayed for next "Outreachy/GSOC" round, however Zihan Yang
> ,
> a PhD student, graciously offered to try it in his private time.

Hi Marcel

Thank you for CCing and mentioning me in the email. I'm really glad to know
people are interested in the project. It is really a joy to contribute
to it. By the
way, I'm currently a master student but that doesn't matter here.

>> We’ll be grateful if you can share some information about,
>>
>> 1.What’s the present developing status?
>>
>
> An early RFC (not a working version) was posted here:
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg04664.html
> The mail thread includes an interesting discussion on issues and constrains.
>
>> 2.When would it possibly be ready? Or when could we get it from QEMU repo?
>>
>
> Since is currently developed as a side project we don't have a clear
> timeline,
> you are welcome to contribute with ideas, reviews, tests and patches :) .
> Any help is appreciated.

Hi Aaron

Thanks for your interest, I wish I could finish it as soon as
possible, but I'm afraid
I can't give any guarantee right now. As Marcel said, we don't have a
clear timeline
yet. The official project lasts 12 weeks, and I plan to finish it
before September.
The pace might be faster after June when all the exams are finished.

About the project itself, we actually just started yet. You might want
to look at the
initial patch set I submitted a few days ago, as posted by Marcel in
the last email.
These patches are just POC, asking for reviews and comments. The AML part in
QEMU and the firmware part are not implemented yet. I get many good reviews
from Marcel and other members, and am preparing for a v2 patch.

Actually, even if I finish all the work, there still might be some
time before the
patches are available in the upstream because there will be reviews
and comments,
and they are likely to go through many versions before they are merged.

>> We appreciate and  look forward to your reply.
>>
>
> I hope I helped.

I hope I helped too :)
Feel free to comment/review/email/contribute if you have any questions about
this project.

Thanks
Zihan

[Qemu-devel] [PATCH v3] block: fix QEMU crash with scsi-hd and drive_del

[Greg Kurz]

Removing a drive with drive_del while it is being used to run an I/O
intensive workload can cause QEMU to crash.

An AIO flush can yield at some point:

blk_aio_flush_entry()
 blk_co_flush(blk)
  bdrv_co_flush(blk->root->bs)
   ...
qemu_coroutine_yield()

and let the HMP command to run, free blk->root and give control
back to the AIO flush:

hmp_drive_del()
 blk_remove_bs()
  bdrv_root_unref_child(blk->root)
   child_bs = blk->root->bs
   bdrv_detach_child(blk->root)
bdrv_replace_child(blk->root, NULL)
 blk->root->bs = NULL
g_free(blk->root) <== blk->root becomes stale
   bdrv_unref(child_bs)
bdrv_delete(child_bs)
 bdrv_close()
  bdrv_drained_begin()
   bdrv_do_drained_begin()
bdrv_drain_recurse()
 aio_poll()
  ...
  qemu_coroutine_switch()

and the AIO flush completion ends up dereferencing blk->root:

  blk_aio_complete()
   scsi_aio_complete()
blk_get_aio_context(blk)
 bs = blk_bs(blk)
 ie, bs = blk->root ? blk->root->bs : NULL
^
stale

The problem is that we should avoid making block driver graph
changes while we have in-flight requests. This patch hence adds
a drained section to bdrv_detach_child(), so that we're sure
all requests have been drained before blk->root becomes stale.

Signed-off-by: Greg Kurz 
---
v3: - start drained section before modifying the graph (Stefan)

v2: - drain I/O requests when detaching the BDS (Stefan, Paolo)
---
 block.c |4 
 1 file changed, 4 insertions(+)

diff --git a/block.c b/block.c
index 501b64c8193f..715c1b56c1e2 100644
--- a/block.c
+++ b/block.c
@@ -2127,12 +2127,16 @@ BdrvChild *bdrv_attach_child(BlockDriverState 
*parent_bs,
 
 static void bdrv_detach_child(BdrvChild *child)
 {
+BlockDriverState *child_bs = child->bs;
+
+bdrv_drained_begin(child_bs);
 if (child->next.le_prev) {
 QLIST_REMOVE(child, next);
 child->next.le_prev = NULL;
 }
 
 bdrv_replace_child(child, NULL);
+bdrv_drained_end(child_bs);
 
 g_free(child->name);
 g_free(child);

Re: [Qemu-devel] [PATCH v1 15/30] RISC-V: Add hartid and \n to interrupt logging

[Alistair Francis]

On Wed, May 23, 2018 at 5:33 AM, Philippe Mathieu-Daudé  wrote:
> Hi Michael,
>
> On 05/22/2018 09:15 PM, Michael Clark wrote:
>> Add carriage return that was erroneously removed
>> when converting to qemu_log. Change hard coded
>> core number to the actual hartid.
>
> I think it makes more sens to move this patch before your 6/30 "Move
> non-ops from op_helper to cpu_helper".
>
>>
>> Cc: Sagar Karandikar 
>> Cc: Bastian Koppelmann 
>> Cc: Palmer Dabbelt 
>> Cc: Alistair Francis 
>> Signed-off-by: Michael Clark 
>
> Reviewed-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

>
>> ---
>>  target/riscv/cpu_helper.c | 18 ++
>>  1 file changed, 10 insertions(+), 8 deletions(-)
>>
>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>> index bc15e19022cc..69592c037042 100644
>> --- a/target/riscv/cpu_helper.c
>> +++ b/target/riscv/cpu_helper.c
>> @@ -446,11 +446,13 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>  if (RISCV_DEBUG_INTERRUPT) {
>>  int log_cause = cs->exception_index & RISCV_EXCP_INT_MASK;
>>  if (cs->exception_index & RISCV_EXCP_INT_FLAG) {
>> -qemu_log_mask(LOG_TRACE, "core   0: trap %s, epc 0x" 
>> TARGET_FMT_lx,
>> -riscv_intr_names[log_cause], env->pc);
>> +qemu_log_mask(LOG_TRACE, "core "
>> +TARGET_FMT_ld ": trap %s, epc 0x" TARGET_FMT_lx "\n",
>> +env->mhartid, riscv_intr_names[log_cause], env->pc);
>>  } else {
>> -qemu_log_mask(LOG_TRACE, "core   0: intr %s, epc 0x" 
>> TARGET_FMT_lx,
>> -riscv_excp_names[log_cause], env->pc);
>> +qemu_log_mask(LOG_TRACE, "core "
>> +TARGET_FMT_ld ": intr %s, epc 0x" TARGET_FMT_lx "\n",
>> +env->mhartid, riscv_excp_names[log_cause], env->pc);
>>  }
>>  }
>>
>> @@ -512,8 +514,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>
>>  if (hasbadaddr) {
>>  if (RISCV_DEBUG_INTERRUPT) {
>> -qemu_log_mask(LOG_TRACE, "core " TARGET_FMT_ld
>> -": badaddr 0x" TARGET_FMT_lx, env->mhartid, 
>> env->badaddr);
>> +qemu_log_mask(LOG_TRACE, "core " TARGET_FMT_ld ": badaddr 
>> 0x"
>> +TARGET_FMT_lx "\n", env->mhartid, env->badaddr);
>>  }
>>  env->sbadaddr = env->badaddr;
>>  } else {
>> @@ -537,8 +539,8 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>>
>>  if (hasbadaddr) {
>>  if (RISCV_DEBUG_INTERRUPT) {
>> -qemu_log_mask(LOG_TRACE, "core " TARGET_FMT_ld
>> -": badaddr 0x" TARGET_FMT_lx, env->mhartid, 
>> env->badaddr);
>> +qemu_log_mask(LOG_TRACE, "core " TARGET_FMT_ld ": badaddr 
>> 0x"
>> +TARGET_FMT_lx "\n", env->mhartid, env->badaddr);
>>  }
>>  env->mbadaddr = env->badaddr;
>>  } else {
>>
>

Re: [Qemu-devel] [PATCH v1 26/30] RISC-V: Remove unnecessary disassembler constraints

[Alistair Francis]

On Tue, May 22, 2018 at 5:15 PM, Michael Clark  wrote:
> Remove machine generated constraints that are not
> referenced by the pseudo-instruction constraints.
>
> Cc: Palmer Dabbelt 
> Cc: Sagar Karandikar 
> Cc: Bastian Koppelmann 
> Cc: Alistair Francis 
> Signed-off-by: Michael Clark 

Acked-by: Alistair Francis 

Alistair

> ---
>  disas/riscv.c | 138 
> --
>  1 file changed, 138 deletions(-)
>
> diff --git a/disas/riscv.c b/disas/riscv.c
> index 7fd1019623ee..27546dd7902c 100644
> --- a/disas/riscv.c
> +++ b/disas/riscv.c
> @@ -87,33 +87,10 @@ typedef enum {
>
>  typedef enum {
>  rvc_end,
> -rvc_simm_6,
> -rvc_imm_6,
> -rvc_imm_7,
> -rvc_imm_8,
> -rvc_imm_9,
> -rvc_imm_10,
> -rvc_imm_12,
> -rvc_imm_18,
> -rvc_imm_nz,
> -rvc_imm_x2,
> -rvc_imm_x4,
> -rvc_imm_x8,
> -rvc_imm_x16,
> -rvc_rd_b3,
> -rvc_rs1_b3,
> -rvc_rs2_b3,
> -rvc_rd_eq_rs1,
>  rvc_rd_eq_ra,
> -rvc_rd_eq_sp,
>  rvc_rd_eq_x0,
> -rvc_rs1_eq_sp,
>  rvc_rs1_eq_x0,
>  rvc_rs2_eq_x0,
> -rvc_rd_ne_x0_x2,
> -rvc_rd_ne_x0,
> -rvc_rs1_ne_x0,
> -rvc_rs2_ne_x0,
>  rvc_rs2_eq_rs1,
>  rvc_rs1_eq_ra,
>  rvc_imm_eq_zero,
> @@ -2522,111 +2499,16 @@ static bool check_constraints(rv_decode *dec, const 
> rvc_constraint *c)
>  uint8_t rd = dec->rd, rs1 = dec->rs1, rs2 = dec->rs2;
>  while (*c != rvc_end) {
>  switch (*c) {
> -case rvc_simm_6:
> -if (!(imm >= -32 && imm < 32)) {
> -return false;
> -}
> -break;
> -case rvc_imm_6:
> -if (!(imm <= 63)) {
> -return false;
> -}
> -break;
> -case rvc_imm_7:
> -if (!(imm <= 127)) {
> -return false;
> -}
> -break;
> -case rvc_imm_8:
> -if (!(imm <= 255)) {
> -return false;
> -}
> -break;
> -case rvc_imm_9:
> -if (!(imm <= 511)) {
> -return false;
> -}
> -break;
> -case rvc_imm_10:
> -if (!(imm <= 1023)) {
> -return false;
> -}
> -break;
> -case rvc_imm_12:
> -if (!(imm <= 4095)) {
> -return false;
> -}
> -break;
> -case rvc_imm_18:
> -if (!(imm <= 262143)) {
> -return false;
> -}
> -break;
> -case rvc_imm_nz:
> -if (!(imm != 0)) {
> -return false;
> -}
> -break;
> -case rvc_imm_x2:
> -if (!((imm & 0b1) == 0)) {
> -return false;
> -}
> -break;
> -case rvc_imm_x4:
> -if (!((imm & 0b11) == 0)) {
> -return false;
> -}
> -break;
> -case rvc_imm_x8:
> -if (!((imm & 0b111) == 0)) {
> -return false;
> -}
> -break;
> -case rvc_imm_x16:
> -if (!((imm & 0b) == 0)) {
> -return false;
> -}
> -break;
> -case rvc_rd_b3:
> -if (!(rd  >= 8 && rd  <= 15)) {
> -return false;
> -}
> -break;
> -case rvc_rs1_b3:
> -if (!(rs1 >= 8 && rs1 <= 15)) {
> -return false;
> -}
> -break;
> -case rvc_rs2_b3:
> -if (!(rs2 >= 8 && rs2 <= 15)) {
> -return false;
> -}
> -break;
> -case rvc_rd_eq_rs1:
> -if (!(rd == rs1)) {
> -return false;
> -}
> -break;
>  case rvc_rd_eq_ra:
>  if (!(rd == 1)) {
>  return false;
>  }
>  break;
> -case rvc_rd_eq_sp:
> -if (!(rd == 2)) {
> -return false;
> -}
> -break;
>  case rvc_rd_eq_x0:
>  if (!(rd == 0)) {
>  return false;
>  }
>  break;
> -case rvc_rs1_eq_sp:
> -if (!(rs1 == 2)) {
> -return false;
> -}
> -break;
>  case rvc_rs1_eq_x0:
>  if (!(rs1 == 0)) {
>  return false;
> @@ -2637,26 +2519,6 @@ static bool check_constraints(rv_decode *dec, const 
> rvc_constraint *c)
>  return false;
>  }
>  break;
> -case rvc_rd_ne_x0_x2:
> -if (!(rd != 0 && rd != 2)) {
> -return false;
> -}
> -break;

Re: [Qemu-devel] [PATCH v1 18/30] RISC-V: Add missing free for plic_hart_config

[Alistair Francis]

On Wed, May 23, 2018 at 5:40 AM, Philippe Mathieu-Daudé  wrote:
> On 05/22/2018 09:15 PM, Michael Clark wrote:
>> Cc: Palmer Dabbelt 
>> Cc: Sagar Karandikar 
>> Cc: Bastian Koppelmann 
>> Cc: Alistair Francis 
>> Signed-off-by: Michael Clark 
>
> Reviewed-by: Philippe Mathieu-Daudé 

Reviewed-by: Alistair Francis 

Alistair

>
>> ---
>>  hw/riscv/virt.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index ad03113e0f72..321fa6e8122a 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -385,6 +385,8 @@ static void riscv_virt_board_init(MachineState *machine)
>>  serial_mm_init(system_memory, memmap[VIRT_UART0].base,
>>  0, SIFIVE_PLIC(s->plic)->irqs[UART0_IRQ], 399193,
>>  serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> +
>> +g_free(plic_hart_config);
>>  }
>>
>>  static void riscv_virt_board_machine_init(MachineClass *mc)
>>
>

[Qemu-devel] [PATCH] gdbstub: Prevent fd leakage

[Philippe Mathieu-Daudé]

Since 2f652224f7, we now check if socket_set_nodelay() errored,
but forgot to close the socket before reporting an error.

Fixes: Coverity CID 1391290 (RESOURCE_LEAK)
Signed-off-by: Philippe Mathieu-Daudé 
---
 gdbstub.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gdbstub.c b/gdbstub.c
index e4ece2f5bc..9c860cd81c 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -1836,6 +1836,7 @@ static bool gdb_accept(void)
 /* set short latency */
 if (socket_set_nodelay(fd)) {
 perror("setsockopt");
+close(fd);
 return false;
 }
 
-- 
2.17.0

Re: [Qemu-devel] [Qemu-trivial] [PULL 20/22] gdbstub: Handle errors in gdb_accept()

[Philippe Mathieu-Daudé]

On 05/24/2018 06:35 PM, Paolo Bonzini wrote:
> On 20/05/2018 08:15, Michael Tokarev wrote:
>> From: Peter Maydell 
>>
>> In gdb_accept(), we both fail to check all errors (notably
>> that from socket_set_nodelay(), as Coverity notes in CID 1005666),
>> and fail to return an error status back to our caller. Correct
>> both of these things, so that errors in accept() result in our
>> stopping with a useful error message rather than ignoring it.
>>
>> Signed-off-by: Peter Maydell 
>> Reviewed-by: Philippe Mathieu-Daudé 
>> Reviewed-by: Thomas Huth 
>> Signed-off-by: Michael Tokarev 
>> ---
>>  gdbstub.c | 16 
>>  1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index b99980d2e2..e4ece2f5bc 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -1814,7 +1814,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>>  put_packet(s, buf);
>>  }
>>  
>> -static void gdb_accept(void)
>> +static bool gdb_accept(void)
>>  {
>>  GDBState *s;
>>  struct sockaddr_in sockaddr;
>> @@ -1826,7 +1826,7 @@ static void gdb_accept(void)
>>  fd = accept(gdbserver_fd, (struct sockaddr *), );
>>  if (fd < 0 && errno != EINTR) {
>>  perror("accept");
>> -return;
>> +return false;
>>  } else if (fd >= 0) {
>>  qemu_set_cloexec(fd);
>>  break;
>> @@ -1834,7 +1834,10 @@ static void gdb_accept(void)
>>  }
>>  
>>  /* set short latency */
>> -socket_set_nodelay(fd);
>> +if (socket_set_nodelay(fd)) {
>> +perror("setsockopt");
>> +return false;
> 
> Coverity notes that this leaks fd.

Oops didn't noticed. It this is so trivial than Coverity could directly
send the fix to the list, like modern compilers.
>> +}
>>  
>>  s = g_malloc0(sizeof(GDBState));
>>  s->c_cpu = first_cpu;
>> @@ -1843,6 +1846,7 @@ static void gdb_accept(void)
>>  gdb_has_xml = false;
>>  
>>  gdbserver_state = s;
>> +return true;
>>  }
>>  
>>  static int gdbserver_open(int port)
>> @@ -1883,7 +1887,11 @@ int gdbserver_start(int port)
>>  if (gdbserver_fd < 0)
>>  return -1;
>>  /* accept connections */
>> -gdb_accept();
>> +if (!gdb_accept()) {
>> +close(gdbserver_fd);
>> +gdbserver_fd = -1;
>> +return -1;
>> +}
>>  return 0;
>>  }

Re: [Qemu-devel] [Qemu-arm] [PATCH] arm: fix malloc type mismatch

[Philippe Mathieu-Daudé]

Cc'ing qemu-devel for patchew and co

On 05/24/2018 06:37 PM, Paolo Bonzini wrote:
> cpregs_keys is an uint32_t* so the allocation should use uint32_t.
> g_new is even better because it is type-safe.
> 
> Signed-off-by: Paolo Bonzini 

Reviewed-by: Philippe Mathieu-Daudé 

> ---
>  target/arm/gdbstub.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
> index e80cfb47c7..0c64c0292e 100644
> --- a/target/arm/gdbstub.c
> +++ b/target/arm/gdbstub.c
> @@ -157,8 +157,7 @@ int arm_gen_dynamic_xml(CPUState *cs)
>  RegisterSysregXmlParam param = {cs, s};
>  
>  cpu->dyn_xml.num_cpregs = 0;
> -cpu->dyn_xml.cpregs_keys = g_malloc(sizeof(uint32_t *) *
> -g_hash_table_size(cpu->cp_regs));
> +cpu->dyn_xml.cpregs_keys = g_new(uint32_t, 
> g_hash_table_size(cpu->cp_regs));
>  g_string_printf(s, "");
>  g_string_append_printf(s, "");
>  g_string_append_printf(s, " name=\"org.qemu.gdb.arm.sys.regs\">");
> 

[Qemu-devel] [RISC-V] Coverity 1390849, Logically dead code

[Richard Henderson]

In the latest Coverity scan, it reports

405if (csrno >= CSR_MHPMCOUNTER3 && csrno <= CSR_MHPMCOUNTER31) {
406return 0;
407}
408#if defined(TARGET_RISCV32)
409if (csrno >= CSR_MHPMCOUNTER3 && csrno <= CSR_MHPMCOUNTER31) {

CID 1390849 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach this statement: return 0U;.

410return 0;
411}
412#endif


I believe the condition at op_helper.c:409 should be testing CSR_MHPMCOUNTER3H
and CSR_MHPMCOUNTER31H.  Must run now, otherwise I'd also send the trivial 
patch.


r~

Re: [Qemu-devel] [PULL 20/22] gdbstub: Handle errors in gdb_accept()

[Paolo Bonzini]

On 20/05/2018 08:15, Michael Tokarev wrote:
> From: Peter Maydell 
> 
> In gdb_accept(), we both fail to check all errors (notably
> that from socket_set_nodelay(), as Coverity notes in CID 1005666),
> and fail to return an error status back to our caller. Correct
> both of these things, so that errors in accept() result in our
> stopping with a useful error message rather than ignoring it.
> 
> Signed-off-by: Peter Maydell 
> Reviewed-by: Philippe Mathieu-Daudé 
> Reviewed-by: Thomas Huth 
> Signed-off-by: Michael Tokarev 
> ---
>  gdbstub.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/gdbstub.c b/gdbstub.c
> index b99980d2e2..e4ece2f5bc 100644
> --- a/gdbstub.c
> +++ b/gdbstub.c
> @@ -1814,7 +1814,7 @@ void gdb_signalled(CPUArchState *env, int sig)
>  put_packet(s, buf);
>  }
>  
> -static void gdb_accept(void)
> +static bool gdb_accept(void)
>  {
>  GDBState *s;
>  struct sockaddr_in sockaddr;
> @@ -1826,7 +1826,7 @@ static void gdb_accept(void)
>  fd = accept(gdbserver_fd, (struct sockaddr *), );
>  if (fd < 0 && errno != EINTR) {
>  perror("accept");
> -return;
> +return false;
>  } else if (fd >= 0) {
>  qemu_set_cloexec(fd);
>  break;
> @@ -1834,7 +1834,10 @@ static void gdb_accept(void)
>  }
>  
>  /* set short latency */
> -socket_set_nodelay(fd);
> +if (socket_set_nodelay(fd)) {
> +perror("setsockopt");
> +return false;

Coverity notes that this leaks fd.

Paolo

> +}
>  
>  s = g_malloc0(sizeof(GDBState));
>  s->c_cpu = first_cpu;
> @@ -1843,6 +1846,7 @@ static void gdb_accept(void)
>  gdb_has_xml = false;
>  
>  gdbserver_state = s;
> +return true;
>  }
>  
>  static int gdbserver_open(int port)
> @@ -1883,7 +1887,11 @@ int gdbserver_start(int port)
>  if (gdbserver_fd < 0)
>  return -1;
>  /* accept connections */
> -gdb_accept();
> +if (!gdb_accept()) {
> +close(gdbserver_fd);
> +gdbserver_fd = -1;
> +return -1;
> +}
>  return 0;
>  }
>  
> 

Re: [Qemu-devel] [Qemu-block] Problem with data miscompare using scsi-hd, cache=none and io=threads

[Daniel Henrique Barboza]

On 05/24/2018 11:04 AM, Stefan Hajnoczi wrote:

On Tue, May 15, 2018 at 06:25:46PM -0300, Daniel Henrique Barboza wrote:

This means that the test executed a write at  LBA 0x94fa and, after
confirming that the write was completed, issue 2 reads in the same LBA to
assert the written contents and found out a mismatch.

Have you confirmed this pattern at various levels in the stack:
1. Application inside the guest (strace)
2. Guest kernel block layer (blktrace)
3. QEMU (strace)
4. Host kernel block layer (blktrace)


Tested (3). In this case the bug stop reproducing. Not sure if there is
anything related with strace adding a delay back and forth the
preadv/pwritev calls, but the act of attaching strace to the QEMU
process changed the behavior.

Haven't tried the other 3 scenarios. (2) and (4) are definitely worth 
trying it

out, specially (4).


The key thing is that the write completes before the 2 reads are
submitted.

Have you tried running the test on bare metal?


Yes. The stress test does not reproduce the miscompare issue when
running on bare metal.


Thanks,

Daniel



Stefan

Re: [Qemu-devel] [PATCH 17/27] exec.c: Handle IOMMUs in address_space_translate_for_iotlb()

[Auger Eric]

Hi Peter,

On 05/23/2018 11:51 AM, Alex Bennée wrote:
> 
> Peter Maydell  writes:
> 
>> Currently we don't support board configurations that put an IOMMU
>> in the path of the CPU's memory transactions, and instead just
>> assert() if the memory region fonud in address_space_translate_for_iotlb()
found
>> is an IOMMUMemoryRegion.
>>
>> Remove this limitation by having the function handle IOMMUs.
>> This is mostly straightforward, but we must make sure we have
>> a notifier registered for every IOMMU that a transaction has
>> passed through, so that we can flush the TLB appropriately
Can you elaborate on what (TCG) TLB we are talking about?

The concept of IOMMUs downstream to a CPU is not obvious to me. Maybe an
example may be documented in the commit message?
>> when any of the IOMMUs change their mappings.
>>
>> Signed-off-by: Peter Maydell 
>> ---
>>  include/exec/exec-all.h |   3 +-
>>  include/qom/cpu.h   |   3 +
>>  accel/tcg/cputlb.c  |   3 +-
>>  exec.c  | 147 +++-
>>  4 files changed, 152 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
>> index 4d09eaba72..e0ff19b711 100644
>> --- a/include/exec/exec-all.h
>> +++ b/include/exec/exec-all.h
>> @@ -469,7 +469,8 @@ void tb_flush_jmp_cache(CPUState *cpu, target_ulong 
>> addr);
>>
>>  MemoryRegionSection *
>>  address_space_translate_for_iotlb(CPUState *cpu, int asidx, hwaddr addr,
>> -  hwaddr *xlat, hwaddr *plen);
>> +  hwaddr *xlat, hwaddr *plen,
>> +  MemTxAttrs attrs, int *prot);
>>  hwaddr memory_region_section_get_iotlb(CPUState *cpu,
>> MemoryRegionSection *section,
>> target_ulong vaddr,
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 9d3afc6c75..d4a30149dd 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -429,6 +429,9 @@ struct CPUState {
>>  uint16_t pending_tlb_flush;
>>
>>  int hvf_fd;
>> +
>> +/* track IOMMUs whose translations we've cached in the TCG TLB */
>> +GSList *iommu_notifiers;
> 
> So we are only concerned about TCG IOMMU notifiers here, specifically
> TCGIOMMUNotifier structures. Why not just use a GArray and save
> ourselves chasing pointers?
> 
>>  };
>>
>>  QTAILQ_HEAD(CPUTailQ, CPUState);
>> diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
>> index 05439039e9..c8acaf21e9 100644
>> --- a/accel/tcg/cputlb.c
>> +++ b/accel/tcg/cputlb.c
>> @@ -632,7 +632,8 @@ void tlb_set_page_with_attrs(CPUState *cpu, target_ulong 
>> vaddr,
>>  }
>>
>>  sz = size;
>> -section = address_space_translate_for_iotlb(cpu, asidx, paddr, , 
>> );
>> +section = address_space_translate_for_iotlb(cpu, asidx, paddr, , 
>> ,
>> +attrs, );
>>  assert(sz >= TARGET_PAGE_SIZE);
>>
>>  tlb_debug("vaddr=" TARGET_FMT_lx " paddr=0x" TARGET_FMT_plx
>> diff --git a/exec.c b/exec.c
>> index c9285c9c39..6c8f2dcc3f 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -650,18 +650,158 @@ MemoryRegion *flatview_translate(FlatView *fv, hwaddr 
>> addr, hwaddr *xlat,
>>  return mr;
>>  }
>>
>> +typedef struct TCGIOMMUNotifier {
>> +IOMMUNotifier n;
>> +MemoryRegion *mr;
>> +CPUState *cpu;
> 
> This seems superfluous if we are storing the list of notifiers in the CPUState
> 
>> +int iommu_idx;
>> +bool active;
>> +} TCGIOMMUNotifier;
>> +
>> +static void tcg_iommu_unmap_notify(IOMMUNotifier *n, IOMMUTLBEntry *iotlb)
>> +{
>> +TCGIOMMUNotifier *notifier = container_of(n, TCGIOMMUNotifier, n);
>> +
>> +if (!notifier->active) {
>> +return;
>> +}
>> +tlb_flush(notifier->cpu);
>> +notifier->active = false;
>> +/* We leave the notifier struct on the list to avoid reallocating it 
>> later.
>> + * Generally the number of IOMMUs a CPU deals with will be small.
>> + * In any case we can't unregister the iommu notifier from a notify
>> + * callback.
>> + */
I don't get the life cycle of the notifier and why it becomes inactive
after the invalidate. Could you detail the specificity of this one?
>> +}
>> +
>> +static gint tcg_iommu_find_notifier(gconstpointer a, gconstpointer b)
>> +{
>> +TCGIOMMUNotifier *notifier = (TCGIOMMUNotifier *)a;
>> +TCGIOMMUNotifier *seeking = (TCGIOMMUNotifier *)b;
>> +
>> +if (notifier->mr == seeking->mr &&
>> +notifier->iommu_idx == seeking->iommu_idx) {
>> +return 0;
>> +}
>> +return 1;
>> +}
>> +
>> +static void tcg_register_iommu_notifier(CPUState *cpu,
>> +IOMMUMemoryRegion *iommu_mr,
>> +int iommu_idx)
>> +{
>> +/* Make sure this CPU has an IOMMU notifier registered for this
>> + * IOMMU/IOMMU index 

Re: [Qemu-devel] [PATCH v11 1/5] i386: Clean up cache CPUID code

[Eduardo Habkost]

On Thu, May 24, 2018 at 11:43:30AM -0400, Babu Moger wrote:
> From: Eduardo Habkost 
> 
> Always initialize CPUCaches structs with cache information, even
> if legacy_cache=true.  Use different CPUCaches struct for
> CPUID[2], CPUID[4], and the AMD CPUID leaves.
> 
> This will simplify a lot the logic inside cpu_x86_cpuid().
> 
> Signed-off-by: Eduardo Habkost 
> Signed-off-by: Babu Moger 

Queued, thanks.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/2] ARM virt: Support up to 256 PCIe buses

[Auger Eric]

On 05/24/2018 07:20 PM, Laszlo Ersek wrote:
> On 05/24/18 16:14, Ard Biesheuvel wrote:
>> On 24 May 2018 at 15:59, Laszlo Ersek  wrote:
>>> On 05/24/18 15:07, Peter Maydell wrote:
 On 24 May 2018 at 13:59, Laszlo Ersek  wrote:
> On 05/24/18 11:11, Peter Maydell wrote:
>> Won't it also break a guest which is just Linux loaded not via
>> firmware which is an aarch32 kernel without LPAE support?
>
> Does such a thing exist? (I honestly have no clue.)

 Yes, it does; LPAE isn't a mandatory kernel config option.
 This is why we have the machine 'highmem' option, so that
 we can run on those kernels by not putting anything above
 the 4G boundary. Looking back at the history on that, we
 opted at the time for "default to highmem on, and if you're
 running an non-lpae kernel you need to turn it off manually".
>>>
>>> Ah, OK, I didn't know that.
>>>
 So we can handle those kernels by just not putting ECAM
 above 4G if highmem is false.
>>>
>>> The problem is we can have a combination of 32-bit UEFI firmware (which
>>> certainly lacks LPAE) and a 32-bit kernel which supports LPAE.
>>> Previously, you wouldn't specify highmem=off, and things would just work
>>> -- the firmware would simply ignore the >=4GB MMIO aperture, and use the
>>> 32-bit MMIO aperture only (and use the sole 32-bit ECAM). The kernel
>>> could then use both low and high MMIO apertures, however (I gather?).
>>>
>>> The difference with "high ECAM" is that it is *moved* (not *added*), so
>>> the 32-bit firmware is left with nothing for config space access. For
>>> booting the same combination as above, you are suddenly forced to add
>>> highmem=off, just to keep the ECAM low -- and that, while it keeps the
>>> firmware happy, prevents the LPAE-capable kernel from using the high
>>> MMIO aperture.
>>>
>>> So I think "highmem_ecam" should be computed like this:
>>>
>>>   highmem_ecam = highmem_ecam_machtype_default &&
>>>  highmem &&
>>>  (!firmware_loaded || aarch64);
>>>
>>
>> Given that the firmware is tightly coupled to the platform, we may
>> decide not to care about ECAM for UEFI itself, and invent a secondary
>> config space access mechanism that does not consume such a huge amount
>> of address space. For instance, legacy PCI uses a pair of I/O ports
>> for this, one to set the address and one to perform the actual read or
>> write, and we could easily implement something similar (such an
>> interface is problematic in SMP context but we don't care about that
>> anyway)
>>
>> Just a thought - perhaps we don't care enough about 32-bit to go
>> through the trouble, but it would be nice if LPAE capable 32-bit
>> guests could make use of the expanded PCIe config space as well.
> 
> Under the above proposal, they could, they'd just have to be launched
> without firmware:
> 
>   highmem_ecam_machtype_default = true;
>   highmem = true;
>   firmware_loaded = false;
>   aarch64 = false;
> 
>   highmem_ecam = true &&
>  true &&
>  (!false || false);

I think we mostly care about 64b guest experience improvement here. So
personally I am fine with your proposal.

Also there is this vmalloc shortage issue, hit with aarch32 guest only,
up to now (Which I reported at the end of the cover letter). This can
cause some existing guest configs (even without FW) to not boot with the
new high ECAM region whereas it booted before. I don't know if this is
acceptable.

Thanks

Eric
> 
> I see a return to the 0xCF8/0xCFC pattern regressive; I'd rather
> restrict the large/high ECAM feature to 64-bit guests (with or without
> firmware), and to 32-bit LPAE kernels that are launched without firmware
> (which, I think, has been the case for most of their history).
> 
> Personally I don't have a stake in 32-bit ARM, so do take my opinion
> with a grain of salt. Wearing my upstream ArmVirtQemu co-maintainer hat,
> my sole 32-bit interest is in keeping command lines working, *if* they
> once worked. Not extending new QEMU features to 32-bit firmware is fine
> with me -- in fact I would value that over seeing more quirky firmware
> code just for 32-bit's sake.
> 
> Side topic: the last subcondition basically says, "IF we use firmware
> THEN the VM had better be 64-bit". This is a "logical implication":
> A-->B. The C language doesn't have an "implication operator", so I
> rewrote it equivalently with the logical negation and logical OR
> operators: A-->B is equivalent to (!A || B). (If A is true, then B must
> hold; if A is false, then B doesn't matter.)
> 
> Thanks,
> Laszlo
> 

Re: [Qemu-devel] [PATCH v11 0/5] i386: Enable TOPOEXT to support hyperthreading on AMD CPU

[Eduardo Habkost]

On Thu, May 24, 2018 at 11:51:29AM -0400, Kash Pande wrote:
> Tested-by: Kash Pande 
> 
> 
> Hopefully we can get this merged because it's taken a ridiculously long
> time with many back-and-forths for small issues that could have been put
> off til later.

I understand this is frustrating, and this is partly my fault
because I didn't detect the issues on the first versions of the
series.  Sorry for that.

I believe the patches that weren't included yet either had bugs
that had to be solved, or depended on other patches that were not
included yet.  If there are patches from other iterations of this
series where you consider the code already ready for inclusion,
please let me know.  Reviewed-by lines are welcome.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 15/27] iommu: Add IOMMU index argument to notifier APIs

[Auger Eric]

Hi Peter,
On 05/24/2018 07:03 PM, Peter Maydell wrote:
> On 24 May 2018 at 16:29, Auger Eric  wrote:
>> On 05/21/2018 04:03 PM, Peter Maydell wrote:
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>>> index f6226fb263..4e6b125add 100644
>>> --- a/include/exec/memory.h
>>> +++ b/include/exec/memory.h
>>> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry {
>>>  hwaddr   iova;
>>>  hwaddr   translated_addr;
>>>  hwaddr   addr_mask;  /* 0xfff = 4k translation */
>>> +int  iommu_idx;
>> I don't get why ne need iommu_idx field here. On translate the caller
>> has it. On notify the notifier has it?
> 
> I think this is an accidental leftover from some earlier draft;
> nothing in the patchset actually reads or writes this field, and
> it should be deleted.
> 
>>>  IOMMUAccessFlags perm;
>>>  };
>>>
> 
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 8e57265edf..fb396cf00a 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener 
>>> *listener,
>>>  if (memory_region_is_iommu(section->mr)) {
>>>  VFIOGuestIOMMU *giommu;
>>>  IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>>> +int iommu_idx;
>>>
>>>  trace_vfio_listener_region_add_iommu(iova, end);
>>>  /*
>>> @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener 
>>> *listener,
>>>  llend = int128_add(int128_make64(section->offset_within_region),
>>> section->size);
>>>  llend = int128_sub(llend, int128_one());
>>> +iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>>> +   
>>> MEMTXATTRS_UNSPECIFIED);
>> In that case VFIO ideally wants to be notified for any guest update
>> (whatever the page set) to reprogram the physical IOMMU corresponding
>> entries and doesn't want to register a notifier per iommu_idx. Also it
>> does not know which ones are supported. Is there a corresponding
>> iommu_idx value? MEMTXATTRS_ANY?
> 
> If VFIO is actually dealing with an IOMMU that needs to handle
> multiple possible transactions for different tx attributes, then
> it needs to know about all of them, because how it programs
> the physical IOMMU needs to be different for "map X to Y for
> Secure transactions" versus "map X to Y for NonSecure" (say).
> (This would require new kernel APIs, I assume.)

Hum agreed. In any case the iommu_idx must be passed to VFIO along with
the notification, either as part of the notifier itself or in the
IOMMUTLBEntry. So VFIO may need to enumerate the supported iommu_idx and
register a notifier for relevant ones.

Thanks

Eric
> 
> If, as is currently the case, the VFIO infrastructure assumes that
> the IOMMU will always translate any transaction from a device
> identically regardless of its transaction attributes, then it
> should just use MEMTXATTRS_UNSPECIFIED, and trust that the
> emulated IOMMU will translate those correctly. (There might be
> scope for VFIO checking that the IOMMU really does, eg that
> it is only using one iommu index?)
> 
> Basically, VFIO is shadowing the behaviour of the emulated
> IOMMU to reflect it into the kernel; if the IOMMU it's shadowing
> is complicated then VFIO is going to need to be similarly
> complicated, and "merge updates for different page tables
> down into one" is not going to be the right behaviour.
> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [PATCH] tcg: Fix helper function vs host abi for float16

[Richard Henderson]

On 05/24/2018 06:21 AM, Peter Maydell wrote:
> Applied to target-arm.next, thanks. Is it worth marking this as
> cc:stable?

Probably, since we've marked most of the other f16 patches for stable.


r~

Re: [Qemu-devel] [PATCH v7 1/3] qmp: adding 'wakeup-suspend-support' in query-target

[Eduardo Habkost]

On Wed, May 23, 2018 at 05:53:34PM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > On Wed, May 23, 2018 at 11:17:55AM +0200, Markus Armbruster wrote:
> >> Eduardo Habkost  writes:
> >> > On Mon, May 21, 2018 at 04:46:36PM -0300, Daniel Henrique Barboza wrote:
> > [...]
> >> >> Since no objection was made back then, this logic was put into 
> >> >> query-target
> >> >> starting
> >> >> in v2. Still, I don't have any favorites though: query-target looks ok,
> >> >> query-machine
> >> >> looks ok and a new API looks ok too. It's all about what makes (more) 
> >> >> sense
> >> >> in the
> >> >> management level, I think.
> >> >
> >> > I understand the original objection from Eric: having to add a
> >> > new command for every runtime flag we want to expose to the user
> >> > looks wrong to me.
> >> 
> >> Agreed.
> >> 
> >> > However, extending query-machines and query-target looks wrong
> >> > too, however.  query-target looks wrong because this not a
> >> > property of the target.  query-machines is wrong because this is
> >> > not a static property of the machine-type, but of the running
> >> > machine instance.
> >> 
> >> Of the two, query-machines looks less wrong.
> >> 
> >> Arguably, -no-acpi should not exist.  It's an ad hoc flag that sneakily
> >> splits a few machine types into two variants, with and without ACPI.
> >> It's silently ignored for other machine types, even APCI-capable ones.
> >> 
> >> If the machine type variants with and without ACPI were separate types,
> >> wakeup-suspend-support would be a static property of the machine type.
> >> 
> >> However, "separate types" probably doesn't scale: I'm afraid we'd end up
> >> with an undesirable number of machine types.  Avoiding that is exactly
> >> why we have machine types with configurable options.  I suspect that's
> >> how ACPI should be configured (if at all).
> >> 
> >> So, should we make -no-acpi sugar for a machine type parameter?  And
> >> then deprecate -no-acpi for good measure?
> >
> > I think we should.
> 
> Would you like to take care of it?

Adding to my TODO-list, I hope I will be able to do it before the
next release.

> 
[...]
> >
> > This isn't the first time a machine capability that seems static
> > actually depends on other configuration arguments.  We will
> > probably need to address this eventually.
> 
> Then the best time to address it is now, provided we can :)

I'm not sure this is the best time.  If libvirt only needs the
runtime value and don't need any info at query-machines time, I
think support for this on query-machines will be left unused and
they will only use the query-current-machine value.

Just giving libvirt the runtime data it wants
(query-current-machine) seems way better than requiring libvirt
to interpret a set of rules and independently calculate something
QEMU already knows.

> 
> >> Would a way to tie the property to the configuration knob help?
> >> Something like wakeup-suspend-support taking values true (supported),
> >> false (not supported), and "acpi" (supported if machine type
> >> configuration knob "acpi" is switched on).
> >> 
> >
> > I would prefer a more generic mechanism.  Maybe make
> > 'query-machines' accept a 'machine-options' argument?
> 
> This can support arbitrary configuration dependencies, unlike my
> proposal.  However, I fear combinatorial explosion would make querying
> anything but "default configuration" and "current configuration"
> impractical, and "default configuration" would be basically useless, as
> you can't predict how arguments will affect the value query-machines.
> 
> Whether this is an issue depends on how management software wants to use
> query-machines.
> 
> Whether the ability to support arbitrary configuration dependencies is a
> useful feature or an invitation to stupid stunts is another open
> question :)
> 
> Here's a synthesis of the two proposals: have query-machines spell out
> which of its results are determinate, and which configuration bits need
> to be supplied to resolve the indeterminate ones.  For machine type
> "pc-q35-*", wakeup-suspend-support would always yield true, but for
> "pc-i440fx-*" it would return true when passed an acpi: true argument,
> false when passed an acpi: false argument, and an encoding of
> "indeterminate, you need to pass an acpi argument to learn more" when
> passed no acpi argument.

I like this proposal for other query-machines fields (like bus
information), but I think doing this for wakeup-suspend-support
is overkill, based on Daniel's description of its intended usage.


> 
> I'm not saying this synthesis makes sense, I'm just exploring the design
> space.
> 
> We need input from libvirt guys.

I have the impression we need real use cases so they can evaluate
the proposal.  wakeup-suspend-support doesn't seem like a use
case that really needs support on query-machines (because we
can simply provide the data at runtime).

-- 
Eduardo

Re: [Qemu-devel] [PATCH] migration: fix exec/fd migrations

[Juan Quintela]

John Snow  wrote:
> On 05/23/2018 05:14 AM, Juan Quintela wrote:
>> Commit:
>> 
>> commit 36c2f8be2c4eb0003ac77a14910842b7ddd7337e
>> Author: Juan Quintela 
>> Date:   Wed Mar 7 08:40:52 2018 +0100
>> 
>> migration: Delay start of migration main routines
>> 
>> Missed tcp and fd transports.  This fix its.
>> 
>> Reported-by: Kevin Wolf 
>> Signed-off-by: Juan Quintela 
>
> Fixes things for me, but I see that Peter Xu has more concerns.
>
> Would be happy with checking this in for now and following up with
> better refactors so that iotests works again in the meantime.
>
> Tested-by: John Snow 

That is my plan.  Will send a pull request Tomorrow, and we can go from
there.  Peter suggestion is good but requires developtemt and testing.

Thanks, Juan.

Re: [Qemu-devel] [PATCH v2 35/40] job: Add JOB_STATUS_CHANGE QMP event

[John Snow]

On 05/24/2018 02:22 PM, Eric Blake wrote:
> On 05/24/2018 12:36 PM, John Snow wrote:
> 
 On 05/18/2018 09:21 AM, Kevin Wolf wrote:
> This adds a QMP event that is emitted whenever a job transitions from
> one status to another.
>
> Signed-off-by: Kevin Wolf 
> 
>>>
>>> The question that I was asking myself was just whether I'd literally
>>> duplicate the existing events, just with s/BLOCK_JOB_/JOB_/, or whether
>>> a single event with a status field can do. I think the latter is more
>>> elegant (also because it can be implemented in a single place), even if
>>> it is emitted a bit more often than strictly necessary.
>>>
>>
>> Code-wise I agree that this is more elegant; just wondering if the
>> implications of the additional API guarantees were considered. This
>> means we have to be a bit more careful about how we restructure the
>> state machine in the future, I think.
>>
>> It also makes the "NULL" state visible, which I didn't really intend...
>> It's probably fine, just a little quirky.
> 
> Is it worth a special case to avoid emitting the event on transition to
> the NULL state?
> 

I would have done it, but it also doesn't necessarily hurt anything to
have it in here either.

Maybe it provides a benefit: I suppose it would definitely indicate to a
client that -- regardless of how they configured the job (with
auto-dismiss or not) that it serves as final record that the job has
*definitely absolutely gone* and can no longer be addressed.

It's just a strange state name for that purpose; I simply didn't
*intend* to expose it.

...OTOH, for early failures it seems like an obviously spurious message
that we don't need or want.

Well, glad I can give you precisely conflicting feelings on it and
arrive at no opinion. Good job everyone, see you tomorrow.

--js

[Qemu-devel] [PATCH v4 3/3] libqtest: add more exit status checks

[Michael S. Tsirkin]

Add more checks on how did QEMU exit.

Legal ways to exit right now:
- exit(0) or return from main
- kill(SIGTERM) - sent by testing infrastructure

Signed-off-by: Michael S. Tsirkin 
---
 tests/libqtest.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index f869854..36ca859 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -109,9 +109,19 @@ static void kill_qemu(QTestState *s)
 kill(s->qemu_pid, SIGTERM);
 pid = waitpid(s->qemu_pid, , 0);
 
-if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
+/* waitpid returns child PID on success */
+assert(pid == s->qemu_pid);
+
+/* If exited on signal - check the reason: core dump is never OK */
+if (WIFSIGNALED(wstatus)) {
 assert(!WCOREDUMP(wstatus));
 }
+/* If exited normally - check exit status */
+if (WIFEXITED(wstatus)) {
+assert(!WEXITSTATUS(wstatus));
+}
+/* Valid ways to exit: right now only return from main or exit */
+assert(WIFEXITED(wstatus));
 }
 }
 
-- 
MST

Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state

[Eduardo Habkost]

On Thu, May 24, 2018 at 08:16:20PM +0200, Markus Armbruster wrote:
> Markus Armbruster  writes:
> 
> > Igor Mammedov  writes:
> >
> >> Ban it for now, if someone would need it to work early,
> >> one would have to implement checks if HMP command is valid
> >> at preconfig state.
> >>
> >> Signed-off-by: Igor Mammedov 
> >> Reviewed-by: Eric Blake 
> >> ---
> >> v5:
> >>   * add 'use QMP instead" to error message, suggesting user
> >> the right interface to use
> >> v4:
> >>   * v3 was only printing error but not preventing command execution,
> >> Fix it by returning after printing error message.
> >> ("Dr. David Alan Gilbert" )
> >> ---
> >>  monitor.c | 6 ++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/monitor.c b/monitor.c
> >> index 39f8ee1..0ffdf1d 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -3374,6 +3374,12 @@ static void handle_hmp_command(Monitor *mon, const 
> >> char *cmdline)
> >>  
> >>  trace_handle_hmp_command(mon, cmdline);
> >>  
> >> +if (runstate_check(RUN_STATE_PRECONFIG)) {
> >> +monitor_printf(mon, "HMP not available in preconfig state, "
> >> +"use QMP instead\n");
> >> +return;
> >> +}
> >> +
> >>  cmd = monitor_parse_command(mon, cmdline, , mon->cmd_table);
> >>  if (!cmd) {
> >>  return;
> >
> > So we offer the user an HMP monitor, but we summarily fail all commands.
> > I'm sorry, but that's... searching for polite word... embarrassing.  We
> > should accept HMP output only when we're ready to accept it.  Yes, that
> > would involve a bit more surgery rather than this cheap hack.  The whole
> > preconfig thing smells like a cheap hack to me, but let's not overdo it.
> 
> Clarification: I don't think we need to hold the series because of
> this.  I do think you should investigate delaying HMP until it can work.

What would it mean to delay HMP?  Not creating the socket?
Creating the socket but not accepting clients?  Accepting clients
but not consuming any input from the socket until we are out of
preconfig?

I'm not sure if any of those options would be better.  If a human
is already trying to talk on the other side, it seems better to
show QEMU is alive (but not ready to hold a conversation yet)
than staying silent.

-- 
Eduardo

[Qemu-devel] [PATCH v4 2/3] libqtest: fail if child coredumps

[Michael S. Tsirkin]

Right now tests report OK status if QEMU crashes during cleanup.
Let's catch that case and fail the test.

Signed-off-by: Michael S. Tsirkin 
---
 tests/libqtest.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index 43fb97e..f869854 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -103,8 +103,15 @@ static int socket_accept(int sock)
 static void kill_qemu(QTestState *s)
 {
 if (s->qemu_pid != -1) {
+int wstatus = 0;
+pid_t pid;
+
 kill(s->qemu_pid, SIGTERM);
-waitpid(s->qemu_pid, NULL, 0);
+pid = waitpid(s->qemu_pid, , 0);
+
+if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
+assert(!WCOREDUMP(wstatus));
+}
 }
 }
 
-- 
MST

[Qemu-devel] [PATCH v4 0/3] libqtest: verify QEMU exit status

[Michael S. Tsirkin]

Whenever QEMU coredumps, the test would previously succeed.
With this patchset applied, one sees:
assertion failed: !WCOREDUMP(wstatus)

Changes from v3:
- add osdep stubs for non-linux platforms, suggested by Thomas

Changes from v2:
- bugfix
- assert returned pid
- rework complex asserts for clarity

Changes from v1:
- drop SIGTERM as suggested by Eric

Michael S. Tsirkin (3):
  osdep: add wait.h compat macros
  libqtest: fail if child coredumps
  libqtest: add more exit status checks

 include/qemu/osdep.h | 10 ++
 tests/libqtest.c | 19 ++-
 2 files changed, 28 insertions(+), 1 deletion(-)

-- 
MST

[Qemu-devel] [PATCH v4 1/3] osdep: add wait.h compat macros

[Michael S. Tsirkin]

Man page for WCOREDUMP says:

  WCOREDUMP(wstatus) returns true if the child produced a core dump.
  This macro should be employed only if WIFSIGNALED returned true.

  This  macro  is  not  specified  in POSIX.1-2001 and is not
  available on some UNIX implementations (e.g., AIX, SunOS).  Therefore,
  enclose its use inside #ifdef WCOREDUMP ... #endif.

Let's do exactly this.

Signed-off-by: Michael S. Tsirkin 
---
 include/qemu/osdep.h | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 4165806..afc28e5 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -108,6 +108,16 @@ extern int daemon(int, int);
 #include "qemu/typedefs.h"
 
 /*
+ * According to waitpid man page:
+ * WCOREDUMP
+ *  This  macro  is  not  specified  in POSIX.1-2001 and is not
+ *  available on some UNIX implementations (e.g., AIX, SunOS).
+ *  Therefore, enclose its use inside #ifdef WCOREDUMP ... #endif.
+ */
+#ifndef WCOREDUMP
+#define WCOREDUMP(status) 0
+#endif
+/*
  * We have a lot of unaudited code that may fail in strange ways, or
  * even be a security risk during migration, if you disable assertions
  * at compile-time.  You may comment out these safety checks if you
-- 
MST

Re: [Qemu-devel] [PATCH v2 35/40] job: Add JOB_STATUS_CHANGE QMP event

[Eric Blake]

On 05/24/2018 12:36 PM, John Snow wrote:


On 05/18/2018 09:21 AM, Kevin Wolf wrote:

This adds a QMP event that is emitted whenever a job transitions from
one status to another.

Signed-off-by: Kevin Wolf 




The question that I was asking myself was just whether I'd literally
duplicate the existing events, just with s/BLOCK_JOB_/JOB_/, or whether
a single event with a status field can do. I think the latter is more
elegant (also because it can be implemented in a single place), even if
it is emitted a bit more often than strictly necessary.



Code-wise I agree that this is more elegant; just wondering if the
implications of the additional API guarantees were considered. This
means we have to be a bit more careful about how we restructure the
state machine in the future, I think.

It also makes the "NULL" state visible, which I didn't really intend...
It's probably fine, just a little quirky.


Is it worth a special case to avoid emitting the event on transition to 
the NULL state?


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

Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks

[Michael S. Tsirkin]

On Thu, May 24, 2018 at 01:16:24PM -0500, Eric Blake wrote:
> On 05/24/2018 11:01 AM, Michael S. Tsirkin wrote:
> > On Thu, May 24, 2018 at 11:00:19AM -0500, Eric Blake wrote:
> > > On 05/24/2018 10:52 AM, Eric Blake wrote:
> > > 
> > > > Also, since waitpid() can only return either s->qemu_pid or -1 as we
> > > > aren't using WNOHANG, it may also be worth asserting that if pid == -1,
> > > > we either have EAGAIN (but why aren't we looping in that case?) or
> > > > ECHILD.
> > > 
> > > I meant EINTR, not EAGAIN.  But in general, using waitpid() to collect
> > > process status without doing it in a loop is risky.
> > 
> > Interesting. Risky how?
> 
> If your process has any signal handler installed, then an EINTR failure
> means you interpret a transient failure to grab process status (because your
> check was interrupted by something else) as a permanent failure, unless you
> go back to another waitpid() in a loop.

I don't think we have a handler installed, though.

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

Re: [Qemu-devel] [PATCH] linux-user: Remove extra mapping

[Laurent Vivier]

Le 16/05/2018 à 16:47, Steve Mcpolin a écrit :
> When a guest mmap()'d a file, a transient MAP_ANONYMOUS mapping was
> created, which required the kernel to reserve this memory, then
> subsequently released by applying a mapping with just the requested
> flags and fd.
> This transient mapping causes spurious failures when the available
> memory is smaller than the mapping.  This patch avoids this transient
> mapping.
> 
> Signed-off-by: Steve Mcpolin 
> ---
>  linux-user/mmap.c | 31 +++
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/linux-user/mmap.c b/linux-user/mmap.c
> index 9168a20..f91841f 100644
> --- a/linux-user/mmap.c
> +++ b/linux-user/mmap.c
> @@ -453,21 +453,28 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, 
> int prot,
>  /* Note: we prefer to control the mapping address. It is
> especially important if qemu_host_page_size >
> qemu_real_host_page_size */
> -p = mmap(g2h(start), host_len, prot,
> - flags | MAP_FIXED | MAP_ANONYMOUS, -1, 0);
> -if (p == MAP_FAILED)
> -goto fail;
> -/* update start so that it points to the file position at 'offset' */
> -host_start = (unsigned long)p;
> -if (!(flags & MAP_ANONYMOUS)) {
> -p = mmap(g2h(start), len, prot,
> +if (flags & MAP_ANONYMOUS) {
> +offset = 0;
> +host_offset = 0;
> +fd = -1;
> +}
> +p = mmap(g2h(start), len, prot,
>   flags | MAP_FIXED, fd, host_offset);
> -if (p == MAP_FAILED) {
> -munmap(g2h(start), host_len);
> -goto fail;
> +host_start = (uintptr_t)p;
> +if (p != MAP_FAILED && host_len > HOST_PAGE_ALIGN(len)) {
> +void *q;
> +q = mmap(g2h(start + len), host_len - HOST_PAGE_ALIGN(len),

I think you should use REAL_HOST_PAGE_ALIGN(len) above.

This is the page size used by the host mmap() (it comes from
getpagesize()), whereas HOST_PAGE_ALIGN() can come from the command line
arguments.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks

[Eric Blake]

On 05/24/2018 11:01 AM, Michael S. Tsirkin wrote:

On Thu, May 24, 2018 at 11:00:19AM -0500, Eric Blake wrote:

On 05/24/2018 10:52 AM, Eric Blake wrote:


Also, since waitpid() can only return either s->qemu_pid or -1 as we
aren't using WNOHANG, it may also be worth asserting that if pid == -1,
we either have EAGAIN (but why aren't we looping in that case?) or
ECHILD.


I meant EINTR, not EAGAIN.  But in general, using waitpid() to collect
process status without doing it in a loop is risky.


Interesting. Risky how?


If your process has any signal handler installed, then an EINTR failure 
means you interpret a transient failure to grab process status (because 
your check was interrupted by something else) as a permanent failure, 
unless you go back to another waitpid() in a loop.


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

Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state

[Markus Armbruster]

Markus Armbruster  writes:

> Igor Mammedov  writes:
>
>> Ban it for now, if someone would need it to work early,
>> one would have to implement checks if HMP command is valid
>> at preconfig state.
>>
>> Signed-off-by: Igor Mammedov 
>> Reviewed-by: Eric Blake 
>> ---
>> v5:
>>   * add 'use QMP instead" to error message, suggesting user
>> the right interface to use
>> v4:
>>   * v3 was only printing error but not preventing command execution,
>> Fix it by returning after printing error message.
>> ("Dr. David Alan Gilbert" )
>> ---
>>  monitor.c | 6 ++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/monitor.c b/monitor.c
>> index 39f8ee1..0ffdf1d 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -3374,6 +3374,12 @@ static void handle_hmp_command(Monitor *mon, const 
>> char *cmdline)
>>  
>>  trace_handle_hmp_command(mon, cmdline);
>>  
>> +if (runstate_check(RUN_STATE_PRECONFIG)) {
>> +monitor_printf(mon, "HMP not available in preconfig state, "
>> +"use QMP instead\n");
>> +return;
>> +}
>> +
>>  cmd = monitor_parse_command(mon, cmdline, , mon->cmd_table);
>>  if (!cmd) {
>>  return;
>
> So we offer the user an HMP monitor, but we summarily fail all commands.
> I'm sorry, but that's... searching for polite word... embarrassing.  We
> should accept HMP output only when we're ready to accept it.  Yes, that
> would involve a bit more surgery rather than this cheap hack.  The whole
> preconfig thing smells like a cheap hack to me, but let's not overdo it.

Clarification: I don't think we need to hold the series because of
this.  I do think you should investigate delaying HMP until it can work.

Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks

[Michael S. Tsirkin]

On Thu, May 24, 2018 at 07:58:06PM +0200, Thomas Huth wrote:
> On 24.05.2018 19:33, Michael S. Tsirkin wrote:
> > On Thu, May 24, 2018 at 07:26:16PM +0200, Thomas Huth wrote:
> >> On 24.05.2018 18:11, Michael S. Tsirkin wrote:
> >>> Add more checks on how did QEMU exit.
> >>>
> >>> Legal ways to exit right now:
> >>> - exit(0) or return from main
> >>> - kill(SIGTERM) - sent by testing infrastructure
> >>>
> >>> Anything else is illegal.
> >> [...]
> >>> -if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
> >>> +/* waitpid returns child PID on success */
> >>> +assert(pid == s->qemu_pid);
> >>> +
> >>> +/* If exited on signal - check the reason: core dump is never OK 
> >>> */
> >>> +if (WIFSIGNALED(wstatus)) {
> >>>  assert(!WCOREDUMP(wstatus));
> >>>  }
> >>> +/* If exited normally - check exit status */
> >>> +if (WIFEXITED(wstatus)) {
> >>> +assert(!WEXITSTATUS(wstatus));
> >>> +}
> >>> +/* Valid ways to exit: right now only return from main or exit */
> >>> +assert(WIFEXITED(wstatus));
> >>>  }
> >>>  }
> >>
> >> It's strange that you always get WIFEXITED(wstatus) == true here, even
> >> if QEMU has been terminated by SIGTERM? I assume that's due to the fact
> >> that QEMU intercepts SIGTERM and terminates via exit() instead?
> > 
> > Right now, yes. This can of course change, so it's not
> > a good idea hard-coding this assumption to deep
> > in the code, imho.
> > 
> >> So I
> >> think you could simply replace the last three asserts with:
> >>
> >>assert(WIFEXITED(wstatus) && !WEXITSTATUS(wstatus));
> >>
> >>  Thomas
> > 
> > I could but they would be harder to debug.
> > 
> > If I see
> > "assertion failed: !WCOREDUMP(wstatus)"
> > then that is very readable.
> > 
> > If I just see
> > "assertion failed: WIFEXITED(wstatus) && !WEXITSTATUS(wstatus)"
> > then I just know something went wrong.
> 
> Then simply use:
> 
>   assert(WIFEXITED(wstatus));
>   assert(!WEXITSTATUS(wstatus));
> 
> If QEMU exited due to a signal, you'll see the first assert, and if it
> returned a non-zero exit code, you'll see the second assert. That's all
> you really need to know here, I think.
> 
> I don't think that you gain anything by checking WCOREDUMP() here.
> And
> according to the man-page of waitpid:
> 
>  This macro is not specified in POSIX.1-2001 and is not
>  available on some UNIX implementations (e.g., AIX, SunOS).
>  Only use this enclosed in #ifdef WCOREDUMP ... #endif.
> 
> So if you insist on using that macro, you might need to add some #ifdef
> code around it, I think.
> 
>  Thomas

I think we are better off defining it if it's not defined.
Will post an osdep patch.

-- 
MST

Re: [Qemu-devel] [PATCH] prep: fix keyboard for the 40p machine

[Hervé Poussineau]

Le 24/05/2018 à 07:39, Mark Cave-Ayland a écrit :

Commit 72d3d8f052 "hw/isa/superio: Add a keyboard/mouse controller (8042)"
added an 8042 keyboard device to the PC87312 superio device to replace that
being used by the prep machine.

Unfortunately this commit didn't do the same for the 40p machine which broke
the keyboard by registering two 8042 keyboard devices at the same address.

Resolve this by similarly removing the 8042 keyboard from the 40p machine as
done for the prep machine in commit 72d3d8f052.

Signed-off-by: Mark Cave-Ayland 


Reviewed-by: Hervé Poussineau 


---
  hw/ppc/prep.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index a1e7219db6..be4db6a687 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -770,7 +770,6 @@ static void ibm_40p_init(MachineState *machine)
  
  /* add some more devices */

  if (defaults_enabled()) {
-isa_create_simple(isa_bus, TYPE_I8042);
  m48t59 = NVRAM(isa_create_simple(isa_bus, "isa-m48t59"));
  
  dev = DEVICE(isa_create(isa_bus, "cs4231a"));

Re: [Qemu-devel] [Qemu-ppc] [PATCH] prep: fix keyboard for the 40p machine

[Thomas Huth]

On 24.05.2018 18:20, Philippe Mathieu-Daudé wrote:
> On 05/24/2018 02:39 AM, Mark Cave-Ayland wrote:
>> Commit 72d3d8f052 "hw/isa/superio: Add a keyboard/mouse controller (8042)"
>> added an 8042 keyboard device to the PC87312 superio device to replace that
>> being used by the prep machine.
>>
>> Unfortunately this commit didn't do the same for the 40p machine which broke
>> the keyboard by registering two 8042 keyboard devices at the same address.
[...]
> Thanks for fixing this (too bad there are no keyboard qtests and this
> got unnoticed).

Then it's time to write some tests? ;-)

 Thomas



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH v3 2/2] vfio-ccw: remove orb.c64 (64 bit data addresses) check

[Halil Pasic]

The vfio-ccw module does the check too, and there is actually no
technical obstacle for supporting fmt 1 idaws. Let us be ready for the
beautiful day when fmt 1 idaws become supported by the vfio-ccw kernel
module. QEMU does not have to do a thing for that, except not insisting
on this check.

Signed-off-by: Halil Pasic 
Acked-by: Jason J. Herne 
Tested-by: Jason J. Herne 
---
 hw/s390x/css.c | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 39ae5bbf7e..5424ea4562 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1199,17 +1199,6 @@ static IOInstEnding 
sch_handle_start_func_passthrough(SubchDev *sch)
 assert(orb != NULL);
 p->intparm = orb->intparm;
 }
-
-/*
- * Only support prefetch enable mode.
- * Only support 64bit addressing idal.
- */
-if (!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
-warn_report("vfio-ccw requires PFCH and C64 flags set");
-sch_gen_unit_exception(sch);
-css_inject_io_interrupt(sch);
-return IOINST_CC_EXPECTED;
-}
 return s390_ccw_cmd_request(sch);
 }
 
-- 
2.16.3

[Qemu-devel] [PATCH v3 1/2] vfio-ccw: add force unlimited prefetch property

[Halil Pasic]

There is at least one guest (OS) such that although it does not rely on
the guarantees provided by ORB 1 word 9 bit (aka unlimited prefetch, aka
P bit) not being set, it fails to tell this to the machine.

Usually this ain't a big deal, as the original purpose of the P bit is to
allow for performance optimizations. vfio-ccw however can not provide the
guarantees required if the bit is not set.

It is not possible to implement support for the P bit not set without
transitioning to lower level protocols for vfio-ccw.  So let's give the
user the opportunity to force setting the P bit, if the user knows this
is safe.  For self modifying channel programs forcing the P bit is not
safe.  If the P bit is forced for a self modifying channel program things
are expected to break in strange ways.

Let's also avoid warning multiple about P bit not set in the ORB in case
P bit is not told to be forced, and designate the affected vfio-ccw
device.

Signed-off-by: Halil Pasic 
Suggested-by: Dong Jia Shi 
Acked-by: Jason J. Herne 
Tested-by: Jason J. Herne 
---

@Jason:
I have kept the tags this time without consulting you because
all that changed is the logging. Scream if that's not OK with you.

v2->v3:
* tweak commit message (Connie)
* designate subsystem (vfio-ccw) and devno in the log messages (Connie)
* turn WARN_ONCE macro into inline function

---
 hw/s390x/css.c |  3 +--
 hw/vfio/ccw.c  | 35 +++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 56c3fa8c89..39ae5bbf7e 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1204,8 +1204,7 @@ static IOInstEnding 
sch_handle_start_func_passthrough(SubchDev *sch)
  * Only support prefetch enable mode.
  * Only support 64bit addressing idal.
  */
-if (!(orb->ctrl0 & ORB_CTRL0_MASK_PFCH) ||
-!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
+if (!(orb->ctrl0 & ORB_CTRL0_MASK_C64)) {
 warn_report("vfio-ccw requires PFCH and C64 flags set");
 sch_gen_unit_exception(sch);
 css_inject_io_interrupt(sch);
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index e67392c5f9..ebf471a310 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -32,8 +32,30 @@ typedef struct VFIOCCWDevice {
 uint64_t io_region_offset;
 struct ccw_io_region *io_region;
 EventNotifier io_notifier;
+bool force_orb_pfch;
+bool warned_orb_pfch;
 } VFIOCCWDevice;
 
+static inline void warn_once(bool *warned, const char *fmt, ...)
+{
+va_list ap;
+
+if (!warned || *warned) {
+return;
+}
+*warned = true;
+va_start(ap, fmt);
+warn_vreport(fmt, ap);
+va_end(ap);
+}
+
+static inline void warn_once_pfch(VFIOCCWDevice *vcdev, SubchDev *sch,
+  const char *msg)
+{
+warn_once(>warned_orb_pfch, "vfio-ccw (devno %x.%x.%04x): %s",
+  sch->cssid, sch->ssid, sch->devno, msg);
+}
+
 static void vfio_ccw_compute_needs_reset(VFIODevice *vdev)
 {
 vdev->needs_reset = false;
@@ -54,6 +76,18 @@ static IOInstEnding vfio_ccw_handle_request(SubchDev *sch)
 struct ccw_io_region *region = vcdev->io_region;
 int ret;
 
+if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
+if (!(vcdev->force_orb_pfch)) {
+warn_once_pfch(vcdev, sch, "requires PFCH flag set");
+sch_gen_unit_exception(sch);
+css_inject_io_interrupt(sch);
+return IOINST_CC_EXPECTED;
+} else {
+sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
+warn_once_pfch(vcdev, sch, "PFCH flag forced");
+}
+}
+
 QEMU_BUILD_BUG_ON(sizeof(region->orb_area) != sizeof(ORB));
 QEMU_BUILD_BUG_ON(sizeof(region->scsw_area) != sizeof(SCSW));
 QEMU_BUILD_BUG_ON(sizeof(region->irb_area) != sizeof(IRB));
@@ -429,6 +463,7 @@ static void vfio_ccw_unrealize(DeviceState *dev, Error 
**errp)
 
 static Property vfio_ccw_properties[] = {
 DEFINE_PROP_STRING("sysfsdev", VFIOCCWDevice, vdev.sysfsdev),
+DEFINE_PROP_BOOL("force-orb-pfch", VFIOCCWDevice, force_orb_pfch, false),
 DEFINE_PROP_END_OF_LIST(),
 };
 
-- 
2.16.3

[Qemu-devel] [PATCH v3 0/2] vfio-ccw: loosen orb flags checks

[Halil Pasic]

See the individual patches (inclusive change log).

Halil Pasic (2):
  vfio-ccw: add force unlimited prefetch property
  vfio-ccw: remove orb.c64 (64 bit data addresses) check

 hw/s390x/css.c | 12 
 hw/vfio/ccw.c  | 35 +++
 2 files changed, 35 insertions(+), 12 deletions(-)

-- 
2.16.3

Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks

[Thomas Huth]

On 24.05.2018 19:33, Michael S. Tsirkin wrote:
> On Thu, May 24, 2018 at 07:26:16PM +0200, Thomas Huth wrote:
>> On 24.05.2018 18:11, Michael S. Tsirkin wrote:
>>> Add more checks on how did QEMU exit.
>>>
>>> Legal ways to exit right now:
>>> - exit(0) or return from main
>>> - kill(SIGTERM) - sent by testing infrastructure
>>>
>>> Anything else is illegal.
>> [...]
>>> -if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
>>> +/* waitpid returns child PID on success */
>>> +assert(pid == s->qemu_pid);
>>> +
>>> +/* If exited on signal - check the reason: core dump is never OK */
>>> +if (WIFSIGNALED(wstatus)) {
>>>  assert(!WCOREDUMP(wstatus));
>>>  }
>>> +/* If exited normally - check exit status */
>>> +if (WIFEXITED(wstatus)) {
>>> +assert(!WEXITSTATUS(wstatus));
>>> +}
>>> +/* Valid ways to exit: right now only return from main or exit */
>>> +assert(WIFEXITED(wstatus));
>>>  }
>>>  }
>>
>> It's strange that you always get WIFEXITED(wstatus) == true here, even
>> if QEMU has been terminated by SIGTERM? I assume that's due to the fact
>> that QEMU intercepts SIGTERM and terminates via exit() instead?
> 
> Right now, yes. This can of course change, so it's not
> a good idea hard-coding this assumption to deep
> in the code, imho.
> 
>> So I
>> think you could simply replace the last three asserts with:
>>
>>  assert(WIFEXITED(wstatus) && !WEXITSTATUS(wstatus));
>>
>>  Thomas
> 
> I could but they would be harder to debug.
> 
> If I see
>   "assertion failed: !WCOREDUMP(wstatus)"
> then that is very readable.
> 
> If I just see
>   "assertion failed: WIFEXITED(wstatus) && !WEXITSTATUS(wstatus)"
> then I just know something went wrong.

Then simply use:

assert(WIFEXITED(wstatus));
assert(!WEXITSTATUS(wstatus));

If QEMU exited due to a signal, you'll see the first assert, and if it
returned a non-zero exit code, you'll see the second assert. That's all
you really need to know here, I think.

I don't think that you gain anything by checking WCOREDUMP() here. And
according to the man-page of waitpid:

 This macro is not specified in POSIX.1-2001 and is not
 available on some UNIX implementations (e.g., AIX, SunOS).
 Only use this enclosed in #ifdef WCOREDUMP ... #endif.

So if you insist on using that macro, you might need to add some #ifdef
code around it, I think.

 Thomas

Re: [Qemu-devel] [PATCH] migration: fix exec/fd migrations

[John Snow]

On 05/23/2018 05:14 AM, Juan Quintela wrote:
> Commit:
> 
> commit 36c2f8be2c4eb0003ac77a14910842b7ddd7337e
> Author: Juan Quintela 
> Date:   Wed Mar 7 08:40:52 2018 +0100
> 
> migration: Delay start of migration main routines
> 
> Missed tcp and fd transports.  This fix its.
> 
> Reported-by: Kevin Wolf 
> Signed-off-by: Juan Quintela 

Fixes things for me, but I see that Peter Xu has more concerns.

Would be happy with checking this in for now and following up with
better refactors so that iotests works again in the meantime.

Tested-by: John Snow 

Re: [Qemu-devel] [PATCH v2 17/40] job: Move BlockJobCreateFlags to Job

[John Snow]

On 05/24/2018 04:17 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 00:24 hat John Snow geschrieben:
>>
>>
>> On 05/18/2018 09:20 AM, Kevin Wolf wrote:
>>> +job->auto_finalize = !(flags & JOB_MANUAL_FINALIZE);
>>> +job->auto_dismiss  = !(flags & JOB_MANUAL_DISMISS);
>>
>> Job API might be a good chance to say "No, this is the default behavior
>> for this API."
>>
>> I don't know how possible this is, but could we remove these behavior
>> flags for jobs (but keep them for block jobs), and then any legacy block
>> job creation interfaces we have can enable/disable them as the user
>> requested,
>>
>> and the block job layer itself has hooks that persuade the core job
>> layer to automatically transition without user input, if appropriate.
>>
>> (Unless that happens later?)
> 
> That's really a question for job-create, which we don't have yet. I'm
> not sure whether job-create would expose those options, and if so, what
> the defaults would be.
> 
> I think auto-dismiss=false is a good default because the reason for
> having a manual job-dismiss is shared by all jobs (you want to be able
> to query the job result after it finished).
> 

Sounds good to me.

> auto-finalize is different. We introduced it to avoid surprise graph
> changes, but this implies that every job changes the block graph when it
> completes. This is obviously not true for non-block jobs. They might
> still commonly change something else that shouldn't be done as a
> surprise, but I am working right now on at least a single job type that
> doesn't do anything like that on completion (blockdev-create).
> 

I'll have to stay tuned.

> For now, I made it unconditionally auto-finalize=true and
> auto-dismiss=false without offering the options in QMP, but that's open
> for discussion.

Sure. I was wondering if we didn't want to change the internal API's
defaults around, but I guess that can happen whenever based on what the
QMP looks like.

> 
> Kevin
> 

Re: [Qemu-devel] [PATCH v2 21/40] job: Convert block_job_cancel_async() to Job

[John Snow]

On 05/24/2018 04:24 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 01:18 hat John Snow geschrieben:
>>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>>> index 3e817beee9..2648c74281 100644
>>> --- a/include/qemu/job.h
>>> +++ b/include/qemu/job.h
>>> @@ -97,6 +97,12 @@ typedef struct Job {
>>>   */
>>>  bool cancelled;
>>>  
>>> +/**
>>> + * Set to true if the job should abort immediately without waiting
>>> + * for data to be in sync.
>>> + */
>>> +bool force_cancel;
>>> +
>>
>> Does this comment need an update now, though?
>>
>> Actually, in terms of "new jobs" API, it'd be really nice if cancel
>> *always meant cancel*.
>>
>> I think "cancel" should never be used to mean "successful completion,
>> but different from the one we'd get if we used job_complete."
>>
>> i.e., either we need a job setting:
>>
>> job-set completion-mode=[pivot|no-pivot]
>>
>> or optional parameters to pass to job-complete:
>>
>> job-complete mode=[understood-by-job-type]
>>
>> or some other mechanism that accomplishes the same type of behavior. It
>> would be nice if it did not have to be determined at job creation time
>> but instead could be determined later.
> 
> I agree. We already made sure that job-cancel really means cancel on the
> QAPI level, so we're free to do that. We just need to keep supporting
> block-job-cancel with the old semantics, so what I have is the easy
> conversion. We can change the internal implementation when we actually
> implement the selection of a completion mode.
> 
> Kevin
> 

We need this before 3.0 though, yeah? unless we make job-cancel
x-job-cancel or some other warning that the way it works might change, yeah?

Or do I misunderstand our leeway to change this at a later point in time?

(can job-cancel apply to block jobs created with the legacy
infrastructure? My reading was "yes.")

--js

Re: [Qemu-devel] [PATCH v2 35/40] job: Add JOB_STATUS_CHANGE QMP event

[John Snow]

On 05/24/2018 04:36 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 02:02 hat John Snow geschrieben:
>>
>>
>> On 05/18/2018 09:21 AM, Kevin Wolf wrote:
>>> This adds a QMP event that is emitted whenever a job transitions from
>>> one status to another.
>>>
>>> Signed-off-by: Kevin Wolf 
>>
>> That's a lot of events, and a lot are redundant to what we already
>> transmitted under block jobs; it also has the effect of making internal
>> state changes explicit behavior that we're responsible for maintaining
>> for external clients.
>>
>> Is that what we want here?
>>
>> (I mean, the answer is probably "Yes" because you're here writing the
>> patch, but I was hoping to find your motivation.)
> 
> The state change is visible in query-jobs anyway, so allowing the client
> to get events rather than polling query-jobs doesn't change the amount
> of information that is exposed.
> 

With differing amounts of guarantees, though; for querying you have to
so happen to catch it and in practice you may never observe many of the
status transitions. It's not something that an external client could
rely on.

With events, we're guaranteeing you will be able to listen to all
possible state transitions. People may begin depending on them in ways
we hadn't anticipated just yet.

...Well, as long as the client is connected to the stream, which we
suggest may not always be possible with the presence of the concluded state.

> You're right that some of the events are redundant with existing block
> job events, but that unavoidable because we can't send BLOCK_JOB_*
> events for non-block jobs. And not sending JOB_* for block jobs would be
> inconsistent.
> 
> The question that I was asking myself was just whether I'd literally
> duplicate the existing events, just with s/BLOCK_JOB_/JOB_/, or whether
> a single event with a status field can do. I think the latter is more
> elegant (also because it can be implemented in a single place), even if
> it is emitted a bit more often than strictly necessary.
> 

Code-wise I agree that this is more elegant; just wondering if the
implications of the additional API guarantees were considered. This
means we have to be a bit more careful about how we restructure the
state machine in the future, I think.

It also makes the "NULL" state visible, which I didn't really intend...
It's probably fine, just a little quirky.

> Kevin
> 

Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks

[Michael S. Tsirkin]

On Thu, May 24, 2018 at 07:26:16PM +0200, Thomas Huth wrote:
> On 24.05.2018 18:11, Michael S. Tsirkin wrote:
> > Add more checks on how did QEMU exit.
> > 
> > Legal ways to exit right now:
> > - exit(0) or return from main
> > - kill(SIGTERM) - sent by testing infrastructure
> > 
> > Anything else is illegal.
> [...]
> > -if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
> > +/* waitpid returns child PID on success */
> > +assert(pid == s->qemu_pid);
> > +
> > +/* If exited on signal - check the reason: core dump is never OK */
> > +if (WIFSIGNALED(wstatus)) {
> >  assert(!WCOREDUMP(wstatus));
> >  }
> > +/* If exited normally - check exit status */
> > +if (WIFEXITED(wstatus)) {
> > +assert(!WEXITSTATUS(wstatus));
> > +}
> > +/* Valid ways to exit: right now only return from main or exit */
> > +assert(WIFEXITED(wstatus));
> >  }
> >  }
> 
> It's strange that you always get WIFEXITED(wstatus) == true here, even
> if QEMU has been terminated by SIGTERM? I assume that's due to the fact
> that QEMU intercepts SIGTERM and terminates via exit() instead?

Right now, yes. This can of course change, so it's not
a good idea hard-coding this assumption to deep
in the code, imho.

> So I
> think you could simply replace the last three asserts with:
> 
>   assert(WIFEXITED(wstatus) && !WEXITSTATUS(wstatus));
> 
>  Thomas

I could but they would be harder to debug.

If I see
"assertion failed: !WCOREDUMP(wstatus)"
then that is very readable.

If I just see
"assertion failed: WIFEXITED(wstatus) && !WEXITSTATUS(wstatus)"
then I just know something went wrong.

-- 
MST

Re: [Qemu-devel] [PULL 0/7] Vga 20180524 patches

[Peter Maydell]

On 24 May 2018 at 16:45, Gerd Hoffmann <kra...@redhat.com> wrote:
> The following changes since commit 4f50c1673a89b07f376ce5c42d22d79a79cd466d:
>
>   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-next-pull-request' 
> into staging (2018-05-22 09:43:58 +0100)
>
> are available in the git repository at:
>
>   git://git.kraxel.org/qemu tags/vga-20180524-pull-request
>
> for you to fetch changes up to dbb2e4726c76dbffb39efe6623bf75d2ec1db8bc:
>
>   MAINTAINERS: add vga entries (2018-05-24 10:42:13 +0200)
>
> 
> vga: catch depth 0
> hw/display: add new bochs-display device
> some cleanups.
>
> 

Applied, thanks.

-- PMM

Re: [Qemu-devel] [qemu PATCH v2] docs/interop: add "firmware.json"

[Laszlo Ersek]

On 05/24/18 18:23, Paolo Bonzini wrote:
> On 24/05/2018 18:21, Laszlo Ersek wrote:
>> On 05/15/18 11:49, Gerd Hoffmann wrote:
>>> On Wed, May 09, 2018 at 05:26:08PM +0200, Laszlo Ersek wrote:
 Add a schema that describes the different uses and properties of virtual
 machine firmware.

 Each firmware executable installed on a host system should come with at
 least one JSON file that conforms to this schema. Each file informs the
 management applications about
 - the firmware's properties and one possible use case / feature set,
 - configuration bits that are required to run the firmware binary.

 In addition, define rules for management apps for picking the highest
 priority firmware JSON file when multiple such files match the search
 criteria.
>>>
>>> Reviewed-by: Gerd Hoffmann 
>>
>> Thanks, Gerd!
>>
>> Does anyone else intend to comment? Or should I ask for someone to queue
>> the patch for a pull request?
> 
> Since no one has done that so far I've queued it, thanks!

Awesome; huge kudos!
Laszlo

Re: [Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks

[Thomas Huth]

On 24.05.2018 18:11, Michael S. Tsirkin wrote:
> Add more checks on how did QEMU exit.
> 
> Legal ways to exit right now:
> - exit(0) or return from main
> - kill(SIGTERM) - sent by testing infrastructure
> 
> Anything else is illegal.
[...]
> -if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
> +/* waitpid returns child PID on success */
> +assert(pid == s->qemu_pid);
> +
> +/* If exited on signal - check the reason: core dump is never OK */
> +if (WIFSIGNALED(wstatus)) {
>  assert(!WCOREDUMP(wstatus));
>  }
> +/* If exited normally - check exit status */
> +if (WIFEXITED(wstatus)) {
> +assert(!WEXITSTATUS(wstatus));
> +}
> +/* Valid ways to exit: right now only return from main or exit */
> +assert(WIFEXITED(wstatus));
>  }
>  }

It's strange that you always get WIFEXITED(wstatus) == true here, even
if QEMU has been terminated by SIGTERM? I assume that's due to the fact
that QEMU intercepts SIGTERM and terminates via exit() instead? So I
think you could simply replace the last three asserts with:

assert(WIFEXITED(wstatus) && !WEXITSTATUS(wstatus));

 Thomas

Re: [Qemu-devel] [PATCH v2 31/40] job: Add job_is_ready()

[John Snow]

On 05/24/2018 04:30 AM, Kevin Wolf wrote:
> Am 24.05.2018 um 01:42 hat John Snow geschrieben:
>> On 05/18/2018 09:21 AM, Kevin Wolf wrote:
>>> Instead of having a 'bool ready' in BlockJob, add a function that
>>> derives its value from the job status.
>>>
>>> At the same time, this fixes the behaviour to match what the QAPI
>>> documentation promises for query-block-job: 'true if the job may be
>>> completed'. When the ready flag was introduced in commit ef6dbf1e46e,
>>> the flag never had to be reset to match the description because after
>>> being ready, the jobs would immediately complete and disappear.
>>>
>>> Job transactions and manual job finalisation were introduced only later.
>>> With these changes, jobs may stay around even after having completed
>>> (and they are not ready to be completed a second time), however their
>>> patches forgot to reset the ready flag.
>>>
>>> Signed-off-by: Kevin Wolf 
>>> Reviewed-by: Max Reitz 
>>> ---
>>>  include/block/blockjob.h |  5 -
>>>  include/qemu/job.h   |  3 +++
>>>  blockjob.c   |  3 +--
>>>  job.c| 22 ++
>>>  qemu-img.c   |  2 +-
>>>  tests/test-blockjob.c|  2 +-
>>>  6 files changed, 28 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
>>> index 5a81afc6c3..8e1e1ee0de 100644
>>> --- a/include/block/blockjob.h
>>> +++ b/include/block/blockjob.h
>>> @@ -49,11 +49,6 @@ typedef struct BlockJob {
>>>  /** The block device on which the job is operating.  */
>>>  BlockBackend *blk;
>>>  
>>> -/**
>>> - * Set to true when the job is ready to be completed.
>>> - */
>>> -bool ready;
>>> -
>>>  /** Status that is published by the query-block-jobs QMP API */
>>>  BlockDeviceIoStatus iostatus;
>>>  
>>> diff --git a/include/qemu/job.h b/include/qemu/job.h
>>> index 1e8050c6fb..487f9d9a32 100644
>>> --- a/include/qemu/job.h
>>> +++ b/include/qemu/job.h
>>> @@ -367,6 +367,9 @@ bool job_is_cancelled(Job *job);
>>>  /** Returns whether the job is in a completed state. */
>>>  bool job_is_completed(Job *job);
>>>  
>>> +/** Returns whether the job is ready to be completed. */
>>> +bool job_is_ready(Job *job);
>>> +
>>>  /**
>>>   * Request @job to pause at the next pause point. Must be paired with
>>>   * job_resume(). If the job is supposed to be resumed by user action, call
>>> diff --git a/blockjob.c b/blockjob.c
>>> index 3ca009bea5..38f18e9ba3 100644
>>> --- a/blockjob.c
>>> +++ b/blockjob.c
>>> @@ -269,7 +269,7 @@ BlockJobInfo *block_job_query(BlockJob *job, Error 
>>> **errp)
>>>  info->offset= job->offset;
>>>  info->speed = job->speed;
>>>  info->io_status = job->iostatus;
>>> -info->ready = job->ready;
>>> +info->ready = job_is_ready(>job),
>>>  info->status= job->job.status;
>>>  info->auto_finalize = job->job.auto_finalize;
>>>  info->auto_dismiss  = job->job.auto_dismiss;
>>> @@ -436,7 +436,6 @@ void block_job_user_resume(Job *job)
>>>  void block_job_event_ready(BlockJob *job)
>>>  {
>>>  job_state_transition(>job, JOB_STATUS_READY);
>>> -job->ready = true;
>>>  
>>>  if (block_job_is_internal(job)) {
>>>  return;
>>> diff --git a/job.c b/job.c
>>> index af31de4669..66ee26f2a0 100644
>>> --- a/job.c
>>> +++ b/job.c
>>> @@ -199,6 +199,28 @@ bool job_is_cancelled(Job *job)
>>>  return job->cancelled;
>>>  }
>>>  
>>> +bool job_is_ready(Job *job)
>>> +{
>>> +switch (job->status) {
>>> +case JOB_STATUS_UNDEFINED:
>>> +case JOB_STATUS_CREATED:
>>> +case JOB_STATUS_RUNNING:
>>> +case JOB_STATUS_PAUSED:
>>> +case JOB_STATUS_WAITING:
>>> +case JOB_STATUS_PENDING:
>>> +case JOB_STATUS_ABORTING:
>>> +case JOB_STATUS_CONCLUDED:
>>> +case JOB_STATUS_NULL:
>>> +return false;
>>> +case JOB_STATUS_READY:
>>> +case JOB_STATUS_STANDBY:
>>> +return true;
>>> +default:
>>> +g_assert_not_reached();
>>> +}
>>> +return false;
>>> +}
>>> +
>>
>> What's the benefit to a switch statement with a default clause here,
>> over the shorter:
>>
>> if (job->status == READY || job->status == STANDBY) {
>>   return true;
>> }
>> return false;
>>
>> (Yes, I realize you already merged this code, but I'm still curious and
>> I need to read the series anyway to see what's changed...)
> 
> That it's easy to copy and paste from job_is_completed()? :-P
> 

Haha! Sure!

> I guess you could argue that the switch ensures that we don't forget to
> explicitly handle every state if we ever add a new one, but the real
> reason is more like, job_is_completed() was already there and I didn't
> see a reason to do something different here.
> 

I think the "default" case removes that benefit somewhat; it's nicer
when the compiler yelps at you for forgetting. The cases that might
cause an assertion could be harder to hit.

No need to change anything 

Re: [Qemu-devel] [RFC 0/2] ARM virt: Support up to 256 PCIe buses

[Laszlo Ersek]

On 05/24/18 16:14, Ard Biesheuvel wrote:
> On 24 May 2018 at 15:59, Laszlo Ersek  wrote:
>> On 05/24/18 15:07, Peter Maydell wrote:
>>> On 24 May 2018 at 13:59, Laszlo Ersek  wrote:
 On 05/24/18 11:11, Peter Maydell wrote:
> Won't it also break a guest which is just Linux loaded not via
> firmware which is an aarch32 kernel without LPAE support?

 Does such a thing exist? (I honestly have no clue.)
>>>
>>> Yes, it does; LPAE isn't a mandatory kernel config option.
>>> This is why we have the machine 'highmem' option, so that
>>> we can run on those kernels by not putting anything above
>>> the 4G boundary. Looking back at the history on that, we
>>> opted at the time for "default to highmem on, and if you're
>>> running an non-lpae kernel you need to turn it off manually".
>>
>> Ah, OK, I didn't know that.
>>
>>> So we can handle those kernels by just not putting ECAM
>>> above 4G if highmem is false.
>>
>> The problem is we can have a combination of 32-bit UEFI firmware (which
>> certainly lacks LPAE) and a 32-bit kernel which supports LPAE.
>> Previously, you wouldn't specify highmem=off, and things would just work
>> -- the firmware would simply ignore the >=4GB MMIO aperture, and use the
>> 32-bit MMIO aperture only (and use the sole 32-bit ECAM). The kernel
>> could then use both low and high MMIO apertures, however (I gather?).
>>
>> The difference with "high ECAM" is that it is *moved* (not *added*), so
>> the 32-bit firmware is left with nothing for config space access. For
>> booting the same combination as above, you are suddenly forced to add
>> highmem=off, just to keep the ECAM low -- and that, while it keeps the
>> firmware happy, prevents the LPAE-capable kernel from using the high
>> MMIO aperture.
>>
>> So I think "highmem_ecam" should be computed like this:
>>
>>   highmem_ecam = highmem_ecam_machtype_default &&
>>  highmem &&
>>  (!firmware_loaded || aarch64);
>>
> 
> Given that the firmware is tightly coupled to the platform, we may
> decide not to care about ECAM for UEFI itself, and invent a secondary
> config space access mechanism that does not consume such a huge amount
> of address space. For instance, legacy PCI uses a pair of I/O ports
> for this, one to set the address and one to perform the actual read or
> write, and we could easily implement something similar (such an
> interface is problematic in SMP context but we don't care about that
> anyway)
> 
> Just a thought - perhaps we don't care enough about 32-bit to go
> through the trouble, but it would be nice if LPAE capable 32-bit
> guests could make use of the expanded PCIe config space as well.

Under the above proposal, they could, they'd just have to be launched
without firmware:

  highmem_ecam_machtype_default = true;
  highmem = true;
  firmware_loaded = false;
  aarch64 = false;

  highmem_ecam = true &&
 true &&
 (!false || false);

I see a return to the 0xCF8/0xCFC pattern regressive; I'd rather
restrict the large/high ECAM feature to 64-bit guests (with or without
firmware), and to 32-bit LPAE kernels that are launched without firmware
(which, I think, has been the case for most of their history).

Personally I don't have a stake in 32-bit ARM, so do take my opinion
with a grain of salt. Wearing my upstream ArmVirtQemu co-maintainer hat,
my sole 32-bit interest is in keeping command lines working, *if* they
once worked. Not extending new QEMU features to 32-bit firmware is fine
with me -- in fact I would value that over seeing more quirky firmware
code just for 32-bit's sake.

Side topic: the last subcondition basically says, "IF we use firmware
THEN the VM had better be 64-bit". This is a "logical implication":
A-->B. The C language doesn't have an "implication operator", so I
rewrote it equivalently with the logical negation and logical OR
operators: A-->B is equivalent to (!A || B). (If A is true, then B must
hold; if A is false, then B doesn't matter.)

Thanks,
Laszlo

Re: [Qemu-devel] [PATCH] nvme: Make nvme_init error handling code more readable

[Paolo Bonzini]

On 21/05/2018 08:35, Fam Zheng wrote:
> Coverity doesn't like the tests under fail label (report CID 1385847).
> Reset the fields so the clean up order is more apparent.
> 
> Signed-off-by: Fam Zheng 
> ---
>  block/nvme.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/block/nvme.c b/block/nvme.c
> index 6f71122bf5..8239b920c8 100644
> --- a/block/nvme.c
> +++ b/block/nvme.c
> @@ -560,6 +560,13 @@ static int nvme_init(BlockDriverState *bs, const char 
> *device, int namespace,
>  qemu_co_queue_init(>dma_flush_queue);
>  s->nsid = namespace;
>  s->aio_context = bdrv_get_aio_context(bs);
> +
> +/* Fields we've not touched should be zero-initialized by block layer
> + * already, but reset them anyway to make the error handling code easier 
> to
> + * reason. */
> +s->regs = NULL;
> +s->vfio = NULL;
> +
>  ret = event_notifier_init(>irq_notifier, 0);
>  if (ret) {
>  error_setg(errp, "Failed to init event notifier");
> 

I think we should just mark it as a false positive or do something like

fail_regs:
qemu_vfio_pci_unmap_bar(s->vfio, 0, (void *)s->regs, 0, NVME_BAR_SIZE);
fail_vfio:
qemu_vfio_close(s->vfio);
fail:
g_free(s->queues);
event_notifier_cleanup(>irq_notifier);
return ret;

even though it's a larger patch.

Paolo

Re: [Qemu-devel] [PATCH v2] block: fix QEMU crash with scsi-hd and drive_del

[Greg Kurz]

On Thu, 24 May 2018 17:52:56 +0100
Stefan Hajnoczi  wrote:

> On Thu, May 24, 2018 at 11:09:47AM +0200, Greg Kurz wrote:
> > diff --git a/block.c b/block.c
> > index 676e57f5623a..fc9379439883 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2127,12 +2127,16 @@ BdrvChild *bdrv_attach_child(BlockDriverState 
> > *parent_bs,
> >  
> >  static void bdrv_detach_child(BdrvChild *child)
> >  {
> > +BlockDriverState *child_bs = child->bs;
> > +
> >  if (child->next.le_prev) {
> >  QLIST_REMOVE(child, next);
> >  child->next.le_prev = NULL;
> >  }  
> 
> This child->next modification makes me nervous.  Please start the
> drained region before modifying the graph.

Ok, I'll send a v3, and Cc qemu-sta...@nongnu.org instead of
sta...@vger.kernel.org this time :P


pgpVFbENaSZ67.pgp
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 15/27] iommu: Add IOMMU index argument to notifier APIs

[Peter Maydell]

On 24 May 2018 at 16:29, Auger Eric  wrote:
> On 05/21/2018 04:03 PM, Peter Maydell wrote:
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index f6226fb263..4e6b125add 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -71,6 +71,7 @@ struct IOMMUTLBEntry {
>>  hwaddr   iova;
>>  hwaddr   translated_addr;
>>  hwaddr   addr_mask;  /* 0xfff = 4k translation */
>> +int  iommu_idx;
> I don't get why ne need iommu_idx field here. On translate the caller
> has it. On notify the notifier has it?

I think this is an accidental leftover from some earlier draft;
nothing in the patchset actually reads or writes this field, and
it should be deleted.

>>  IOMMUAccessFlags perm;
>>  };
>>

>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 8e57265edf..fb396cf00a 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -507,6 +507,7 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>  if (memory_region_is_iommu(section->mr)) {
>>  VFIOGuestIOMMU *giommu;
>>  IOMMUMemoryRegion *iommu_mr = IOMMU_MEMORY_REGION(section->mr);
>> +int iommu_idx;
>>
>>  trace_vfio_listener_region_add_iommu(iova, end);
>>  /*
>> @@ -523,10 +524,13 @@ static void vfio_listener_region_add(MemoryListener 
>> *listener,
>>  llend = int128_add(int128_make64(section->offset_within_region),
>> section->size);
>>  llend = int128_sub(llend, int128_one());
>> +iommu_idx = memory_region_iommu_attrs_to_index(iommu_mr,
>> +   
>> MEMTXATTRS_UNSPECIFIED);
> In that case VFIO ideally wants to be notified for any guest update
> (whatever the page set) to reprogram the physical IOMMU corresponding
> entries and doesn't want to register a notifier per iommu_idx. Also it
> does not know which ones are supported. Is there a corresponding
> iommu_idx value? MEMTXATTRS_ANY?

If VFIO is actually dealing with an IOMMU that needs to handle
multiple possible transactions for different tx attributes, then
it needs to know about all of them, because how it programs
the physical IOMMU needs to be different for "map X to Y for
Secure transactions" versus "map X to Y for NonSecure" (say).
(This would require new kernel APIs, I assume.)

If, as is currently the case, the VFIO infrastructure assumes that
the IOMMU will always translate any transaction from a device
identically regardless of its transaction attributes, then it
should just use MEMTXATTRS_UNSPECIFIED, and trust that the
emulated IOMMU will translate those correctly. (There might be
scope for VFIO checking that the IOMMU really does, eg that
it is only using one iommu index?)

Basically, VFIO is shadowing the behaviour of the emulated
IOMMU to reflect it into the kernel; if the IOMMU it's shadowing
is complicated then VFIO is going to need to be similarly
complicated, and "merge updates for different page tables
down into one" is not going to be the right behaviour.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/2] ARM virt: Support up to 256 PCIe buses

[Laszlo Ersek]

On 05/24/18 16:09, Auger Eric wrote:
> Hi Laszlo,
> 
> On 05/24/2018 03:59 PM, Laszlo Ersek wrote:
>> On 05/24/18 15:07, Peter Maydell wrote:
>>> On 24 May 2018 at 13:59, Laszlo Ersek  wrote:
 On 05/24/18 11:11, Peter Maydell wrote:
> Won't it also break a guest which is just Linux loaded not via
> firmware which is an aarch32 kernel without LPAE support?

 Does such a thing exist? (I honestly have no clue.)
>>>
>>> Yes, it does; LPAE isn't a mandatory kernel config option.
>>> This is why we have the machine 'highmem' option, so that
>>> we can run on those kernels by not putting anything above
>>> the 4G boundary. Looking back at the history on that, we
>>> opted at the time for "default to highmem on, and if you're
>>> running an non-lpae kernel you need to turn it off manually".
>>
>> Ah, OK, I didn't know that.
>>
>>> So we can handle those kernels by just not putting ECAM
>>> above 4G if highmem is false.
>>
>> The problem is we can have a combination of 32-bit UEFI firmware (which
>> certainly lacks LPAE) and a 32-bit kernel which supports LPAE.
> 
> Is it what happens with the FW you provided to me? There is no LPAE in it?

That's the case, to my knowledge.

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v2] block: fix QEMU crash with scsi-hd and drive_del

[Stefan Hajnoczi]

On Thu, May 24, 2018 at 11:09:47AM +0200, Greg Kurz wrote:
> diff --git a/block.c b/block.c
> index 676e57f5623a..fc9379439883 100644
> --- a/block.c
> +++ b/block.c
> @@ -2127,12 +2127,16 @@ BdrvChild *bdrv_attach_child(BlockDriverState 
> *parent_bs,
>  
>  static void bdrv_detach_child(BdrvChild *child)
>  {
> +BlockDriverState *child_bs = child->bs;
> +
>  if (child->next.le_prev) {
>  QLIST_REMOVE(child, next);
>  child->next.le_prev = NULL;
>  }

This child->next modification makes me nervous.  Please start the
drained region before modifying the graph.


signature.asc
Description: PGP signature

[Qemu-devel] [PULL v3 4/4] test: Add test cases that use the external swtpm with CRB interface

[Stefan Berger]

Add a test program for testing the CRB with the external swtpm.

The 1st test case extends a PCR and reads back the value and compares
it against an expected return packet.

The 2nd test case repeats the 1st test case and then migrates the
external swtpm's state along with the VM state to a destination
QEMU and swtpm and checks that the PCR has the expected value now.

The test cases require 'swtpm' to be installed on the system and
in the PATH and 'swtpm' must support the --tpm2 option. If this is
not the case, the test will be skipped.

Signed-off-by: Stefan Berger 
Reviewed-by: Marc-André Lureau 
---
 tests/Makefile.include |   3 +
 tests/tpm-crb-swtpm-test.c | 247 +
 tests/tpm-util.c   | 186 ++
 tests/tpm-util.h   |  36 +++
 4 files changed, 472 insertions(+)
 create mode 100644 tests/tpm-crb-swtpm-test.c
 create mode 100644 tests/tpm-util.c
 create mode 100644 tests/tpm-util.h

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2..b499ba1813 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -297,6 +297,7 @@ check-qtest-i386-$(CONFIG_VHOST_USER_NET_TEST_i386) += 
tests/vhost-user-test$(EX
 ifeq ($(CONFIG_VHOST_USER_NET_TEST_i386),)
 check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) += 
tests/vhost-user-test$(EXESUF)
 endif
+check-qtest-i386-$(CONFIG_TPM) += tests/tpm-crb-swtpm-test$(EXESUF)
 check-qtest-i386-$(CONFIG_TPM) += tests/tpm-crb-test$(EXESUF)
 check-qtest-i386-$(CONFIG_TPM) += tests/tpm-tis-test$(EXESUF)
 check-qtest-i386-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
@@ -721,6 +722,8 @@ tests/test-util-sockets$(EXESUF): tests/test-util-sockets.o 
\
 tests/test-io-task$(EXESUF): tests/test-io-task.o $(test-io-obj-y)
 tests/test-io-channel-socket$(EXESUF): tests/test-io-channel-socket.o \
 tests/io-channel-helpers.o tests/socket-helpers.o $(test-io-obj-y)
+tests/tpm-crb-swtpm-test$(EXESUF): tests/tpm-crb-swtpm-test.o tests/tpm-emu.o \
+   tests/tpm-util.o $(test-io-obj-y)
 tests/tpm-crb-test$(EXESUF): tests/tpm-crb-test.o tests/tpm-emu.o 
$(test-io-obj-y)
 tests/tpm-tis-test$(EXESUF): tests/tpm-tis-test.o tests/tpm-emu.o 
$(test-io-obj-y)
 tests/test-io-channel-file$(EXESUF): tests/test-io-channel-file.o \
diff --git a/tests/tpm-crb-swtpm-test.c b/tests/tpm-crb-swtpm-test.c
new file mode 100644
index 00..c2bde0cbaa
--- /dev/null
+++ b/tests/tpm-crb-swtpm-test.c
@@ -0,0 +1,247 @@
+/*
+ * QTest testcase for TPM CRB talking to external swtpm and swtpm migration
+ *
+ * Copyright (c) 2018 IBM Corporation
+ *  with parts borrowed from migration-test.c that is:
+ * Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
+ *
+ * Authors:
+ *   Stefan Berger 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+
+#include "hw/acpi/tpm.h"
+#include "io/channel-socket.h"
+#include "libqtest.h"
+#include "tpm-util.h"
+#include "sysemu/tpm.h"
+#include "qapi/qmp/qdict.h"
+
+typedef struct TestState {
+char *src_tpm_path;
+char *dst_tpm_path;
+char *uri;
+} TestState;
+
+bool got_stop;
+
+static void migrate(QTestState *who, const char *uri)
+{
+QDict *rsp;
+gchar *cmd;
+
+cmd = g_strdup_printf("{ 'execute': 'migrate',"
+  "'arguments': { 'uri': '%s' } }",
+  uri);
+rsp = qtest_qmp(who, cmd);
+g_free(cmd);
+g_assert(qdict_haskey(rsp, "return"));
+qobject_unref(rsp);
+}
+
+/*
+ * Events can get in the way of responses we are actually waiting for.
+ */
+static QDict *wait_command(QTestState *who, const char *command)
+{
+const char *event_string;
+QDict *response;
+
+response = qtest_qmp(who, command);
+
+while (qdict_haskey(response, "event")) {
+/* OK, it was an event */
+event_string = qdict_get_str(response, "event");
+if (!strcmp(event_string, "STOP")) {
+got_stop = true;
+}
+qobject_unref(response);
+response = qtest_qmp_receive(who);
+}
+return response;
+}
+
+static void wait_for_migration_complete(QTestState *who)
+{
+while (true) {
+QDict *rsp, *rsp_return;
+bool completed;
+const char *status;
+
+rsp = wait_command(who, "{ 'execute': 'query-migrate' }");
+rsp_return = qdict_get_qdict(rsp, "return");
+status = qdict_get_str(rsp_return, "status");
+completed = strcmp(status, "completed") == 0;
+g_assert_cmpstr(status, !=,  "failed");
+qobject_unref(rsp);
+if (completed) {
+return;
+}
+usleep(1000);
+}
+}
+
+static void migration_start_qemu(QTestState **src_qemu, QTestState **dst_qemu,
+ SocketAddress 

[Qemu-devel] [PULL v3 3/4] docs: tpm: add VM save/restore example and troubleshooting guide

[Stefan Berger]

Extend the docs related to TPM with specs related to VM save and
restore and a troubleshooting guide for TPM migration.

Signed-off-by: Stefan Berger 
Reviewed-by: Dr. David Alan Gilbert 
---
 docs/specs/tpm.txt | 106 +
 1 file changed, 106 insertions(+)

diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index d1d71571e9..c230c4c93e 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -200,3 +200,109 @@ crw---. 1 root root 10, 224 Jul 11 10:11 /dev/tpm0
 PCR-00: 35 4E 3B CE 23 9F 38 59 ...
 ...
 PCR-23: 00 00 00 00 00 00 00 00 ...
+
+
+=== Migration with the TPM emulator ===
+
+The TPM emulator supports the following types of virtual machine migration:
+
+- VM save / restore (migration into a file)
+- Network migration
+- Snapshotting (migration into storage like QoW2 or QED)
+
+The following command sequences can be used to test VM save / restore.
+
+
+In a 1st terminal start an instance of a swtpm using the following command:
+
+mkdir /tmp/mytpm1
+swtpm socket --tpmstate dir=/tmp/mytpm1 \
+  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock \
+  --log level=20 --tpm2
+
+In a 2nd terminal start the VM:
+
+qemu-system-x86_64 -display sdl -enable-kvm \
+  -m 1024 -boot d -bios bios-256k.bin -boot menu=on \
+  -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+  -tpmdev emulator,id=tpm0,chardev=chrtpm \
+  -device tpm-tis,tpmdev=tpm0 \
+  -monitor stdio \
+  test.img
+
+Verify that the attached TPM is working as expected using applications inside
+the VM.
+
+To store the state of the VM use the following command in the QEMU monitor in
+the 2nd terminal:
+
+(qemu) migrate "exec:cat > testvm.bin"
+(qemu) quit
+
+At this point a file called 'testvm.bin' should exists and the swtpm and QEMU
+processes should have ended.
+
+To test 'VM restore' you have to start the swtpm with the same parameters
+as before. If previously a TPM 2 [--tpm2] was saved, --tpm2 must now be
+passed again on the command line.
+
+In the 1st terminal restart the swtpm with the same command line as before:
+
+swtpm socket --tpmstate dir=/tmp/mytpm1 \
+  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock \
+  --log level=20 --tpm2
+
+In the 2nd terminal restore the state of the VM using the additonal
+'-incoming' option.
+
+qemu-system-x86_64 -display sdl -enable-kvm \
+  -m 1024 -boot d -bios bios-256k.bin -boot menu=on \
+  -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+  -tpmdev emulator,id=tpm0,chardev=chrtpm \
+  -device tpm-tis,tpmdev=tpm0 \
+  -incoming "exec:cat < testvm.bin" \
+  test.img
+
+
+Troubleshooting migration:
+
+There are several reasons why migration may fail. In case of problems,
+please ensure that the command lines adhere to the following rules and,
+if possible, that identical versions of QEMU and swtpm are used at all
+times.
+
+VM save and restore:
+ - QEMU command line parameters should be identical apart from the
+   '-incoming' option on VM restore
+ - swtpm command line parameters should be identical
+
+VM migration to 'localhost':
+ - QEMU command line parameters should be identical apart from the
+   '-incoming' option on the destination side
+ - swtpm command line parameters should point to two different
+   directories on the source and destination swtpm (--tpmstate dir=...)
+   (especially if different versions of libtpms were to be used on the
+   same machine).
+
+VM migration across the network:
+ - QEMU command line parameters should be identical apart from the
+   '-incoming' option on the destination side
+ - swtpm command line parameters should be identical
+
+VM Snapshotting:
+ - QEMU command line parameters should be identical
+ - swtpm command line parameters should be identical
+
+
+Besides that, migration failure reasons on the swtpm level may include
+the following:
+
+ - the versions of the swtpm on the source and destination sides are
+   incompatible
+   - downgrading of TPM state may not be supported
+   - the source and destination libtpms were compiled with different
+ compile-time options and the destination side refuses to accept the
+ state
+ - different migration keys are used on the source and destination side
+   and the destination side cannot decrypt the migrated state
+   (swtpm ... --migration-key ... )
-- 
2.14.3

[Qemu-devel] [PULL v3 2/4] tpm: extend TPM TIS with state migration support

[Stefan Berger]

Extend the TPM TIS interface with state migration support.

We need to synchronize with the backend thread to make sure that a command
being processed by the external TPM emulator has completed and its
response been received.

Signed-off-by: Stefan Berger 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Marc-André Lureau 
---
 hw/tpm/tpm_tis.c| 52 ++--
 hw/tpm/trace-events |  1 +
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 2ac7e74307..12f5c9a759 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -894,9 +894,57 @@ static void tpm_tis_reset(DeviceState *dev)
 tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size);
 }
 
+/* persistent state handling */
+
+static int tpm_tis_pre_save(void *opaque)
+{
+TPMState *s = opaque;
+uint8_t locty = s->active_locty;
+
+trace_tpm_tis_pre_save(locty, s->rw_offset);
+
+if (DEBUG_TIS) {
+tpm_tis_dump_state(opaque, 0);
+}
+
+/*
+ * Synchronize with backend completion.
+ */
+tpm_backend_finish_sync(s->be_driver);
+
+return 0;
+}
+
+static const VMStateDescription vmstate_locty = {
+.name = "tpm-tis/locty",
+.version_id = 0,
+.fields  = (VMStateField[]) {
+VMSTATE_UINT32(state, TPMLocality),
+VMSTATE_UINT32(inte, TPMLocality),
+VMSTATE_UINT32(ints, TPMLocality),
+VMSTATE_UINT8(access, TPMLocality),
+VMSTATE_UINT32(sts, TPMLocality),
+VMSTATE_UINT32(iface_id, TPMLocality),
+VMSTATE_END_OF_LIST(),
+}
+};
+
 static const VMStateDescription vmstate_tpm_tis = {
-.name = "tpm",
-.unmigratable = 1,
+.name = "tpm-tis",
+.version_id = 0,
+.pre_save  = tpm_tis_pre_save,
+.fields = (VMStateField[]) {
+VMSTATE_BUFFER(buffer, TPMState),
+VMSTATE_UINT16(rw_offset, TPMState),
+VMSTATE_UINT8(active_locty, TPMState),
+VMSTATE_UINT8(aborting_locty, TPMState),
+VMSTATE_UINT8(next_locty, TPMState),
+
+VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 0,
+ vmstate_locty, TPMLocality),
+
+VMSTATE_END_OF_LIST()
+}
 };
 
 static Property tpm_tis_properties[] = {
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index c5bfbf1d4b..25bee0cecf 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -50,3 +50,4 @@ tpm_tis_mmio_write_locty_seized(uint8_t locty, uint8_t 
active) "Locality %d seiz
 tpm_tis_mmio_write_init_abort(void) "Initiating abort"
 tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
 tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to 
TPM: 0x%08x (size=%d)"
+tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
-- 
2.14.3

[Qemu-devel] [PULL v3 1/4] tpm: extend TPM emulator with state migration support

[Stefan Berger]

Extend the TPM emulator backend device with state migration support.

The external TPM emulator 'swtpm' provides a protocol over
its control channel to retrieve its state blobs. We implement
functions for getting and setting the different state blobs.
In case the setting of the state blobs fails, we return a
negative errno code to fail the start of the VM.

Since we have an external TPM emulator, we need to make sure
that we do not migrate the state for as long as it is busy
processing a request. We need to wait for notification that
the request has completed processing.

Signed-off-by: Stefan Berger 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Marc-André Lureau 
---
 hw/tpm/tpm_emulator.c | 323 --
 hw/tpm/trace-events   |   8 +-
 2 files changed, 319 insertions(+), 12 deletions(-)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 6418ef0831..10bc20dbec 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -4,7 +4,7 @@
  *  Copyright (c) 2017 Intel Corporation
  *  Author: Amarnath Valluri 
  *
- *  Copyright (c) 2010 - 2013 IBM Corporation
+ *  Copyright (c) 2010 - 2013, 2018 IBM Corporation
  *  Authors:
  *Stefan Berger 
  *
@@ -49,6 +49,19 @@
 #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
 
 /* data structures */
+
+/* blobs from the TPM; part of VM state when migrating */
+typedef struct TPMBlobBuffers {
+uint32_t permanent_flags;
+TPMSizedBuffer permanent;
+
+uint32_t volatil_flags;
+TPMSizedBuffer volatil;
+
+uint32_t savestate_flags;
+TPMSizedBuffer savestate;
+} TPMBlobBuffers;
+
 typedef struct TPMEmulator {
 TPMBackend parent;
 
@@ -64,6 +77,8 @@ typedef struct TPMEmulator {
 
 unsigned int established_flag:1;
 unsigned int established_flag_cached:1;
+
+TPMBlobBuffers state_blobs;
 } TPMEmulator;
 
 
@@ -293,7 +308,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
 return 0;
 }
 
-static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
+ bool is_resume)
 {
 TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
 ptm_init init = {
@@ -301,12 +317,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, 
size_t buffersize)
 };
 ptm_res res;
 
+trace_tpm_emulator_startup_tpm_resume(is_resume, buffersize);
+
 if (buffersize != 0 &&
 tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
 goto err_exit;
 }
 
-trace_tpm_emulator_startup_tpm();
+if (is_resume) {
+init.u.req.init_flags |= cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
+}
+
 if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, , sizeof(init),
  sizeof(init)) < 0) {
 error_report("tpm-emulator: could not send INIT: %s",
@@ -325,6 +346,11 @@ err_exit:
 return -1;
 }
 
+static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+{
+return tpm_emulator_startup_tpm_resume(tb, buffersize, false);
+}
+
 static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
 {
 TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
@@ -423,16 +449,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
 static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
 {
 Error *err = NULL;
+ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
+   PTM_CAP_STOP;
 
-error_setg(_emu->migration_blocker,
-   "Migration disabled: TPM emulator not yet migratable");
-migrate_add_blocker(tpm_emu->migration_blocker, );
-if (err) {
-error_report_err(err);
-error_free(tpm_emu->migration_blocker);
-tpm_emu->migration_blocker = NULL;
+if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
+error_setg(_emu->migration_blocker,
+   "Migration disabled: TPM emulator does not support "
+   "migration");
+migrate_add_blocker(tpm_emu->migration_blocker, );
+if (err) {
+error_report_err(err);
+error_free(tpm_emu->migration_blocker);
+tpm_emu->migration_blocker = NULL;
 
-return -1;
+return -1;
+}
 }
 
 return 0;
@@ -570,6 +601,267 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
 { /* end of list */ },
 };
 
+/*
+ * Transfer a TPM state blob from the TPM into a provided buffer.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of blob to transfer
+ * @tsb: the TPMSizeBuffer to fill with the blob
+ * @flags: the flags to return to the caller
+ */
+static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
+   uint8_t type,
+   TPMSizedBuffer *tsb,
+

[Qemu-devel] [PULL v3 0/4] Merge tpm 2018/05/23

[Stefan Berger]

This series of patches adds TPM emulator state migration support and a
test case for testing (local) migration.

   Stefan


The following changes since commit 4f50c1673a89b07f376ce5c42d22d79a79cd466d:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-next-pull-request' 
into staging (2018-05-22 09:43:58 +0100)

are available in the Git repository at:

  git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-05-23-3

for you to fetch changes up to 37fa382f327405b6516e9983c1aa1ca32c726892:

  test: Add test cases that use the external swtpm with CRB interface 
(2018-05-24 12:07:04 -0400)


Merge tpm 2018/05/23 v3


Stefan Berger (4):
  tpm: extend TPM emulator with state migration support
  tpm: extend TPM TIS with state migration support
  docs: tpm: add VM save/restore example and troubleshooting guide
  test: Add test cases that use the external swtpm with CRB interface

 docs/specs/tpm.txt | 106 +
 hw/tpm/tpm_emulator.c  | 323 
+++---
 hw/tpm/tpm_tis.c   |  52 +-
 hw/tpm/trace-events|   9 +-
 tests/Makefile.include |   3 +
 tests/tpm-crb-swtpm-test.c | 247 
+++
 tests/tpm-util.c   | 186 
 tests/tpm-util.h   |  36 +++
 8 files changed, 948 insertions(+), 14 deletions(-)
 create mode 100644 tests/tpm-crb-swtpm-test.c
 create mode 100644 tests/tpm-util.c
 create mode 100644 tests/tpm-util.h


-- 
2.14.3

Re: [Qemu-devel] [PATCH v3 6/8] linux-user: update ARCH_HAS_SOCKET_TYPES use

[Peter Maydell]

On 24 May 2018 at 16:50, Laurent Vivier  wrote:
> Le 24/05/2018 à 17:29, Laurent Vivier a écrit :
>> Le 21/05/2018 à 11:19, Peter Maydell a écrit :
>>> On 19 May 2018 at 10:29, Laurent Vivier  wrote:
 to be like in the kernel and rename it TARGET_ARCH_HAS_SOCKET_TYPES
>>>
>>> You could note in the commit message that this fixes our
>>> incorrect definition of TARGET_SOCK_CLOEXEC for SPARC.
>>
>> I agree
>
> In fact it doesn't change the value of TARGET_SOCK_CLOEXEC for SPARC,
> because original value is 02000 (0x40) and the new value from
> linux-user/socket.h is TARGET_O_CLOEXEC which is also 0x40 for SPARC.

Argh, octal vs hex. Thanks for checking that.

-- PMM

Re: [Qemu-devel] [PULL v2 0/4] Merge tpm 2018/05/23

[Stefan Berger]

On 05/24/2018 12:14 PM, Stefan Berger wrote:

This series of patches adds TPM emulator state migration support and a
test case for testing (local) migration.

Stefan

The following changes since commit 4f50c1673a89b07f376ce5c42d22d79a79cd466d:

   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-next-pull-request' 
into staging (2018-05-22 09:43:58 +0100)

are available in the Git repository at:

   git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-05-23-1


EBADTAG: That should be a '-2'. I guess I will have to fix this in a v3.

   Stefan

Re: [Qemu-devel] [PATCH 4/6] vhost-user: support registering external host notifiers

[Tiwei Bie]

On Thu, May 24, 2018 at 07:15:24PM +0300, Michael S. Tsirkin wrote:
> On Fri, May 25, 2018 at 12:05:50AM +0800, Tiwei Bie wrote:
> > On Thu, May 24, 2018 at 06:49:47PM +0300, Michael S. Tsirkin wrote:
> > > On Thu, May 24, 2018 at 06:33:34PM +0800, Tiwei Bie wrote:
> > > > This patch introduces VHOST_USER_PROTOCOL_F_HOST_NOTIFIER.
> > > > With this feature negotiated, vhost-user backend can register
> > > > memory region based host notifiers. And it will allow the guest
> > > > driver in the VM to notify the hardware accelerator at the
> > > > vhost-user backend directly.
> > > > 
> > > > Signed-off-by: Tiwei Bie 
> > > 
> > > So maybe we don't need a new VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD?
> > > Let's check VHOST_USER_PROTOCOL_F_HOST_NOTIFIER instead?
> > 
> > Yeah. I think it would be the best choice!
> > 
> > If this feature (HOST NOTIFIER) is negotiated, it means
> > the QEMU is able to safely receive (the expected number
> > of) file descriptors.
> > 
> > > 
> > > > ---
> > [...]
> > > > +typedef struct VhostUserHostNotifier {
> > > > +MemoryRegion mr;
> > > > +void *addr;
> > > > +bool set;
> > > > +} VhostUserHostNotifier;
> > > >  
> > > >  typedef struct VhostUserState {
> > > >  CharBackend *chr;
> > > > +VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX];
> > > >  } VhostUserState;
> > > 
> > > So this notifier is per-queue. Can't we maintain it in
> > > NetVhostUserState then?
> > 
> > This notifier is per virtio-queue. But NetVhostUserState
> > is per net-queue-pair.
> > 
> > And ideally, I think this structure shouldn't be visible
> > to net/vhost-user, vhost-user-scsi, vhost-user-crypto, etc.
> > 
> > Best regards,
> > Tiwei Bie
> 
> I wonder whether we can create a new per vq structure.

Maybe something like:

typedef struct VhostUserStatePerVQ {
VhostUserHostNotifier notifier;
} VhostUserStatePerVQ;

typedef struct VhostUserState {
CharBackend *chr;
VhostUserStatePerVQ queue[VIRTIO_QUEUE_MAX];
} VhostUserState;

Best regards,
Tiwei Bie

Re: [Qemu-devel] [qemu PATCH v2] docs/interop: add "firmware.json"

[Laszlo Ersek]

On 05/15/18 11:49, Gerd Hoffmann wrote:
> On Wed, May 09, 2018 at 05:26:08PM +0200, Laszlo Ersek wrote:
>> Add a schema that describes the different uses and properties of virtual
>> machine firmware.
>>
>> Each firmware executable installed on a host system should come with at
>> least one JSON file that conforms to this schema. Each file informs the
>> management applications about
>> - the firmware's properties and one possible use case / feature set,
>> - configuration bits that are required to run the firmware binary.
>>
>> In addition, define rules for management apps for picking the highest
>> priority firmware JSON file when multiple such files match the search
>> criteria.
> 
> Reviewed-by: Gerd Hoffmann 

Thanks, Gerd!

Does anyone else intend to comment? Or should I ask for someone to queue
the patch for a pull request?

Thanks!
Laszlo

Re: [Qemu-devel] [PATCH] prep: fix keyboard for the 40p machine

[Philippe Mathieu-Daudé]

On 05/24/2018 02:39 AM, Mark Cave-Ayland wrote:
> Commit 72d3d8f052 "hw/isa/superio: Add a keyboard/mouse controller (8042)"
> added an 8042 keyboard device to the PC87312 superio device to replace that
> being used by the prep machine.
> 
> Unfortunately this commit didn't do the same for the 40p machine which broke
> the keyboard by registering two 8042 keyboard devices at the same address.

Oops sorry... I have this fixed in the following up series after SuperIO
cleanup, which is SouthBridge cleanup, involving a good rework of the
PIIX and I82378 chipsets, using Hervé Poussineau patches.
But I have 2 more prioritary series to finish before returning to this
one :/

> Resolve this by similarly removing the 8042 keyboard from the 40p machine as
> done for the prep machine in commit 72d3d8f052.
> 
> Signed-off-by: Mark Cave-Ayland 

Reviewed-by: Philippe Mathieu-Daudé 

Thanks for fixing this (too bad there are no keyboard qtests and this
got unnoticed).

> ---
>  hw/ppc/prep.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index a1e7219db6..be4db6a687 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -770,7 +770,6 @@ static void ibm_40p_init(MachineState *machine)
>  
>  /* add some more devices */
>  if (defaults_enabled()) {
> -isa_create_simple(isa_bus, TYPE_I8042);
>  m48t59 = NVRAM(isa_create_simple(isa_bus, "isa-m48t59"));
>  
>  dev = DEVICE(isa_create(isa_bus, "cs4231a"));
> 



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] storing machine data in qcow images?

[Markus Armbruster]

"Richard W.M. Jones"  writes:

> On Thu, May 24, 2018 at 05:08:17PM +0200, Kevin Wolf wrote:
>> Am 24.05.2018 um 16:56 hat Michael S. Tsirkin geschrieben:
>> > On Thu, May 24, 2018 at 12:32:51PM +0100, Richard W.M. Jones wrote:
>> > > There is however a seed of a good idea in the thread:
>> > > 
>> > > > I don't think QEMU needs to use this information automatically,
>> > > > necessarily.  I think the first step is to simply make QEMU save
>> > > > this information in the disk image, and making qemu-img able to
>> > > > read and write this information.
>> > > 
>> > > It would be nice if qcow2 added arbitrary data sections (which would
>> > > always be ignored by qemu) for storing additional data.  This could be
>> > > used to create a compact qcow2 + metadata format to rival OVA for
>> > > management layers to use, and there are various other uses too.
>> > > 
>> > > Rich.
>> > 
>> > I think this part is pretty uncontroversial.
>> > 
>> > But can we add data without changing the verion?
>> 
>> Yes. Don't worry about where to store it, we'll solve this. Do worry
>> about what to store.
>
> Ideally from my point of view: named blobs.  More formally, any number
> of (key, value) pairs where the key is a simple string, and the value
> is a binary blob.  The binary blobs might really be XML or YAML or an
> icon or whatever, but qemu would not need to look inside them.
>
> We could then attach metadata (in some to-be-decided format) to qcow2
> files and create a compact rival to OVA without needing to encode any
> knowledge of the metadata into qemu at all.
>
> Another use for this is allowing qcow2 files to contain names, titles,
> descriptions, creation date, OS icons, etc. which could be displayed
> in file managers.

Have we just reinvented resource forks?

SCNR ;)

Re: [Qemu-devel] [PATCH] pnv: add a physical mapping array describing MMIO ranges in each chip

[Cédric Le Goater]

On 05/23/2018 04:04 PM, Greg Kurz wrote:
> On Wed, 23 May 2018 14:37:30 +0200
> Cédric Le Goater  wrote:
> 
>> On 05/18/2018 11:00 AM, Greg Kurz wrote:
>>> On Thu, 17 May 2018 15:18:14 +0200
>>> Cédric Le Goater  wrote:
>>>   
 Based on previous work done in skiboot, the physical mapping array
 helps in calculating the MMIO ranges of each controller depending on
 the chip id and the chip type. This is will be particularly useful for
 the P9 models which use less the XSCOM bus and rely more on MMIOs.

 A link on the chip is now necessary to calculate MMIO BARs and
 sizes. This is why such a link is introduced in the PSIHB model.

 Signed-off-by: Cédric Le Goater 
 ---
  hw/ppc/pnv.c | 32 +
  hw/ppc/pnv_psi.c | 11 +-
  hw/ppc/pnv_xscom.c   |  8 
  include/hw/ppc/pnv.h | 58 
 +---
  4 files changed, 80 insertions(+), 29 deletions(-)

 diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c
 index 031488131629..330bf6f74810 100644
 --- a/hw/ppc/pnv.c
 +++ b/hw/ppc/pnv.c
 @@ -712,6 +712,16 @@ static uint32_t pnv_chip_core_pir_p9(PnvChip *chip, 
 uint32_t core_id)
   */
  #define POWER9_CORE_MASK   (0xffull)
  
 +/*
 + * POWER8 MMIOs
 + */
 +static const PnvPhysMapEntry pnv_chip_power8_phys_map[] = {
 +[PNV_MAP_XSCOM] = { 0x0003fc00ull, 0x0008ull 
 },
 +[PNV_MAP_ICP]   = { 0x00038000ull, 0x0010ull 
 },
 +[PNV_MAP_PSIHB] = { 0x0003fffe8000ull, 0x0010ull 
 },
 +[PNV_MAP_PSIHB_FSP] = { 0x0003ffe0ull, 0x0001ull 
 },
 +};
 +
  static void pnv_chip_power8e_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc = DEVICE_CLASS(klass);
 @@ -721,7 +731,7 @@ static void pnv_chip_power8e_class_init(ObjectClass 
 *klass, void *data)
  k->chip_cfam_id = 0x221ef0498000ull;  /* P8 Murano DD2.1 */
  k->cores_mask = POWER8E_CORE_MASK;
  k->core_pir = pnv_chip_core_pir_p8;
 -k->xscom_base = 0x003fc00ull;
 +k->phys_map = pnv_chip_power8_phys_map;
  dc->desc = "PowerNV Chip POWER8E";
  }
  
 @@ -734,7 +744,7 @@ static void pnv_chip_power8_class_init(ObjectClass 
 *klass, void *data)
  k->chip_cfam_id = 0x220ea0498000ull; /* P8 Venice DD2.0 */
  k->cores_mask = POWER8_CORE_MASK;
  k->core_pir = pnv_chip_core_pir_p8;
 -k->xscom_base = 0x003fc00ull;
 +k->phys_map = pnv_chip_power8_phys_map;
  dc->desc = "PowerNV Chip POWER8";
  }
  
 @@ -747,10 +757,17 @@ static void 
 pnv_chip_power8nvl_class_init(ObjectClass *klass, void *data)
  k->chip_cfam_id = 0x120d30498000ull;  /* P8 Naples DD1.0 */
  k->cores_mask = POWER8_CORE_MASK;
  k->core_pir = pnv_chip_core_pir_p8;
 -k->xscom_base = 0x003fc00ull;
 +k->phys_map = pnv_chip_power8_phys_map;
  dc->desc = "PowerNV Chip POWER8NVL";
  }
  
 +/*
 + * POWER9 MMIOs
 + */
 +static const PnvPhysMapEntry pnv_chip_power9_phys_map[] = {
 +[PNV_MAP_XSCOM] = { 0x000603fcull, 0x0400ull 
 },
 +};
 +
  static void pnv_chip_power9_class_init(ObjectClass *klass, void *data)
  {
  DeviceClass *dc = DEVICE_CLASS(klass);
 @@ -760,7 +777,7 @@ static void pnv_chip_power9_class_init(ObjectClass 
 *klass, void *data)
  k->chip_cfam_id = 0x220d10498000ull; /* P9 Nimbus DD2.0 */
  k->cores_mask = POWER9_CORE_MASK;
  k->core_pir = pnv_chip_core_pir_p9;
 -k->xscom_base = 0x00603fcull;
 +k->phys_map = pnv_chip_power9_phys_map;
  dc->desc = "PowerNV Chip POWER9";
  }
  
 @@ -797,15 +814,14 @@ static void pnv_chip_core_sanitize(PnvChip *chip, 
 Error **errp)
  static void pnv_chip_init(Object *obj)
  {
  PnvChip *chip = PNV_CHIP(obj);
 -PnvChipClass *pcc = PNV_CHIP_GET_CLASS(chip);
 -
 -chip->xscom_base = pcc->xscom_base;
  
  object_initialize(>lpc, sizeof(chip->lpc), TYPE_PNV_LPC);
  object_property_add_child(obj, "lpc", OBJECT(>lpc), NULL);
  
  object_initialize(>psi, sizeof(chip->psi), TYPE_PNV_PSI);
  object_property_add_child(obj, "psi", OBJECT(>psi), NULL);
 +object_property_add_const_link(OBJECT(>psi), "chip", obj,
 +   _abort);
  object_property_add_const_link(OBJECT(>psi), "xics",
 OBJECT(qdev_get_machine()), 
 _abort);
  
 @@ -829,7 +845,7 @@ static void pnv_chip_icp_realize(PnvChip *chip, Error 
 **errp)
  

Re: [Qemu-devel] [qemu PATCH v2] docs/interop: add "firmware.json"

[Paolo Bonzini]

On 24/05/2018 18:21, Laszlo Ersek wrote:
> On 05/15/18 11:49, Gerd Hoffmann wrote:
>> On Wed, May 09, 2018 at 05:26:08PM +0200, Laszlo Ersek wrote:
>>> Add a schema that describes the different uses and properties of virtual
>>> machine firmware.
>>>
>>> Each firmware executable installed on a host system should come with at
>>> least one JSON file that conforms to this schema. Each file informs the
>>> management applications about
>>> - the firmware's properties and one possible use case / feature set,
>>> - configuration bits that are required to run the firmware binary.
>>>
>>> In addition, define rules for management apps for picking the highest
>>> priority firmware JSON file when multiple such files match the search
>>> criteria.
>>
>> Reviewed-by: Gerd Hoffmann 
> 
> Thanks, Gerd!
> 
> Does anyone else intend to comment? Or should I ask for someone to queue
> the patch for a pull request?

Since no one has done that so far I've queued it, thanks!

Paolo

[Qemu-devel] [PULL v2 2/4] tpm: extend TPM TIS with state migration support

[Stefan Berger]

Extend the TPM TIS interface with state migration support.

We need to synchronize with the backend thread to make sure that a command
being processed by the external TPM emulator has completed and its
response been received.

Signed-off-by: Stefan Berger 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Marc-André Lureau 
---
 hw/tpm/tpm_tis.c| 52 ++--
 hw/tpm/trace-events |  1 +
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/hw/tpm/tpm_tis.c b/hw/tpm/tpm_tis.c
index 2ac7e74307..12f5c9a759 100644
--- a/hw/tpm/tpm_tis.c
+++ b/hw/tpm/tpm_tis.c
@@ -894,9 +894,57 @@ static void tpm_tis_reset(DeviceState *dev)
 tpm_backend_startup_tpm(s->be_driver, s->be_buffer_size);
 }
 
+/* persistent state handling */
+
+static int tpm_tis_pre_save(void *opaque)
+{
+TPMState *s = opaque;
+uint8_t locty = s->active_locty;
+
+trace_tpm_tis_pre_save(locty, s->rw_offset);
+
+if (DEBUG_TIS) {
+tpm_tis_dump_state(opaque, 0);
+}
+
+/*
+ * Synchronize with backend completion.
+ */
+tpm_backend_finish_sync(s->be_driver);
+
+return 0;
+}
+
+static const VMStateDescription vmstate_locty = {
+.name = "tpm-tis/locty",
+.version_id = 0,
+.fields  = (VMStateField[]) {
+VMSTATE_UINT32(state, TPMLocality),
+VMSTATE_UINT32(inte, TPMLocality),
+VMSTATE_UINT32(ints, TPMLocality),
+VMSTATE_UINT8(access, TPMLocality),
+VMSTATE_UINT32(sts, TPMLocality),
+VMSTATE_UINT32(iface_id, TPMLocality),
+VMSTATE_END_OF_LIST(),
+}
+};
+
 static const VMStateDescription vmstate_tpm_tis = {
-.name = "tpm",
-.unmigratable = 1,
+.name = "tpm-tis",
+.version_id = 0,
+.pre_save  = tpm_tis_pre_save,
+.fields = (VMStateField[]) {
+VMSTATE_BUFFER(buffer, TPMState),
+VMSTATE_UINT16(rw_offset, TPMState),
+VMSTATE_UINT8(active_locty, TPMState),
+VMSTATE_UINT8(aborting_locty, TPMState),
+VMSTATE_UINT8(next_locty, TPMState),
+
+VMSTATE_STRUCT_ARRAY(loc, TPMState, TPM_TIS_NUM_LOCALITIES, 0,
+ vmstate_locty, TPMLocality),
+
+VMSTATE_END_OF_LIST()
+}
 };
 
 static Property tpm_tis_properties[] = {
diff --git a/hw/tpm/trace-events b/hw/tpm/trace-events
index c5bfbf1d4b..25bee0cecf 100644
--- a/hw/tpm/trace-events
+++ b/hw/tpm/trace-events
@@ -50,3 +50,4 @@ tpm_tis_mmio_write_locty_seized(uint8_t locty, uint8_t 
active) "Locality %d seiz
 tpm_tis_mmio_write_init_abort(void) "Initiating abort"
 tpm_tis_mmio_write_lowering_irq(void) "Lowering IRQ"
 tpm_tis_mmio_write_data2send(uint32_t value, unsigned size) "Data to send to 
TPM: 0x%08x (size=%d)"
+tpm_tis_pre_save(uint8_t locty, uint32_t rw_offset) "locty: %d, rw_offset = %u"
-- 
2.14.3

[Qemu-devel] [PULL v2 1/4] tpm: extend TPM emulator with state migration support

[Stefan Berger]

Extend the TPM emulator backend device with state migration support.

The external TPM emulator 'swtpm' provides a protocol over
its control channel to retrieve its state blobs. We implement
functions for getting and setting the different state blobs.
In case the setting of the state blobs fails, we return a
negative errno code to fail the start of the VM.

Since we have an external TPM emulator, we need to make sure
that we do not migrate the state for as long as it is busy
processing a request. We need to wait for notification that
the request has completed processing.

Signed-off-by: Stefan Berger 
Reviewed-by: Dr. David Alan Gilbert 
Reviewed-by: Marc-André Lureau 
---
 hw/tpm/tpm_emulator.c | 323 --
 hw/tpm/trace-events   |   8 +-
 2 files changed, 319 insertions(+), 12 deletions(-)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 6418ef0831..10bc20dbec 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -4,7 +4,7 @@
  *  Copyright (c) 2017 Intel Corporation
  *  Author: Amarnath Valluri 
  *
- *  Copyright (c) 2010 - 2013 IBM Corporation
+ *  Copyright (c) 2010 - 2013, 2018 IBM Corporation
  *  Authors:
  *Stefan Berger 
  *
@@ -49,6 +49,19 @@
 #define TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(S, cap) (((S)->caps & (cap)) == (cap))
 
 /* data structures */
+
+/* blobs from the TPM; part of VM state when migrating */
+typedef struct TPMBlobBuffers {
+uint32_t permanent_flags;
+TPMSizedBuffer permanent;
+
+uint32_t volatil_flags;
+TPMSizedBuffer volatil;
+
+uint32_t savestate_flags;
+TPMSizedBuffer savestate;
+} TPMBlobBuffers;
+
 typedef struct TPMEmulator {
 TPMBackend parent;
 
@@ -64,6 +77,8 @@ typedef struct TPMEmulator {
 
 unsigned int established_flag:1;
 unsigned int established_flag_cached:1;
+
+TPMBlobBuffers state_blobs;
 } TPMEmulator;
 
 
@@ -293,7 +308,8 @@ static int tpm_emulator_set_buffer_size(TPMBackend *tb,
 return 0;
 }
 
-static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+static int tpm_emulator_startup_tpm_resume(TPMBackend *tb, size_t buffersize,
+ bool is_resume)
 {
 TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
 ptm_init init = {
@@ -301,12 +317,17 @@ static int tpm_emulator_startup_tpm(TPMBackend *tb, 
size_t buffersize)
 };
 ptm_res res;
 
+trace_tpm_emulator_startup_tpm_resume(is_resume, buffersize);
+
 if (buffersize != 0 &&
 tpm_emulator_set_buffer_size(tb, buffersize, NULL) < 0) {
 goto err_exit;
 }
 
-trace_tpm_emulator_startup_tpm();
+if (is_resume) {
+init.u.req.init_flags |= cpu_to_be32(PTM_INIT_FLAG_DELETE_VOLATILE);
+}
+
 if (tpm_emulator_ctrlcmd(tpm_emu, CMD_INIT, , sizeof(init),
  sizeof(init)) < 0) {
 error_report("tpm-emulator: could not send INIT: %s",
@@ -325,6 +346,11 @@ err_exit:
 return -1;
 }
 
+static int tpm_emulator_startup_tpm(TPMBackend *tb, size_t buffersize)
+{
+return tpm_emulator_startup_tpm_resume(tb, buffersize, false);
+}
+
 static bool tpm_emulator_get_tpm_established_flag(TPMBackend *tb)
 {
 TPMEmulator *tpm_emu = TPM_EMULATOR(tb);
@@ -423,16 +449,21 @@ static size_t tpm_emulator_get_buffer_size(TPMBackend *tb)
 static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
 {
 Error *err = NULL;
+ptm_cap caps = PTM_CAP_GET_STATEBLOB | PTM_CAP_SET_STATEBLOB |
+   PTM_CAP_STOP;
 
-error_setg(_emu->migration_blocker,
-   "Migration disabled: TPM emulator not yet migratable");
-migrate_add_blocker(tpm_emu->migration_blocker, );
-if (err) {
-error_report_err(err);
-error_free(tpm_emu->migration_blocker);
-tpm_emu->migration_blocker = NULL;
+if (!TPM_EMULATOR_IMPLEMENTS_ALL_CAPS(tpm_emu, caps)) {
+error_setg(_emu->migration_blocker,
+   "Migration disabled: TPM emulator does not support "
+   "migration");
+migrate_add_blocker(tpm_emu->migration_blocker, );
+if (err) {
+error_report_err(err);
+error_free(tpm_emu->migration_blocker);
+tpm_emu->migration_blocker = NULL;
 
-return -1;
+return -1;
+}
 }
 
 return 0;
@@ -570,6 +601,267 @@ static const QemuOptDesc tpm_emulator_cmdline_opts[] = {
 { /* end of list */ },
 };
 
+/*
+ * Transfer a TPM state blob from the TPM into a provided buffer.
+ *
+ * @tpm_emu: TPMEmulator
+ * @type: the type of blob to transfer
+ * @tsb: the TPMSizeBuffer to fill with the blob
+ * @flags: the flags to return to the caller
+ */
+static int tpm_emulator_get_state_blob(TPMEmulator *tpm_emu,
+   uint8_t type,
+   TPMSizedBuffer *tsb,
+

Re: [Qemu-devel] About the TCG code in arm64!

[Richard Henderson]

On 05/24/2018 07:53 AM, Zhong, Yang wrote:
> Hello Richard,
> 
> I am coming from intel and ready to disable TCG code in arm64, there are some
> unclear issue in the target/arm directory
> 
> (The TCG code in x86 was disabled by Paolo and me)
>

[Identifying some files that are indeed TCG related.]

> but I checked the code, and felt below helper files are still TCG related 
> files,
> 
> helper.c, helper.h, iwmmxt_helper.c ,  neon_helper.c  vec_helper.c
>  crypto_helper.c 
> 
> Please help correct me if I am wrong, many thanks!


If you are asking if these files are TCG related, the answer is yes to all but
the first.  In helper.c it appears there are things related to gdbstub and
system registers, both of which I believe are still required for kvm and for
migration.

But perhaps a broader audience for the question can give a more complete answer.


r~

Re: [Qemu-devel] [PATCH 4/6] vhost-user: support registering external host notifiers

[Michael S. Tsirkin]

On Fri, May 25, 2018 at 12:05:50AM +0800, Tiwei Bie wrote:
> On Thu, May 24, 2018 at 06:49:47PM +0300, Michael S. Tsirkin wrote:
> > On Thu, May 24, 2018 at 06:33:34PM +0800, Tiwei Bie wrote:
> > > This patch introduces VHOST_USER_PROTOCOL_F_HOST_NOTIFIER.
> > > With this feature negotiated, vhost-user backend can register
> > > memory region based host notifiers. And it will allow the guest
> > > driver in the VM to notify the hardware accelerator at the
> > > vhost-user backend directly.
> > > 
> > > Signed-off-by: Tiwei Bie 
> > 
> > So maybe we don't need a new VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD?
> > Let's check VHOST_USER_PROTOCOL_F_HOST_NOTIFIER instead?
> 
> Yeah. I think it would be the best choice!
> 
> If this feature (HOST NOTIFIER) is negotiated, it means
> the QEMU is able to safely receive (the expected number
> of) file descriptors.
> 
> > 
> > > ---
> [...]
> > > +typedef struct VhostUserHostNotifier {
> > > +MemoryRegion mr;
> > > +void *addr;
> > > +bool set;
> > > +} VhostUserHostNotifier;
> > >  
> > >  typedef struct VhostUserState {
> > >  CharBackend *chr;
> > > +VhostUserHostNotifier notifier[VIRTIO_QUEUE_MAX];
> > >  } VhostUserState;
> > 
> > So this notifier is per-queue. Can't we maintain it in
> > NetVhostUserState then?
> 
> This notifier is per virtio-queue. But NetVhostUserState
> is per net-queue-pair.
> 
> And ideally, I think this structure shouldn't be visible
> to net/vhost-user, vhost-user-scsi, vhost-user-crypto, etc.
> 
> Best regards,
> Tiwei Bie

I wonder whether we can create a new per vq structure.

-- 
MST

[Qemu-devel] [PULL v2 0/4] Merge tpm 2018/05/23

[Stefan Berger]

This series of patches adds TPM emulator state migration support and a
test case for testing (local) migration.

   Stefan

The following changes since commit 4f50c1673a89b07f376ce5c42d22d79a79cd466d:

  Merge remote-tracking branch 'remotes/ehabkost/tags/x86-next-pull-request' 
into staging (2018-05-22 09:43:58 +0100)

are available in the Git repository at:

  git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-05-23-1

for you to fetch changes up to 319bb20d1e88ea2e13baeb158a4989ec5e86:

  test: Add test cases that use the external swtpm with CRB interface 
(2018-05-23 20:35:25 -0400)


Merge tpm 2018/05/23 v1


Stefan Berger (4):
  tpm: extend TPM emulator with state migration support
  tpm: extend TPM TIS with state migration support
  docs: tpm: add VM save/restore example and troubleshooting guide
  test: Add test cases that use the external swtpm with CRB interface

 docs/specs/tpm.txt | 106 +
 hw/tpm/tpm_emulator.c  | 323 
+++---
 hw/tpm/tpm_tis.c   |  52 +-
 hw/tpm/trace-events|   9 +-
 tests/Makefile.include |   3 +
 tests/tpm-crb-swtpm-test.c | 247 
+++
 tests/tpm-util.c   | 186 
 tests/tpm-util.h   |  36 +++
 8 files changed, 948 insertions(+), 14 deletions(-)
 create mode 100644 tests/tpm-crb-swtpm-test.c
 create mode 100644 tests/tpm-util.c
 create mode 100644 tests/tpm-util.h

-- 
2.14.3

[Qemu-devel] [PULL v2 3/4] docs: tpm: add VM save/restore example and troubleshooting guide

[Stefan Berger]

Extend the docs related to TPM with specs related to VM save and
restore and a troubleshooting guide for TPM migration.

Signed-off-by: Stefan Berger 
Reviewed-by: Dr. David Alan Gilbert 
---
 docs/specs/tpm.txt | 106 +
 1 file changed, 106 insertions(+)

diff --git a/docs/specs/tpm.txt b/docs/specs/tpm.txt
index d1d71571e9..c230c4c93e 100644
--- a/docs/specs/tpm.txt
+++ b/docs/specs/tpm.txt
@@ -200,3 +200,109 @@ crw---. 1 root root 10, 224 Jul 11 10:11 /dev/tpm0
 PCR-00: 35 4E 3B CE 23 9F 38 59 ...
 ...
 PCR-23: 00 00 00 00 00 00 00 00 ...
+
+
+=== Migration with the TPM emulator ===
+
+The TPM emulator supports the following types of virtual machine migration:
+
+- VM save / restore (migration into a file)
+- Network migration
+- Snapshotting (migration into storage like QoW2 or QED)
+
+The following command sequences can be used to test VM save / restore.
+
+
+In a 1st terminal start an instance of a swtpm using the following command:
+
+mkdir /tmp/mytpm1
+swtpm socket --tpmstate dir=/tmp/mytpm1 \
+  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock \
+  --log level=20 --tpm2
+
+In a 2nd terminal start the VM:
+
+qemu-system-x86_64 -display sdl -enable-kvm \
+  -m 1024 -boot d -bios bios-256k.bin -boot menu=on \
+  -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+  -tpmdev emulator,id=tpm0,chardev=chrtpm \
+  -device tpm-tis,tpmdev=tpm0 \
+  -monitor stdio \
+  test.img
+
+Verify that the attached TPM is working as expected using applications inside
+the VM.
+
+To store the state of the VM use the following command in the QEMU monitor in
+the 2nd terminal:
+
+(qemu) migrate "exec:cat > testvm.bin"
+(qemu) quit
+
+At this point a file called 'testvm.bin' should exists and the swtpm and QEMU
+processes should have ended.
+
+To test 'VM restore' you have to start the swtpm with the same parameters
+as before. If previously a TPM 2 [--tpm2] was saved, --tpm2 must now be
+passed again on the command line.
+
+In the 1st terminal restart the swtpm with the same command line as before:
+
+swtpm socket --tpmstate dir=/tmp/mytpm1 \
+  --ctrl type=unixio,path=/tmp/mytpm1/swtpm-sock \
+  --log level=20 --tpm2
+
+In the 2nd terminal restore the state of the VM using the additonal
+'-incoming' option.
+
+qemu-system-x86_64 -display sdl -enable-kvm \
+  -m 1024 -boot d -bios bios-256k.bin -boot menu=on \
+  -chardev socket,id=chrtpm,path=/tmp/mytpm1/swtpm-sock \
+  -tpmdev emulator,id=tpm0,chardev=chrtpm \
+  -device tpm-tis,tpmdev=tpm0 \
+  -incoming "exec:cat < testvm.bin" \
+  test.img
+
+
+Troubleshooting migration:
+
+There are several reasons why migration may fail. In case of problems,
+please ensure that the command lines adhere to the following rules and,
+if possible, that identical versions of QEMU and swtpm are used at all
+times.
+
+VM save and restore:
+ - QEMU command line parameters should be identical apart from the
+   '-incoming' option on VM restore
+ - swtpm command line parameters should be identical
+
+VM migration to 'localhost':
+ - QEMU command line parameters should be identical apart from the
+   '-incoming' option on the destination side
+ - swtpm command line parameters should point to two different
+   directories on the source and destination swtpm (--tpmstate dir=...)
+   (especially if different versions of libtpms were to be used on the
+   same machine).
+
+VM migration across the network:
+ - QEMU command line parameters should be identical apart from the
+   '-incoming' option on the destination side
+ - swtpm command line parameters should be identical
+
+VM Snapshotting:
+ - QEMU command line parameters should be identical
+ - swtpm command line parameters should be identical
+
+
+Besides that, migration failure reasons on the swtpm level may include
+the following:
+
+ - the versions of the swtpm on the source and destination sides are
+   incompatible
+   - downgrading of TPM state may not be supported
+   - the source and destination libtpms were compiled with different
+ compile-time options and the destination side refuses to accept the
+ state
+ - different migration keys are used on the source and destination side
+   and the destination side cannot decrypt the migrated state
+   (swtpm ... --migration-key ... )
-- 
2.14.3

Re: [Qemu-devel] [PULL v1 0/4] Merge tpm 2018/05/23

[Stefan Berger]

On 05/24/2018 11:04 AM, Peter Maydell wrote:

On 24 May 2018 at 01:57, Stefan Berger  wrote:

This series of patches adds TPM emulator state migration support and a
test case for testing (local) migration.

Stefan

The following changes since commit 4f50c1673a89b07f376ce5c42d22d79a79cd466d:

   Merge remote-tracking branch 'remotes/ehabkost/tags/x86-next-pull-request' 
into staging (2018-05-22 09:43:58 +0100)

are available in the Git repository at:

   git://github.com/stefanberger/qemu-tpm.git tags/pull-tpm-2018-05-23-1

for you to fetch changes up to 319bb20d1e88ea2e13baeb158a4989ec5e86:

   test: Add test cases that use the external swtpm with CRB interface 
(2018-05-23 20:35:25 -0400)


Merge tpm 2018/05/23 v1


Stefan Berger (4):
   tpm: extend TPM emulator with state migration support
   tpm: extend TPM TIS with state migration support
   docs: tpm: add VM save/restore example and troubleshooting guide
   test: Add test cases that use the external swtpm with CRB interface


Hi. I'm afraid this fails to build on OpenBSD:

   CC  hw/tpm/tpm_emulator.o
/home/qemu/hw/tpm/tpm_emulator.c: In function 'tpm_emulator_set_state_blobs':
/home/qemu/hw/tpm/tpm_emulator.c:794:17: error: 'EBADMSG' undeclared
(first use in this function)
  return -EBADMSG;


Ok. Fixing to -EIO, which is also good.

   Stefan

Re: [Qemu-devel] [PATCH v2 2/1] libqtest: add more exit status checks

[Michael S. Tsirkin]

On Thu, May 24, 2018 at 11:00:19AM -0500, Eric Blake wrote:
> On 05/24/2018 10:52 AM, Eric Blake wrote:
> 
> > Also, since waitpid() can only return either s->qemu_pid or -1 as we
> > aren't using WNOHANG, it may also be worth asserting that if pid == -1,
> > we either have EAGAIN (but why aren't we looping in that case?) or
> > ECHILD.
> 
> I meant EINTR, not EAGAIN.  But in general, using waitpid() to collect
> process status without doing it in a loop is risky.

Interesting. Risky how?

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

[Qemu-devel] [PULL v2 4/4] test: Add test cases that use the external swtpm with CRB interface

[Stefan Berger]

Add a test program for testing the CRB with the external swtpm.

The 1st test case extends a PCR and reads back the value and compares
it against an expected return packet.

The 2nd test case repeats the 1st test case and then migrates the
external swtpm's state along with the VM state to a destination
QEMU and swtpm and checks that the PCR has the expected value now.

The test cases require 'swtpm' to be installed on the system and
in the PATH and 'swtpm' must support the --tpm2 option. If this is
not the case, the test will be skipped.

Signed-off-by: Stefan Berger 
Reviewed-by: Marc-André Lureau 
---
 tests/Makefile.include |   3 +
 tests/tpm-crb-swtpm-test.c | 247 +
 tests/tpm-util.c   | 186 ++
 tests/tpm-util.h   |  36 +++
 4 files changed, 472 insertions(+)
 create mode 100644 tests/tpm-crb-swtpm-test.c
 create mode 100644 tests/tpm-util.c
 create mode 100644 tests/tpm-util.h

diff --git a/tests/Makefile.include b/tests/Makefile.include
index 3b9a5e31a2..b499ba1813 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -297,6 +297,7 @@ check-qtest-i386-$(CONFIG_VHOST_USER_NET_TEST_i386) += 
tests/vhost-user-test$(EX
 ifeq ($(CONFIG_VHOST_USER_NET_TEST_i386),)
 check-qtest-x86_64-$(CONFIG_VHOST_USER_NET_TEST_x86_64) += 
tests/vhost-user-test$(EXESUF)
 endif
+check-qtest-i386-$(CONFIG_TPM) += tests/tpm-crb-swtpm-test$(EXESUF)
 check-qtest-i386-$(CONFIG_TPM) += tests/tpm-crb-test$(EXESUF)
 check-qtest-i386-$(CONFIG_TPM) += tests/tpm-tis-test$(EXESUF)
 check-qtest-i386-$(CONFIG_SLIRP) += tests/test-netfilter$(EXESUF)
@@ -721,6 +722,8 @@ tests/test-util-sockets$(EXESUF): tests/test-util-sockets.o 
\
 tests/test-io-task$(EXESUF): tests/test-io-task.o $(test-io-obj-y)
 tests/test-io-channel-socket$(EXESUF): tests/test-io-channel-socket.o \
 tests/io-channel-helpers.o tests/socket-helpers.o $(test-io-obj-y)
+tests/tpm-crb-swtpm-test$(EXESUF): tests/tpm-crb-swtpm-test.o tests/tpm-emu.o \
+   tests/tpm-util.o $(test-io-obj-y)
 tests/tpm-crb-test$(EXESUF): tests/tpm-crb-test.o tests/tpm-emu.o 
$(test-io-obj-y)
 tests/tpm-tis-test$(EXESUF): tests/tpm-tis-test.o tests/tpm-emu.o 
$(test-io-obj-y)
 tests/test-io-channel-file$(EXESUF): tests/test-io-channel-file.o \
diff --git a/tests/tpm-crb-swtpm-test.c b/tests/tpm-crb-swtpm-test.c
new file mode 100644
index 00..c2bde0cbaa
--- /dev/null
+++ b/tests/tpm-crb-swtpm-test.c
@@ -0,0 +1,247 @@
+/*
+ * QTest testcase for TPM CRB talking to external swtpm and swtpm migration
+ *
+ * Copyright (c) 2018 IBM Corporation
+ *  with parts borrowed from migration-test.c that is:
+ * Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
+ *
+ * Authors:
+ *   Stefan Berger 
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include 
+
+#include "hw/acpi/tpm.h"
+#include "io/channel-socket.h"
+#include "libqtest.h"
+#include "tpm-util.h"
+#include "sysemu/tpm.h"
+#include "qapi/qmp/qdict.h"
+
+typedef struct TestState {
+char *src_tpm_path;
+char *dst_tpm_path;
+char *uri;
+} TestState;
+
+bool got_stop;
+
+static void migrate(QTestState *who, const char *uri)
+{
+QDict *rsp;
+gchar *cmd;
+
+cmd = g_strdup_printf("{ 'execute': 'migrate',"
+  "'arguments': { 'uri': '%s' } }",
+  uri);
+rsp = qtest_qmp(who, cmd);
+g_free(cmd);
+g_assert(qdict_haskey(rsp, "return"));
+qobject_unref(rsp);
+}
+
+/*
+ * Events can get in the way of responses we are actually waiting for.
+ */
+static QDict *wait_command(QTestState *who, const char *command)
+{
+const char *event_string;
+QDict *response;
+
+response = qtest_qmp(who, command);
+
+while (qdict_haskey(response, "event")) {
+/* OK, it was an event */
+event_string = qdict_get_str(response, "event");
+if (!strcmp(event_string, "STOP")) {
+got_stop = true;
+}
+qobject_unref(response);
+response = qtest_qmp_receive(who);
+}
+return response;
+}
+
+static void wait_for_migration_complete(QTestState *who)
+{
+while (true) {
+QDict *rsp, *rsp_return;
+bool completed;
+const char *status;
+
+rsp = wait_command(who, "{ 'execute': 'query-migrate' }");
+rsp_return = qdict_get_qdict(rsp, "return");
+status = qdict_get_str(rsp_return, "status");
+completed = strcmp(status, "completed") == 0;
+g_assert_cmpstr(status, !=,  "failed");
+qobject_unref(rsp);
+if (completed) {
+return;
+}
+usleep(1000);
+}
+}
+
+static void migration_start_qemu(QTestState **src_qemu, QTestState **dst_qemu,
+ SocketAddress 

Re: [Qemu-devel] [qemu-s390x] [PATCH v2 1/2] vfio-ccw: add force unlimited prefetch property

[Cornelia Huck]

On Thu, 24 May 2018 17:42:38 +0200
Halil Pasic  wrote:

> On 05/24/2018 12:29 PM, Halil Pasic wrote:
> > 
> > 
> > On 05/24/2018 09:16 AM, Cornelia Huck wrote:  
> >> On Wed, 23 May 2018 19:28:31 +0200
> >> Halil Pasic  wrote:
> >>  
> >>> On 05/23/2018 06:59 PM, Cornelia Huck wrote:  
>  On Wed, 23 May 2018 18:23:44 +0200
>  Halil Pasic  wrote:  
> > On 05/23/2018 04:46 PM, Cornelia Huck wrote:  
> > +    if (!(sch->orb.ctrl0 & ORB_CTRL0_MASK_PFCH)) {
> > +    if (!(vcdev->force_orb_pfch)) {
> > +    warn_report("vfio-ccw requires PFCH flag set");
> > +    sch_gen_unit_exception(sch);
> > +    css_inject_io_interrupt(sch);
> > +    return IOINST_CC_EXPECTED;
> > +    } else {
> > +    sch->orb.ctrl0 |= ORB_CTRL0_MASK_PFCH;
> > +    WARN_ONCE(vcdev->warned_force_orb_pfch, "PFCH flag 
> > forced");  
>  This message should probably mention vfio-ccw as well as the 
>  subchannel
>  id?  
> >>> I was thinking about this. I think all it would make sense to have a 
> >>> common
> >>> prefix for all reports coming form vfio-ccw (QEMU). But then I was 
> >>> like, that
> >>> is a separate patch.
> >>>
> >>> Maybe something like:
> >>> vfio-ccw (xx.xx.): specific message
> >>>
> >>> OTOH we don't seem to do that elsewhere (git grep -e 
> >>> 'warn\|error_report\|error_setg' -- hw/s390x/).
> >>> AFAIR the error_setg captures context (like, src, line, func) but 
> >>> does not
> >>> necessarily report it. Another question is if this should be extended 
> >>> to
> >>> hw/s390x/s390-ccw.c
> >>>
> >>> What do you think?  
> >> I'm not sure that makes sense, especially as not everything might
> >> explicitly refer to a certain subchannel.
> >>
> >> Let's just add the subchannel id here? In this case, this is really a
> >> useful piece of information (which device is showing this behaviour?)  
> 
> I'm doing the changes right now. And while doing it occurred to to me that
> a device number is probably preferable over the subchannel id, ie. 
> cssid.ssid.devno
> is probably better that cssid.ssid.schid. What we really want to tell is,
> which device is affected and not over which subchannel is this device talking.
> 
> Agree, disagree?

A good argument can be made for both cases: While the admin may care
about the device, we work on the subchannel level. So choose whatever
you think makes most sense, but label it clearly :)

[Qemu-devel] [PATCH v3 2/1] libqtest: add more exit status checks

[Michael S. Tsirkin]

Add more checks on how did QEMU exit.

Legal ways to exit right now:
- exit(0) or return from main
- kill(SIGTERM) - sent by testing infrastructure

Anything else is illegal.

Include explicit asserts on bad behaviour encountered
to make debugging easier.

Signed-off-by: Michael S. Tsirkin 
---

Changes from v2:
- bugfix
- assert returned pid
- rework complex asserts for clarity

Changes from v1:
- drop SIGTERM as suggested by Eric

 tests/libqtest.c | 5 +


 tests/libqtest.c | 12 +++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tests/libqtest.c b/tests/libqtest.c
index f869854..36ca859 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -109,9 +109,19 @@ static void kill_qemu(QTestState *s)
 kill(s->qemu_pid, SIGTERM);
 pid = waitpid(s->qemu_pid, , 0);
 
-if (pid == s->qemu_pid && WIFSIGNALED(wstatus)) {
+/* waitpid returns child PID on success */
+assert(pid == s->qemu_pid);
+
+/* If exited on signal - check the reason: core dump is never OK */
+if (WIFSIGNALED(wstatus)) {
 assert(!WCOREDUMP(wstatus));
 }
+/* If exited normally - check exit status */
+if (WIFEXITED(wstatus)) {
+assert(!WEXITSTATUS(wstatus));
+}
+/* Valid ways to exit: right now only return from main or exit */
+assert(WIFEXITED(wstatus));
 }
 }
 
-- 
MST

Re: [Qemu-devel] [PATCH v7 04/11] hmp: disable monitor in preconfig state

[Markus Armbruster]

Igor Mammedov  writes:

> Ban it for now, if someone would need it to work early,
> one would have to implement checks if HMP command is valid
> at preconfig state.
>
> Signed-off-by: Igor Mammedov 
> Reviewed-by: Eric Blake 
> ---
> v5:
>   * add 'use QMP instead" to error message, suggesting user
> the right interface to use
> v4:
>   * v3 was only printing error but not preventing command execution,
> Fix it by returning after printing error message.
> ("Dr. David Alan Gilbert" )
> ---
>  monitor.c | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index 39f8ee1..0ffdf1d 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3374,6 +3374,12 @@ static void handle_hmp_command(Monitor *mon, const 
> char *cmdline)
>  
>  trace_handle_hmp_command(mon, cmdline);
>  
> +if (runstate_check(RUN_STATE_PRECONFIG)) {
> +monitor_printf(mon, "HMP not available in preconfig state, "
> +"use QMP instead\n");
> +return;
> +}
> +
>  cmd = monitor_parse_command(mon, cmdline, , mon->cmd_table);
>  if (!cmd) {
>  return;

So we offer the user an HMP monitor, but we summarily fail all commands.
I'm sorry, but that's... searching for polite word... embarrassing.  We
should accept HMP output only when we're ready to accept it.  Yes, that
would involve a bit more surgery rather than this cheap hack.  The whole
preconfig thing smells like a cheap hack to me, but let's not overdo it.