[Qemu-devel] [RFC v3 3/8] virtio_iommu: add skeleton

[Eric Auger]

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

Signed-off-by: Eric Auger 

---

v2 -> v3:
- rebase on 2.10-rc0, ie. use IOMMUMemoryRegion and remove
  iommu_ops.
- advertise VIRTIO_IOMMU_F_MAP_UNMAP feature
- page_sizes set to TARGET_PAGE_SIZE
---
 hw/virtio/Makefile.objs  |   1 +
 hw/virtio/virtio-iommu.c | 248 +++
 include/hw/virtio/virtio-iommu.h |  59 ++
 3 files changed, 308 insertions(+)
 create mode 100644 hw/virtio/virtio-iommu.c
 create mode 100644 include/hw/virtio/virtio-iommu.h

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 765d363..8967a4a 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-y += virtio-mmio.o
 
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
+obj-$(CONFIG_LINUX) += virtio-iommu.o
 obj-$(CONFIG_VHOST_VSOCK) += vhost-vsock.o
 obj-y += virtio-crypto.o
 obj-$(CONFIG_VIRTIO_PCI) += virtio-crypto-pci.o
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
new file mode 100644
index 000..4570e19
--- /dev/null
+++ b/hw/virtio/virtio-iommu.c
@@ -0,0 +1,248 @@
+/*
+ * virtio-iommu device
+ *
+ * Copyright (c) 2017 Red Hat, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2 or later, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see .
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/iov.h"
+#include "qemu-common.h"
+#include "hw/virtio/virtio.h"
+#include "sysemu/kvm.h"
+#include "qapi-event.h"
+#include "trace.h"
+
+#include "standard-headers/linux/virtio_ids.h"
+#include 
+
+#include "hw/virtio/virtio-bus.h"
+#include "hw/virtio/virtio-access.h"
+#include "hw/virtio/virtio-iommu.h"
+
+/* Max size */
+#define VIOMMU_DEFAULT_QUEUE_SIZE 256
+
+static int virtio_iommu_handle_attach(VirtIOIOMMU *s,
+  struct iovec *iov,
+  unsigned int iov_cnt)
+{
+return -ENOENT;
+}
+static int virtio_iommu_handle_detach(VirtIOIOMMU *s,
+  struct iovec *iov,
+  unsigned int iov_cnt)
+{
+return -ENOENT;
+}
+static int virtio_iommu_handle_map(VirtIOIOMMU *s,
+   struct iovec *iov,
+   unsigned int iov_cnt)
+{
+return -ENOENT;
+}
+static int virtio_iommu_handle_unmap(VirtIOIOMMU *s,
+ struct iovec *iov,
+ unsigned int iov_cnt)
+{
+return -ENOENT;
+}
+
+static void virtio_iommu_handle_command(VirtIODevice *vdev, VirtQueue *vq)
+{
+VirtIOIOMMU *s = VIRTIO_IOMMU(vdev);
+VirtQueueElement *elem;
+struct virtio_iommu_req_head head;
+struct virtio_iommu_req_tail tail;
+unsigned int iov_cnt;
+struct iovec *iov;
+size_t sz;
+
+for (;;) {
+elem = virtqueue_pop(vq, sizeof(VirtQueueElement));
+if (!elem) {
+return;
+}
+
+if (iov_size(elem->in_sg, elem->in_num) < sizeof(tail) ||
+iov_size(elem->out_sg, elem->out_num) < sizeof(head)) {
+virtio_error(vdev, "virtio-iommu erroneous head or tail");
+virtqueue_detach_element(vq, elem, 0);
+g_free(elem);
+break;
+}
+
+iov_cnt = elem->out_num;
+iov = g_memdup(elem->out_sg, sizeof(struct iovec) * elem->out_num);
+sz = iov_to_buf(iov, iov_cnt, 0, , sizeof(head));
+if (sz != sizeof(head)) {
+tail.status = VIRTIO_IOMMU_S_UNSUPP;
+}
+qemu_mutex_lock(>mutex);
+switch (head.type) {
+case VIRTIO_IOMMU_T_ATTACH:
+tail.status = virtio_iommu_handle_attach(s, iov, iov_cnt);
+break;
+case VIRTIO_IOMMU_T_DETACH:
+tail.status = virtio_iommu_handle_detach(s, iov, iov_cnt);
+break;
+case VIRTIO_IOMMU_T_MAP:
+tail.status = virtio_iommu_handle_map(s, iov, iov_cnt);
+break;
+case VIRTIO_IOMMU_T_UNMAP:
+tail.status = virtio_iommu_handle_unmap(s, iov, iov_cnt);
+break;
+default:
+tail.status = VIRTIO_IOMMU_S_UNSUPP;
+}
+qemu_mutex_unlock(>mutex);
+
+sz = iov_from_buf(elem->in_sg, elem->in_num, 0,
+  , sizeof(tail));
+assert(sz == sizeof(tail));
+
+virtqueue_push(vq, 

[Qemu-devel] [RFC v3 2/8] linux-headers: Update for virtio-iommu

[Eric Auger]

This is a partial linux header update against Jean-Philippe's branch:
git://linux-arm.org/linux-jpb.git virtio-iommu/base (unstable)

Signed-off-by: Eric Auger 
---
 include/standard-headers/linux/virtio_ids.h   |   1 +
 include/standard-headers/linux/virtio_iommu.h | 142 ++
 linux-headers/linux/virtio_iommu.h|   1 +
 3 files changed, 144 insertions(+)
 create mode 100644 include/standard-headers/linux/virtio_iommu.h
 create mode 100644 linux-headers/linux/virtio_iommu.h

diff --git a/include/standard-headers/linux/virtio_ids.h 
b/include/standard-headers/linux/virtio_ids.h
index 6d5c3b2..934ed3d 100644
--- a/include/standard-headers/linux/virtio_ids.h
+++ b/include/standard-headers/linux/virtio_ids.h
@@ -43,5 +43,6 @@
 #define VIRTIO_ID_INPUT18 /* virtio input */
 #define VIRTIO_ID_VSOCK19 /* virtio vsock transport */
 #define VIRTIO_ID_CRYPTO   20 /* virtio crypto */
+#define VIRTIO_ID_IOMMU61216 /* virtio IOMMU (temporary) */
 
 #endif /* _LINUX_VIRTIO_IDS_H */
diff --git a/include/standard-headers/linux/virtio_iommu.h 
b/include/standard-headers/linux/virtio_iommu.h
new file mode 100644
index 000..e139587
--- /dev/null
+++ b/include/standard-headers/linux/virtio_iommu.h
@@ -0,0 +1,142 @@
+/*
+ * Copyright (C) 2017 ARM Ltd.
+ *
+ * This header is BSD licensed so anyone can use the definitions
+ * to implement compatible drivers/servers:
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *notice, this list of conditions and the following disclaimer in the
+ *documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of ARM Ltd. nor the names of its contributors
+ *may be used to endorse or promote products derived from this software
+ *without specific prior written permission.
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * ``AS IS'' AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL IBM OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+#ifndef _LINUX_VIRTIO_IOMMU_H
+#define _LINUX_VIRTIO_IOMMU_H
+
+/* Feature bits */
+#define VIRTIO_IOMMU_F_INPUT_RANGE 0
+#define VIRTIO_IOMMU_F_IOASID_BITS 1
+#define VIRTIO_IOMMU_F_MAP_UNMAP   2
+#define VIRTIO_IOMMU_F_BYPASS  3
+
+QEMU_PACKED
+struct virtio_iommu_config {
+   /* Supported page sizes */
+   uint64_tpage_sizes;
+   struct virtio_iommu_range {
+   uint64_tstart;
+   uint64_tend;
+   } input_range;
+   uint8_t ioasid_bits;
+};
+
+/* Request types */
+#define VIRTIO_IOMMU_T_ATTACH  0x01
+#define VIRTIO_IOMMU_T_DETACH  0x02
+#define VIRTIO_IOMMU_T_MAP 0x03
+#define VIRTIO_IOMMU_T_UNMAP   0x04
+
+/* Status types */
+#define VIRTIO_IOMMU_S_OK  0x00
+#define VIRTIO_IOMMU_S_IOERR   0x01
+#define VIRTIO_IOMMU_S_UNSUPP  0x02
+#define VIRTIO_IOMMU_S_DEVERR  0x03
+#define VIRTIO_IOMMU_S_INVAL   0x04
+#define VIRTIO_IOMMU_S_RANGE   0x05
+#define VIRTIO_IOMMU_S_NOENT   0x06
+#define VIRTIO_IOMMU_S_FAULT   0x07
+
+QEMU_PACKED
+struct virtio_iommu_req_head {
+   uint8_t type;
+   uint8_t reserved[3];
+};
+
+QEMU_PACKED
+struct virtio_iommu_req_tail {
+   uint8_t status;
+   uint8_t reserved[3];
+};
+
+QEMU_PACKED
+struct virtio_iommu_req_attach {
+   struct virtio_iommu_req_headhead;
+
+   uint32_taddress_space;
+   uint32_tdevice;
+   uint32_treserved;
+
+   struct 

[Qemu-devel] [RFC v3 0/8] VIRTIO-IOMMU device

[Eric Auger]

This series implements the virtio-iommu device.

This v3 mostly is a rebase on top of v2.10-rc0 that uses
IOMMUMmeoryRegion plus some small fixes.

This is a proof of concept based on the virtio-iommu specification
written by Jean-Philippe Brucker [1].

The device gets instantiated using the "-device virtio-iommu-device"
option. It currently works with ARM virt machine only, as the machine
must handle the dt binding between the virtio-mmio "iommu" node and
the PCI host bridge node.

ACPI booting is not yet supported.

Best Regards

Eric

This series can be found at:
https://github.com/eauger/qemu/tree/v2.10.0-rc0-virtio-iommu-rfcv3

References:
[1] [RFC 0/3] virtio-iommu: a paravirtualized IOMMU,
[2] [RFC PATCH linux] iommu: Add virtio-iommu driver
[3] [RFC PATCH kvmtool 00/15] Add virtio-iommu

Testing:
- >= 4.12 guest kernel + virtio-iommu driver [2]
- guest using a virtio-net-pci device:
  ,vhost=off,iommu_platform,disable-modern=off,disable-legacy=on

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

v1 -> v2:
- fix redifinition of viommu_as typedef


Eric Auger (8):
  update-linux-headers: import virtio_iommu.h
  linux-headers: Update for virtio-iommu
  virtio_iommu: add skeleton
  virtio-iommu: Decode the command payload
  virtio_iommu: Add the iommu regions
  virtio-iommu: Implement the translation and commands
  hw/arm/virt: Add 2.10 machine type
  hw/arm/virt: Add virtio-iommu the virt board

 hw/arm/virt.c | 116 -
 hw/virtio/Makefile.objs   |   1 +
 hw/virtio/trace-events|  14 +
 hw/virtio/virtio-iommu.c  | 670 ++
 include/hw/arm/virt.h |   5 +
 include/hw/virtio/virtio-iommu.h  |  61 +++
 include/standard-headers/linux/virtio_ids.h   |   1 +
 include/standard-headers/linux/virtio_iommu.h | 142 ++
 linux-headers/linux/virtio_iommu.h|   1 +
 scripts/update-linux-headers.sh   |   3 +
 10 files changed, 1005 insertions(+), 9 deletions(-)
 create mode 100644 hw/virtio/virtio-iommu.c
 create mode 100644 include/hw/virtio/virtio-iommu.h
 create mode 100644 include/standard-headers/linux/virtio_iommu.h
 create mode 100644 linux-headers/linux/virtio_iommu.h

-- 
2.5.5

[Qemu-devel] [RFC v3 1/8] update-linux-headers: import virtio_iommu.h

[Eric Auger]

Update the script to update the virtio_iommu.h header.

Signed-off-by: Eric Auger 
---
 scripts/update-linux-headers.sh | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/scripts/update-linux-headers.sh b/scripts/update-linux-headers.sh
index 2f906c4..03f6712 100755
--- a/scripts/update-linux-headers.sh
+++ b/scripts/update-linux-headers.sh
@@ -134,6 +134,9 @@ EOF
 cat <$output/linux-headers/linux/virtio_config.h
 #include "standard-headers/linux/virtio_config.h"
 EOF
+cat <$output/linux-headers/linux/virtio_iommu.h
+#include "standard-headers/linux/virtio_iommu.h"
+EOF
 cat <$output/linux-headers/linux/virtio_ring.h
 #include "standard-headers/linux/virtio_ring.h"
 EOF
-- 
2.5.5

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Edgar E. Iglesias]

On Tue, Aug 01, 2017 at 11:13:56AM +0200, KONRAD Frederic wrote:
> 
> 
> On 08/01/2017 11:00 AM, Peter Maydell wrote:
> >On 1 August 2017 at 09:10, KONRAD Frederic  
> >wrote:
> >>This adds a migration blocker when mmio_execution has been used.
> >>
> >>Signed-off-by: KONRAD Frederic 
> >>---
> >>  hw/ssi/xilinx_spips.c | 11 +++
> >>  1 file changed, 11 insertions(+)
> >>
> >>diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> >>index e833028..d46491f 100644
> >>--- a/hw/ssi/xilinx_spips.c
> >>+++ b/hw/ssi/xilinx_spips.c
> >>@@ -31,6 +31,8 @@
> >>  #include "hw/ssi/ssi.h"
> >>  #include "qemu/bitops.h"
> >>  #include "hw/ssi/xilinx_spips.h"
> >>+#include "qapi/error.h"
> >>+#include "migration/blocker.h"
> >>
> >>  #ifndef XILINX_SPIPS_ERR_DEBUG
> >>  #define XILINX_SPIPS_ERR_DEBUG 0
> >>@@ -139,6 +141,7 @@ typedef struct {
> >>
> >>  uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
> >>  hwaddr lqspi_cached_addr;
> >>+Error *migration_blocker;
> >>  } XilinxQSPIPS;
> >>
> >>  typedef struct XilinxSPIPSClass {
> >>@@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, 
> >>hwaddr addr, unsigned *size,
> >>  XilinxQSPIPS *q = opaque;
> >>  hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
> >>
> >>+/* mmio_execution breaks migration better aborting than having strange
> >>+ * bugs.
> >>+ */
> >>+if (!q->migration_blocker) {
> >>+error_setg(>migration_blocker, "booting from SPI breaks 
> >>migration");
> >>+migrate_add_blocker(q->migration_blocker, _fatal);
> >>+}
> >>+
> >
> >This doesn't handle the case when migration is already in progress
> >and this function is called (which will cause migrate_add_blocker
> >to fail).
> 
> Maybe I can make the request_ptr to fail if migration is in
> progress.. But is that safe or do I risk a race.
>

Hi Fred,

At this stage, perhaps we should just register the blocker when this dev 
realizes.

If a request_ptr comes in during migration, the VM will fail either way...

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v2 2/2] trace: add trace_event_get_state_backends()

[Daniel P. Berrange]

On Mon, Jul 31, 2017 at 03:07:18PM +0100, Stefan Hajnoczi wrote:
> Code that checks dstate is unaware of SystemTap and LTTng UST dstate, so
> the following trace event will not fire when solely enabled by SystemTap
> or LTTng UST:
> 
>   if (trace_event_get_state(TRACE_MY_EVENT)) {
>   str = g_strdup_printf("Expensive string to generate ...",
> ...);
>   trace_my_event(str);
>   g_free(str);
>   }
> 
> Add trace_event_get_state_backends() to fetch backend dstate.  Those
> backends that use QEMU dstate fetch it as part of
> generate_h_backend_dstate().
> 
> Update existing trace_event_get_state() callers to use
> trace_event_get_state_backends() instead.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
> v2:
>  * Use _backends() postfix to clarify function purpose [Lluís]
> ---
>  docs/devel/tracing.txt |  2 +-
>  trace/control.h| 18 +-
>  hw/usb/hcd-ohci.c  | 13 +
>  net/colo-compare.c | 11 ++-
>  net/filter-rewriter.c  |  4 ++--
>  5 files changed, 31 insertions(+), 17 deletions(-)

Reviewed-by: Daniel P. Berrange 


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

Re: [Qemu-devel] [PATCH v2 1/2] trace: add TRACE__BACKEND_DSTATE()

[Daniel P. Berrange]

On Mon, Jul 31, 2017 at 05:35:11PM +0100, Stefan Hajnoczi wrote:
> On Mon, Jul 31, 2017 at 04:16:39PM +0100, Daniel P. Berrange wrote:
> > On Mon, Jul 31, 2017 at 03:07:17PM +0100, Stefan Hajnoczi wrote:
> > > diff --git a/scripts/tracetool/backend/dtrace.py 
> > > b/scripts/tracetool/backend/dtrace.py
> > > index c6812b70a2..17f902cc62 100644
> > > --- a/scripts/tracetool/backend/dtrace.py
> > > +++ b/scripts/tracetool/backend/dtrace.py
> > > @@ -44,8 +44,20 @@ def generate_h_begin(events, group):
> > >  out('#include "%s"' % header,
> > >  '')
> > >  
> > > +# SystemTap defines __ENABLED() but other DTrace
> > > +# implementations might not.
> > > +for e in events:
> > > +out('#ifndef QEMU_%(uppername)s_ENABLED',
> > > +'#define QEMU_%(uppername)s_ENABLED() false',
> > > +'#endif',
> > > +uppername=e.name.upper())
> > 
> > 
> > IIUC, this means that on other DTrace impls, any trace points guarded by
> > QEMU_*_ENABLED() will never run, even if DTrace probes are set. Having
> > some trace points silently never run makes it pretty useless IMHO.
> > 
> > IOW, you need the opposite, #define it to true. The probe will still
> > only be executed if DTrace has enabled it, but you'll have to take the
> > hit of evaluating the probe arguments regardless. Not as optimized
> > performance-wise, but functionally correct at least.
> 
> Good point!  Thank you, will fix when merging.

Ok, You can add

Reviewed-by: Daniel P. Berrange 

if the change of false to true is made.

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

Re: [Qemu-devel] [Qemu-block] [PATCH v3] qemu-iotests: add a "how to" to ./README

[Stefan Hajnoczi]

On Mon, Jul 31, 2017 at 12:28:44PM -0500, Eric Blake wrote:
> On 07/31/2017 11:26 AM, Stefan Hajnoczi wrote:
> > There is not much getting started documentation for qemu-iotests.  This
> > patch explains how to create a new test and covers the overall testing
> > approach.
> > 
> > Cc: Ishani Chugh 
> > Reviewed-by: Eric Blake 
> > Reviewed-by: Philippe Mathieu-Daudé 
> > Signed-off-by: Stefan Hajnoczi 
> > ---
> 
> >  Please send improvements to the test suite, general feedback or just
> >  reports of failing tests cases to qemu-devel@nongnu.org with a CC:
> >  to qemu-bl...@nongnu.org.
> > +
> > 
> 
> Is this adding a spurious blank line at EOF?

Yes.  Please remove when merging.

Thanks,
Stefan


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[KONRAD Frederic]

On 08/01/2017 11:00 AM, Peter Maydell wrote:

On 1 August 2017 at 09:10, KONRAD Frederic  wrote:

This adds a migration blocker when mmio_execution has been used.

Signed-off-by: KONRAD Frederic 
---
  hw/ssi/xilinx_spips.c | 11 +++
  1 file changed, 11 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index e833028..d46491f 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -31,6 +31,8 @@
  #include "hw/ssi/ssi.h"
  #include "qemu/bitops.h"
  #include "hw/ssi/xilinx_spips.h"
+#include "qapi/error.h"
+#include "migration/blocker.h"

  #ifndef XILINX_SPIPS_ERR_DEBUG
  #define XILINX_SPIPS_ERR_DEBUG 0
@@ -139,6 +141,7 @@ typedef struct {

  uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
  hwaddr lqspi_cached_addr;
+Error *migration_blocker;
  } XilinxQSPIPS;

  typedef struct XilinxSPIPSClass {
@@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr 
addr, unsigned *size,
  XilinxQSPIPS *q = opaque;
  hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);

+/* mmio_execution breaks migration better aborting than having strange
+ * bugs.
+ */
+if (!q->migration_blocker) {
+error_setg(>migration_blocker, "booting from SPI breaks migration");
+migrate_add_blocker(q->migration_blocker, _fatal);
+}
+


This doesn't handle the case when migration is already in progress
and this function is called (which will cause migrate_add_blocker
to fail).


Maybe I can make the request_ptr to fail if migration is in
progress.. But is that safe or do I risk a race.

Fred



thanks
-- PMM

Re: [Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[Peter Maydell]

On 1 August 2017 at 09:10, KONRAD Frederic  wrote:
> This adds a migration blocker when mmio_execution has been used.
>
> Signed-off-by: KONRAD Frederic 
> ---
>  hw/ssi/xilinx_spips.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
> index e833028..d46491f 100644
> --- a/hw/ssi/xilinx_spips.c
> +++ b/hw/ssi/xilinx_spips.c
> @@ -31,6 +31,8 @@
>  #include "hw/ssi/ssi.h"
>  #include "qemu/bitops.h"
>  #include "hw/ssi/xilinx_spips.h"
> +#include "qapi/error.h"
> +#include "migration/blocker.h"
>
>  #ifndef XILINX_SPIPS_ERR_DEBUG
>  #define XILINX_SPIPS_ERR_DEBUG 0
> @@ -139,6 +141,7 @@ typedef struct {
>
>  uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
>  hwaddr lqspi_cached_addr;
> +Error *migration_blocker;
>  } XilinxQSPIPS;
>
>  typedef struct XilinxSPIPSClass {
> @@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr 
> addr, unsigned *size,
>  XilinxQSPIPS *q = opaque;
>  hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
>
> +/* mmio_execution breaks migration better aborting than having strange
> + * bugs.
> + */
> +if (!q->migration_blocker) {
> +error_setg(>migration_blocker, "booting from SPI breaks 
> migration");
> +migrate_add_blocker(q->migration_blocker, _fatal);
> +}
> +

This doesn't handle the case when migration is already in progress
and this function is called (which will cause migrate_add_blocker
to fail).

thanks
-- PMM

Re: [Qemu-devel] [RFC 03/29] io: fix qio_channel_socket_accept err handling

[Dr. David Alan Gilbert]

* Daniel P. Berrange (berra...@redhat.com) wrote:
> On Tue, Aug 01, 2017 at 10:25:19AM +0800, Peter Xu wrote:
> > On Mon, Jul 31, 2017 at 05:53:39PM +0100, Dr. David Alan Gilbert wrote:
> > > * Peter Xu (pet...@redhat.com) wrote:
> > > > When accept failed, we should setup errp with the reason. More
> > > > importantly, the caller may assume errp be non-NULL when error happens,
> > > > and not setting the errp may crash QEMU.
> > > > 
> > > > Signed-off-by: Peter Xu 
> > > > ---
> > > >  io/channel-socket.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > > index 53386b7..7bc308e 100644
> > > > --- a/io/channel-socket.c
> > > > +++ b/io/channel-socket.c
> > > > @@ -344,6 +344,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
> > > >  if (errno == EINTR) {
> > > >  goto retry;
> > > >  }
> > > > +error_setg_errno(errp, errno, "Unable to accept connection");
> > > >  goto error;
> > > 
> > > OK, but this code actually has a bigger problem as well:
> > > 
> > > the original is:
> > > 
> > > cioc->fd = qemu_accept(ioc->fd, (struct sockaddr *)>remoteAddr,
> > >>remoteAddrLen);
> > > if (cioc->fd < 0) {
> > > trace_qio_channel_socket_accept_fail(ioc);
> > > if (errno == EINTR) {
> > > goto retry;
> > > }
> > > goto error;
> > > }
> > > 
> > > Stefan confirmed that trace_ doesn't preserve errno; so the if
> > > following it is wrong.  It needs to preserve errno.
> > 
> > Ah... If so, not sure whether we can do the reservation in trace codes
> > in general?
> > 
> > For this one, I can just move the trace_*() below the errno check.
> > After all, if EINTR is got, it's not really a fail, so imho we should
> > not trace it with "accept fail".
> 
> Agreed, we just need to move the trace below the if.

Peter: Can you split this as a separate patch and it seems OK to try and
put this in 2.10 since it's a strict bug fix.

Dave

> 
> Regards,
> Daniel
> -- 
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 10/29] migration: new property "x-postcopy-fast"

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> On Mon, Jul 31, 2017 at 07:52:24PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> > > This provides a way to start postcopy ASAP when migration starts. To do
> > > this, we need both:
> > > 
> > >   -global migration.x-postcopy-ram=on \
> > >   -global migration.x-postcopy-fast=on
> > 
> > Can you explain why this is necessary?  Both sides already know
> > they're doing a postcopy recovery don't they?
> 
> What I wanted to do here is to provide a way to start postcopy at the
> very beginning (actually it'll possibly start postcopy at the first
> loop in migration_thread), instead of start postcopy until we trigger
> it using "migrate_start_postcopy" command.
> 
> I used it for easier debugging (so I don't need to type
> "migrate_start_postcopy" every time when I trigger postcopy
> migration), meanwhile I think it can also be used when someone really
> want to start postcopy from the very beginning.
> 
> Would such a new parameter makes sense?

Other than debugging, I don't think there's a real use for it; the
slight delay between starting migration and triggering postcopy has
very little cost.

Dave

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

[Qemu-devel] [RFC PATCH] booke206: fix MAS update on tlb miss

[KONRAD Frederic]

When a tlb instruction miss happen, rw is set to 0 at the bottom
of cpu_ppc_handle_mmu_fault which cause the MAS update function to miss
the SAS and TS bit in MAS6, MAS1 in booke206_update_mas_tlb_miss.

Just calling booke206_update_mas_tlb_miss with rw = 2 solve the issue.

Signed-off-by: KONRAD Frederic 
---
 target/ppc/mmu_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
index b7b9088..f06b938 100644
--- a/target/ppc/mmu_helper.c
+++ b/target/ppc/mmu_helper.c
@@ -1551,7 +1551,7 @@ static int cpu_ppc_handle_mmu_fault(CPUPPCState *env, 
target_ulong address,
 env->spr[SPR_40x_ESR] = 0x;
 break;
 case POWERPC_MMU_BOOKE206:
-booke206_update_mas_tlb_miss(env, address, rw);
+booke206_update_mas_tlb_miss(env, address, 2);
 /* fall through */
 case POWERPC_MMU_BOOKE:
 cs->exception_index = POWERPC_EXCP_ITLB;
-- 
1.8.3.1

Re: [Qemu-devel] [RFC 04/29] bitmap: introduce bitmap_invert()

[Dr. David Alan Gilbert]

* Peter Xu (pet...@redhat.com) wrote:
> On Mon, Jul 31, 2017 at 06:11:56PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> > > It is used to invert the whole bitmap.
> > 
> > Would it be easier to change bitmap_complement to use ^
> > in it's macro and slow_bitmap_complement, and then you could call it
> > with src==dst  to do the same thing with just that small change?
> 
> Or, I can directly use that and drop this patch. :-)

Yes, that's fine - note the only difference I see is what happens to the
bits in the last word after the end of the count; your code leaves them
as is, the complement code will zero them on the destination I think.

Dave

> (I didn't really notice that one before)
> 
> Thanks,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-block] [PATCH] block: check BlockDriverState object before dereference

[Paolo Bonzini]

On 01/08/2017 02:14, John Snow wrote:
> I may need some nudging towards understanding what the right solution
> here is, though. Should the blk_aio_flush assume that there always is a
> root BDS? should it not assume that?

I think blk_aio_flush is not special.  If there is no root BDS, either
you return -ENOMEDIUM, or you crash.  But all functions should be doing
the same.

The former makes sense, but right now blk_prwv for one are crashing if
there is no root BDS so the minimum patch would fix the caller rather
than blk_aio_flush.

Paolo

> It's difficult for me to understand right now if the bug is in the
> expectation for the blk_ functions and the caller should be amended, or
> if you need changes to the way the blk_ functions are trying to
> increment a counter that doesn't exist.
> 
> I can handle the former fairly easily; if it's the latter, I'm afraid
> it's stuck in the middle of some of your changes and I'd need a stronger
> hint.

Re: [Qemu-devel] [PATCH v3 1/2] Add more function keys to QEMU

[Daniel P. Berrange]

On Mon, Jul 31, 2017 at 03:44:36PM -0400, John Arbuckle wrote:
> There are now keyboards that have 19 function keys. This patch extends QEMU 
> so these function keys can be used.
> 
> Signed-off-by: John Arbuckle 
> ---
>  qapi-schema.json  | 16 +++-
>  ui/input-keymap.c | 12 
>  2 files changed, 27 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrange 

> 
> diff --git a/qapi-schema.json b/qapi-schema.json
> index c96f0a26f6..f3433aabe6 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -4862,6 +4862,18 @@
>  # @ac_refresh: since 2.10
>  # @ac_bookmarks: since 2.10
>  # altgr, altgr_r: dropped in 2.10
> +# @f13: since 2.11
> +# @f14: since 2.11
> +# @f15: since 2.11
> +# @f16: since 2.11
> +# @f17: since 2.11
> +# @f18: since 2.11
> +# @f19: since 2.11
> +# @f20: since 2.11
> +# @f21: since 2.11
> +# @f22: since 2.11
> +# @f23: since 2.11
> +# @f24: since 2.11
>  #
>  # Since: 1.3.0
>  #
> @@ -4888,7 +4900,9 @@
>  'audionext', 'audioprev', 'audiostop', 'audioplay', 'audiomute',
>  'volumeup', 'volumedown', 'mediaselect',
>  'mail', 'calculator', 'computer',
> -'ac_home', 'ac_back', 'ac_forward', 'ac_refresh', 'ac_bookmarks' 
> ] }
> +'ac_home', 'ac_back', 'ac_forward', 'ac_refresh', 'ac_bookmarks',
> +'f13', 'f14', 'f15', 'f16', 'f17', 'f18', 'f19', 'f20', 'f21',
> +'f22', 'f23', 'f24']}
>  
>  ##
>  # @KeyValue:
> diff --git a/ui/input-keymap.c b/ui/input-keymap.c
> index cf979c2ce9..21a25d9c63 100644
> --- a/ui/input-keymap.c
> +++ b/ui/input-keymap.c
> @@ -251,6 +251,18 @@ static const int qcode_to_number[] = {
>  
>  [Q_KEY_CODE_F11] = 0x57,
>  [Q_KEY_CODE_F12] = 0x58,
> +[Q_KEY_CODE_F13] = 0x5d,
> +[Q_KEY_CODE_F14] = 0x5e,
> +[Q_KEY_CODE_F15] = 0x5f,
> +[Q_KEY_CODE_F16] = 0x55,
> +[Q_KEY_CODE_F17] = 0x83,
> +[Q_KEY_CODE_F18] = 0xf7,
> +[Q_KEY_CODE_F19] = 0x84,
> +[Q_KEY_CODE_F20] = 0x5a,
> +[Q_KEY_CODE_F21] = 0x74,
> +[Q_KEY_CODE_F22] = 0xf9,
> +[Q_KEY_CODE_F23] = 0x6d,
> +[Q_KEY_CODE_F24] = 0x6f,
>  
>  [Q_KEY_CODE_PRINT] = 0xb7,
>  
> -- 
> 2.11.0 (Apple Git-81)
> 

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

Re: [Qemu-devel] [RFC 03/29] io: fix qio_channel_socket_accept err handling

[Daniel P. Berrange]

On Tue, Aug 01, 2017 at 10:25:19AM +0800, Peter Xu wrote:
> On Mon, Jul 31, 2017 at 05:53:39PM +0100, Dr. David Alan Gilbert wrote:
> > * Peter Xu (pet...@redhat.com) wrote:
> > > When accept failed, we should setup errp with the reason. More
> > > importantly, the caller may assume errp be non-NULL when error happens,
> > > and not setting the errp may crash QEMU.
> > > 
> > > Signed-off-by: Peter Xu 
> > > ---
> > >  io/channel-socket.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > > index 53386b7..7bc308e 100644
> > > --- a/io/channel-socket.c
> > > +++ b/io/channel-socket.c
> > > @@ -344,6 +344,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
> > >  if (errno == EINTR) {
> > >  goto retry;
> > >  }
> > > +error_setg_errno(errp, errno, "Unable to accept connection");
> > >  goto error;
> > 
> > OK, but this code actually has a bigger problem as well:
> > 
> > the original is:
> > 
> > cioc->fd = qemu_accept(ioc->fd, (struct sockaddr *)>remoteAddr,
> >>remoteAddrLen);
> > if (cioc->fd < 0) {
> > trace_qio_channel_socket_accept_fail(ioc);
> > if (errno == EINTR) {
> > goto retry;
> > }
> > goto error;
> > }
> > 
> > Stefan confirmed that trace_ doesn't preserve errno; so the if
> > following it is wrong.  It needs to preserve errno.
> 
> Ah... If so, not sure whether we can do the reservation in trace codes
> in general?
> 
> For this one, I can just move the trace_*() below the errno check.
> After all, if EINTR is got, it's not really a fail, so imho we should
> not trace it with "accept fail".

Agreed, we just need to move the trace below the if.


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

Re: [Qemu-devel] [Qemu-arm] [PATCH v2 0/2] Add global device ID in virt machine

[Edgar E. Iglesias]

On Tue, Aug 01, 2017 at 05:05:42AM +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 31, 2017 at 03:13:09PM +, Diana Madalina Craciun wrote:
> > On 07/31/2017 05:06 PM, Michael S. Tsirkin wrote:
> > > On Mon, Jul 31, 2017 at 01:22:45PM +, Diana Madalina Craciun wrote:
> >  If we are to use a value of 0 for the constant in case of PCI devices,
> >  what happens if we have multiple PCI controllers?
> > >>> I guess we'd use the PCI Segment number for that?
> > >>>
> > >>>
> > >> Yes, we can use the PCI segment for this scenario. But this would mean
> > >> different solutions for the same problem. The main problem is that we
> > >> can have multiple entities in the system that are using MSIs (for now
> > >> PCI and NXP non-PCI bus infrastructure
> > >> (https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn.net%2FArticles%2F635905%2F=01%7C01%7Cdiana.craciun%40nxp.com%7C6b0c6c879af64718a21908d4d81d534e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0=bpYMMqajWzgzdbdQgy%2FUYR7y%2BswyvwE%2BqFzs7wdIkkA%3D=0).
> > >>  I guess that we may have other
> > >> platform devices that are using MSIs in the future.
> > >>
> > >> Thanks,
> > >> Diana
> > >>
> > >>
> > > Don't have the time to explore NXP in depth, sorry - there's
> > > a lot of complexity there.
> > > Could you maybe stick some bits to specify bus type in there?
> > > It just looks very wrong to push low level things like this
> > > that users have no interest in up the stack.
> > >
> > Let's generalize the problem a little bit, the NXP details just does not
> > matter much. The problem we have is the following:
> > 
> > The GIC-ITS, the ARM MSI controller is using deviceIDs in order to remap
> > the interrupts. Each device which is expected to send MSIs has a
> > deviceID associated with it. These deviceIDs are configured into devices
> > by software/firmware. There is support in the device tree to specify the
> > correlation between requesterID and deviceID:
> > 
> > "msi-map: Maps a Requester ID to an MSI controller and associated
> >   msi-specifier data. The property is an arbitrary number of tuples of
> >   (rid-base,msi-controller,msi-base,length)"
> > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/pci/pci-msi.txt)
> > 
> > Our problem is that we have to allocate these deviceIDs in QEMU as well
> > and we have to ensure that they are unique. Currently, for PCI, the
> > assumption requesterID=deviceID is made which will no longer be true in
> > case other devices are added. So we need a way (preferable a general
> > one) to allocate these IDs to different devices in the system in a
> > consistent way which will ensure that two devices do not share the same ID.
> 
> My question would be, do other types of devices that are there
> right now have some kind of ID like the requester ID?
> If so I would say just use that, and set high bits in the device ID
> to specify the type (e.g. 00 for pci, etc).
> 
> 
> IMHO if possible that is preferable to pushing this up to users.
> 
> 
> > The reason I put this ID into the controller itself is because on real
> > hardware is actually programmed into the controller. It is needed (for
> > example) when the MSIs are sent.
> > 
> > Thanks,
> > 
> > Diana
> > 
> 
> IIUC what happens on real hardware is controller maps each requester ID
> (or presumably other source ID in the request) to the device ID,
> and the mapping is internal to controller.
> If you wanted a lot of flexibility then looks like you could pass this
> mapping to controllers, but is it really necessary?
> Why don't we build a mapping that's convenient for us?
> 

Hi,

I agree with you for boards that are defined by QEMU (like the ARM virt board).
We can pick a convenient scheme and avoid too much user interaction.

But when we model real SoCs, the IDs and the way bridges convert these IDs
needs to match real HW. Otherwise guest SW will not work unmodified.
To make this work, users should not need to be involved though, it's
the machine and SoC modules that should instantiate the necessary handling
of it, IMO.

Cheers,
Edgar

Re: [Qemu-devel] [PATCH v2 1/2] build-sys: add --disable-vhost-user

[Cornelia Huck]

On Fri, 28 Jul 2017 16:13:08 +0200
Marc-André Lureau  wrote:

> Learn to compile out vhost-user. Keep it enabled by default on
> non-win32, that is assumed to be POSIX. Fail if trying to enable it on
> win32.
> 
> When trying to make a vhost-user netdev, it gives the following error:
> 
> -netdev vhost-user,id=foo,chardev=chr-test: Parameter 'type' expects a netdev 
> backend type
> 
> And similar error with the HMP/QMP monitors.
> 
> While at it, rename CONFIG_VHOST_NET_TEST CONFIG_VHOST_USER_NET_TEST
> since it's a vhost-user specific variable.
> 
> Signed-off-by: Marc-André Lureau 
> ---
>  hw/virtio/virtio-pci.c|  4 ++--
>  configure | 29 +++--
>  default-configs/pci.mak   |  2 +-
>  default-configs/s390x-softmmu.mak |  2 +-
>  tests/Makefile.include|  6 +++---
>  5 files changed, 34 insertions(+), 9 deletions(-)
> 
(...)
> diff --git a/configure b/configure
> index 987f59ba88..efec1a613e 100755
> --- a/configure
> +++ b/configure
> @@ -306,6 +306,7 @@ tcg="yes"
>  vhost_net="no"
>  vhost_scsi="no"
>  vhost_vsock="no"
> +vhost_user=""
>  kvm="no"
>  hax="no"
>  rdma=""
> @@ -1282,6 +1283,15 @@ for opt do
>;;
>--enable-vxhs) vxhs="yes"
>;;
> +  --disable-vhost-user) vhost_user="no"
> +  ;;
> +  --enable-vhost-user)
> +  vhost_user="yes"
> +  if test "$mingw32" = "yes" ; then
> +  echo "ERROR: vhost-user isn't available on win32"
> +  exit 1

error_exit?

> +  fi
> +  ;;
>*)
>echo "ERROR: unknown option $opt"
>echo "Try '$0 --help' for more information"
(...)

> diff --git a/default-configs/s390x-softmmu.mak 
> b/default-configs/s390x-softmmu.mak
> index b227a36179..51191b77df 100644
> --- a/default-configs/s390x-softmmu.mak
> +++ b/default-configs/s390x-softmmu.mak
> @@ -1,6 +1,6 @@
>  CONFIG_PCI=y
>  CONFIG_VIRTIO_PCI=y
> -CONFIG_VHOST_USER_SCSI=$(CONFIG_LINUX)
> +CONFIG_VHOST_USER_SCSI=$(and $(CONFIG_VHOST_USER),$(CONFIG_LINUX))

Huh. I wonder if anyone actually tried this on s390x?

(The change is fine in the context of this patch, of course.)

>  CONFIG_VIRTIO=y
>  CONFIG_SCLPCONSOLE=y
>  CONFIG_TERMINAL3270=y

[Qemu-devel] [PATCH for-2.10 0/2] mmio-execution and migration

[KONRAD Frederic]

mmio-execution has incompatibilities with migration so this informs the
potential developer that implementing request_ptr will break migration and
adds a migration blocker in xilinx-spips when using mmio-execution.

This bug affects the user which wants to execute code from SPI (which wasn't
possible before) and uses migration at the same time.

Let's fix that in the next stable release?

Thanks,
Fred

KONRAD Frederic (2):
  mmio-execution: warn the potential developer about migration
  xilinx-spips: add a migration blocker when using mmio_execution

 hw/ssi/xilinx_spips.c | 11 +++
 include/exec/memory.h |  4 
 2 files changed, 15 insertions(+)

-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.10 2/2] xilinx-spips: add a migration blocker when using mmio_execution

[KONRAD Frederic]

This adds a migration blocker when mmio_execution has been used.

Signed-off-by: KONRAD Frederic 
---
 hw/ssi/xilinx_spips.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/hw/ssi/xilinx_spips.c b/hw/ssi/xilinx_spips.c
index e833028..d46491f 100644
--- a/hw/ssi/xilinx_spips.c
+++ b/hw/ssi/xilinx_spips.c
@@ -31,6 +31,8 @@
 #include "hw/ssi/ssi.h"
 #include "qemu/bitops.h"
 #include "hw/ssi/xilinx_spips.h"
+#include "qapi/error.h"
+#include "migration/blocker.h"
 
 #ifndef XILINX_SPIPS_ERR_DEBUG
 #define XILINX_SPIPS_ERR_DEBUG 0
@@ -139,6 +141,7 @@ typedef struct {
 
 uint8_t lqspi_buf[LQSPI_CACHE_SIZE];
 hwaddr lqspi_cached_addr;
+Error *migration_blocker;
 } XilinxQSPIPS;
 
 typedef struct XilinxSPIPSClass {
@@ -603,6 +606,14 @@ static void *lqspi_request_mmio_ptr(void *opaque, hwaddr 
addr, unsigned *size,
 XilinxQSPIPS *q = opaque;
 hwaddr offset_within_the_region = addr & ~(LQSPI_CACHE_SIZE - 1);
 
+/* mmio_execution breaks migration better aborting than having strange
+ * bugs.
+ */
+if (!q->migration_blocker) {
+error_setg(>migration_blocker, "booting from SPI breaks migration");
+migrate_add_blocker(q->migration_blocker, _fatal);
+}
+
 lqspi_load_cache(opaque, offset_within_the_region);
 *size = LQSPI_CACHE_SIZE;
 *offset = offset_within_the_region;
-- 
1.8.3.1

[Qemu-devel] [PATCH for-2.10 1/2] mmio-execution: warn the potential developer about migration

[KONRAD Frederic]

There are two potential corner cases with mmio-execution:
 * It adds a mmio-interface device which will be migrated.
 * It modifies the RAMBlock list during live migration which odd
   side effects.

Signed-off-by: KONRAD Frederic 
---
 include/exec/memory.h | 4 
 1 file changed, 4 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 400dd44..4ded02c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -154,6 +154,10 @@ struct MemoryRegionOps {
  * @offset is the location of the pointer inside @mr.
  *
  * Returns a pointer to a location which contains guest code.
+ *
+ * Warning: This breaks migration if used before or during migration
+ *  because mmio-interface device will be migrated and because
+ *  RAMBlock list _must_ be static during migration.
  */
 void *(*request_ptr)(void *opaque, hwaddr addr, unsigned *size,
  unsigned *offset);
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] s390x/css: generate solicited crw for rchp completion signaling

[Dong Jia Shi]

A successful completion of rchp should signal a solicited channel path
initialized CRW (channel report word), while the current implementation
always generates an un-solicited one. Let's fix this.

Reported-by: Halil Pasic 
Signed-off-by: Dong Jia Shi 
---
 hw/s390x/css.c | 16 ++--
 include/hw/s390x/css.h |  3 ++-
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 5321ca016b..bfa56f4b12 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -1745,10 +1745,10 @@ int css_do_rchp(uint8_t cssid, uint8_t chpid)
 }
 
 /* We don't really use a channel path, so we're done here. */
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT,
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1,
   channel_subsys.max_cssid > 0 ? 1 : 0, chpid);
 if (channel_subsys.max_cssid > 0) {
-css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 0, real_cssid << 8);
+css_queue_crw(CRW_RSC_CHP, CRW_ERC_INIT, 1, 0, real_cssid << 8);
 }
 return 0;
 }
@@ -2028,7 +2028,8 @@ void css_subch_assign(uint8_t cssid, uint8_t ssid, 
uint16_t schid,
 }
 }
 
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid)
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+   int chain, uint16_t rsid)
 {
 CrwContainer *crw_cont;
 
@@ -2040,6 +2041,9 @@ void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, 
uint16_t rsid)
 return;
 }
 crw_cont->crw.flags = (rsc << 8) | erc;
+if (solicited) {
+crw_cont->crw.flags |= CRW_FLAGS_MASK_S;
+}
 if (chain) {
 crw_cont->crw.flags |= CRW_FLAGS_MASK_C;
 }
@@ -2086,9 +2090,9 @@ void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, 
uint16_t schid,
 }
 chain_crw = (channel_subsys.max_ssid > 0) ||
 (channel_subsys.max_cssid > 0);
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, chain_crw ? 1 : 0, schid);
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, chain_crw ? 1 : 0, schid);
 if (chain_crw) {
-css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0,
+css_queue_crw(CRW_RSC_SUBCH, CRW_ERC_IPI, 0, 0,
   (guest_cssid << 8) | (ssid << 4));
 }
 /* RW_ERC_IPI --> clear pending interrupts */
@@ -2103,7 +2107,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
 void css_generate_css_crws(uint8_t cssid)
 {
 if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, 0, cssid);
 }
 channel_subsys.sei_pending = true;
 }
diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
index 5c5fe6b202..5b017e1fc3 100644
--- a/include/hw/s390x/css.h
+++ b/include/hw/s390x/css.h
@@ -150,7 +150,8 @@ void copy_scsw_to_guest(SCSW *dest, const SCSW *src);
 void css_inject_io_interrupt(SubchDev *sch);
 void css_reset(void);
 void css_reset_sch(SubchDev *sch);
-void css_queue_crw(uint8_t rsc, uint8_t erc, int chain, uint16_t rsid);
+void css_queue_crw(uint8_t rsc, uint8_t erc, int solicited,
+   int chain, uint16_t rsid);
 void css_generate_sch_crws(uint8_t cssid, uint8_t ssid, uint16_t schid,
int hotplugged, int add);
 void css_generate_chp_crws(uint8_t cssid, uint8_t chpid);
-- 
2.11.2

[Qemu-devel] [PATCH v2 0/2] ERC cleanup and CRW bugfix (was: Channel Path realted CRW generation)

[Dong Jia Shi]

This series is trying to:
1. clear up ERC related code
2. bugfix for channel path related CRW generation

Change log
--
v1->v2:
Patch #1:
  Add all ERCs.
  Commit message update.
Patch #2:
  Rename the new added parameter to more speaking name -- solicited.

Dong Jia Shi (2):
  s390x/css: use macro for event-information pending error recover code
  s390x/css: generate solicited crw for rchp completion signaling

 hw/s390x/css.c| 16 ++--
 include/hw/s390x/css.h|  3 ++-
 include/hw/s390x/ioinst.h | 11 +--
 3 files changed, 21 insertions(+), 9 deletions(-)

-- 
2.11.2

[Qemu-devel] [PATCH v2 1/2] s390x/css: use macro for event-information pending error recover code

[Dong Jia Shi]

Let's use a macro for the ERC (error recover code) when generating a
Channel Subsystem Event-information pending CRW (channel report word).

While we are at it, let's also add all other ERCs.

Signed-off-by: Dong Jia Shi 
---
 hw/s390x/css.c|  2 +-
 include/hw/s390x/ioinst.h | 11 +--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/s390x/css.c b/hw/s390x/css.c
index 6a42b95cee..5321ca016b 100644
--- a/hw/s390x/css.c
+++ b/hw/s390x/css.c
@@ -2103,7 +2103,7 @@ void css_generate_chp_crws(uint8_t cssid, uint8_t chpid)
 void css_generate_css_crws(uint8_t cssid)
 {
 if (!channel_subsys.sei_pending) {
-css_queue_crw(CRW_RSC_CSS, 0, 0, cssid);
+css_queue_crw(CRW_RSC_CSS, CRW_ERC_EVENT, 0, cssid);
 }
 channel_subsys.sei_pending = true;
 }
diff --git a/include/hw/s390x/ioinst.h b/include/hw/s390x/ioinst.h
index 92d15655e4..f89019f78f 100644
--- a/include/hw/s390x/ioinst.h
+++ b/include/hw/s390x/ioinst.h
@@ -201,8 +201,15 @@ typedef struct CRW {
 #define CRW_FLAGS_MASK_A 0x0080
 #define CRW_FLAGS_MASK_ERC 0x003f
 
-#define CRW_ERC_INIT 0x02
-#define CRW_ERC_IPI  0x04
+#define CRW_ERC_EVENT0x00 /* event information pending */
+#define CRW_ERC_AVAIL0x01 /* available */
+#define CRW_ERC_INIT 0x02 /* initialized */
+#define CRW_ERC_TERROR   0x03 /* temporary error */
+#define CRW_ERC_IPI  0x04 /* installed parm initialized */
+#define CRW_ERC_TERM 0x05 /* terminal */
+#define CRW_ERC_PERRN0x06 /* perm. error, facility not init */
+#define CRW_ERC_PERRI0x07 /* perm. error, facility init */
+#define CRW_ERC_PMOD 0x08 /* installed parameters modified */
 
 #define CRW_RSC_SUBCH 0x3
 #define CRW_RSC_CHP   0x4
-- 
2.11.2

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Dong Jia Shi]

* Cornelia Huck  [2017-08-01 09:24:20 +0200]:

> On Tue, 1 Aug 2017 10:29:10 +0800
> Dong Jia Shi  wrote:
> 
> > * Cornelia Huck  [2017-07-31 13:13:02 +0200]:
> > 
> > > On Mon, 31 Jul 2017 11:51:37 +0800
> > > Dong Jia Shi  wrote:
> 
> > > > When defining a vfio-ccw device, since the real subchannel implicitly
> > > > indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> > > > my current work, we could even retrieve these information from a new
> > > > added MMIO region). In this case, defining some channel path devices
> > > > separately does not make sense to me.  
> > > 
> > > We might want to pass only a subset of the channel paths to guest. This
> > > can only work if we can define individual chp objects.  
> > Why would we want this?
> 
> For example, if you know that a reconfiguration is coming on soon, you
> can just exclude the paths that will go away anyway and the guest will
> never know about them. Or for preferred pathing, although that one
> fortunately seems to have died out.
> 
> Not very strong reasons to spend time on this, though.
Got it.

> 
> > 
> > We can add, for example, a "chpids" parameter for the "vfio-ccw" device
> > to limit its chpids to a subset that we want it to have? E.g.:
> > 
> > For this mdev:
> > MDEV  Subchan.  PIM PAM POM  CHPIDs
> > --
> > 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 
> > 
> > 
> > We could use this command line:
> > -device 
> > vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245
> >   
> 
> Yes, that would work, should we want that. We can probably do without for now.
> 
Let's deffer this too!

-- 
Dong Jia Shi

Re: [Qemu-devel] [PATCH 3/3] s390x/css: generate channel path initialized CRW for channel path hotplug

[Cornelia Huck]

On Tue, 1 Aug 2017 10:29:10 +0800
Dong Jia Shi  wrote:

> * Cornelia Huck  [2017-07-31 13:13:02 +0200]:
> 
> > On Mon, 31 Jul 2017 11:51:37 +0800
> > Dong Jia Shi  wrote:

> > > When defining a vfio-ccw device, since the real subchannel implicitly
> > > indicates the chps it bound to, we grasp the CHPIDs from sysfs (or, with
> > > my current work, we could even retrieve these information from a new
> > > added MMIO region). In this case, defining some channel path devices
> > > separately does not make sense to me.  
> > 
> > We might want to pass only a subset of the channel paths to guest. This
> > can only work if we can define individual chp objects.  
> Why would we want this?

For example, if you know that a reconfiguration is coming on soon, you
can just exclude the paths that will go away anyway and the guest will
never know about them. Or for preferred pathing, although that one
fortunately seems to have died out.

Not very strong reasons to spend time on this, though.

> 
> We can add, for example, a "chpids" parameter for the "vfio-ccw" device
> to limit its chpids to a subset that we want it to have? E.g.:
> 
> For this mdev:
> MDEV  Subchan.  PIM PAM POM  CHPIDs
> --
> 6dfd3ec5-e8b3-4e18-a6fe-57bc9eceb920  0.0.013f  f0  f0  ff   42434445 
> 
> We could use this command line:
> -device vfio-ccw,sysfsdev=$MDEV_CCW013f,devno=0.0.1234,chpids=4245
>   

Yes, that would work, should we want that. We can probably do without for now.

Re: [Qemu-devel] [PATCH] watchdog: wdt_aspeed: Add support for the reset width register

[Cédric Le Goater]

On 08/01/2017 03:04 AM, Andrew Jeffery wrote:
> The reset width register controls how the pulse on the SoC's WDTRST{1,2}
> pins behaves. A pulse is emitted if the external reset bit is set in
> WDT_CTRL. WDT_RESET_WIDTH requires magic bit patterns to configure both
> push-pull/open-drain and active-high/active-low behaviours and thus
> needs some special handling in the write path.
> 
> Signed-off-by: Andrew Jeffery 
> ---
> I understand that we're in stabilisation mode, but I thought I'd send this out
> to provoke any feedback. Happy to resend after the 2.10 release if required.
> 
>  hw/watchdog/wdt_aspeed.c | 47 +--
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> index 8bbe579b6b66..4ef1412e99fc 100644
> --- a/hw/watchdog/wdt_aspeed.c
> +++ b/hw/watchdog/wdt_aspeed.c
> @@ -14,10 +14,10 @@
>  #include "qemu/timer.h"
>  #include "hw/watchdog/wdt_aspeed.h"
>  
> -#define WDT_STATUS  (0x00 / 4)
> -#define WDT_RELOAD_VALUE(0x04 / 4)
> -#define WDT_RESTART (0x08 / 4)
> -#define WDT_CTRL(0x0C / 4)
> +#define WDT_STATUS  (0x00 / 4)
> +#define WDT_RELOAD_VALUE(0x04 / 4)
> +#define WDT_RESTART (0x08 / 4)
> +#define WDT_CTRL(0x0C / 4)
>  #define   WDT_CTRL_RESET_MODE_SOC   (0x00 << 5)
>  #define   WDT_CTRL_RESET_MODE_FULL_CHIP (0x01 << 5)
>  #define   WDT_CTRL_1MHZ_CLK BIT(4)
> @@ -25,12 +25,21 @@
>  #define   WDT_CTRL_WDT_INTR BIT(2)
>  #define   WDT_CTRL_RESET_SYSTEM BIT(1)
>  #define   WDT_CTRL_ENABLE   BIT(0)
> +#define WDT_RESET_WIDTH (0x18 / 4)
> +#define   WDT_RESET_WIDTH_ACTIVE_HIGH   BIT(31)
> +#define WDT_POLARITY_MASK   (0xFF << 24)
> +#define WDT_ACTIVE_HIGH_MAGIC   (0xA5 << 24)
> +#define WDT_ACTIVE_LOW_MAGIC(0x5A << 24)
> +#define   WDT_RESET_WIDTH_PUSH_PULL BIT(30)
> +#define WDT_DRIVE_TYPE_MASK (0xFF << 24)
> +#define WDT_PUSH_PULL_MAGIC (0xA8 << 24)
> +#define WDT_OPEN_DRAIN_MAGIC(0x8A << 24)
> +#define   WDT_RESET_WIDTH_DURATION  0xFFF;

I would call this define WDT_RESET_WIDTH_DEFAULT (0xFF) and 
use it also in the aspeed_wdt_reset()

I have checked the specs and the bits definitions are correct.
What else could we model ? Would the pulse be interesting ? 

C.


>  
> -#define WDT_TIMEOUT_STATUS  (0x10 / 4)
> -#define WDT_TIMEOUT_CLEAR   (0x14 / 4)
> -#define WDT_RESET_WDITH (0x18 / 4)
> +#define WDT_TIMEOUT_STATUS  (0x10 / 4)
> +#define WDT_TIMEOUT_CLEAR   (0x14 / 4)
>  
> -#define WDT_RESTART_MAGIC   0x4755
> +#define WDT_RESTART_MAGIC   0x4755
>  
>  static bool aspeed_wdt_is_enabled(const AspeedWDTState *s)
>  {
> @@ -55,9 +64,10 @@ static uint64_t aspeed_wdt_read(void *opaque, hwaddr 
> offset, unsigned size)
>  return 0;
>  case WDT_CTRL:
>  return s->regs[WDT_CTRL];
> +case WDT_RESET_WIDTH:
> +return s->regs[WDT_RESET_WIDTH];
>  case WDT_TIMEOUT_STATUS:
>  case WDT_TIMEOUT_CLEAR:
> -case WDT_RESET_WDITH:
>  qemu_log_mask(LOG_UNIMP,
>"%s: uninmplemented read at offset 0x%" HWADDR_PRIx 
> "\n",
>__func__, offset);
> @@ -119,9 +129,25 @@ static void aspeed_wdt_write(void *opaque, hwaddr 
> offset, uint64_t data,
>  timer_del(s->timer);
>  }
>  break;
> +case WDT_RESET_WIDTH:
> +{
> +uint32_t property = data & WDT_POLARITY_MASK;
> +
> +if (property == WDT_ACTIVE_HIGH_MAGIC) {
> +s->regs[WDT_RESET_WIDTH] |= WDT_RESET_WIDTH_ACTIVE_HIGH;
> +} else if (property == WDT_ACTIVE_LOW_MAGIC) {
> +s->regs[WDT_RESET_WIDTH] &= ~WDT_RESET_WIDTH_ACTIVE_HIGH;
> +} else if (property == WDT_PUSH_PULL_MAGIC) {
> +s->regs[WDT_RESET_WIDTH] |= WDT_RESET_WIDTH_PUSH_PULL;
> +} else if (property == WDT_OPEN_DRAIN_MAGIC) {
> +s->regs[WDT_RESET_WIDTH] &= ~WDT_RESET_WIDTH_PUSH_PULL;
> +}
> +s->regs[WDT_RESET_WIDTH] &= ~WDT_RESET_WIDTH_DURATION;
> +s->regs[WDT_RESET_WIDTH] |= data & WDT_RESET_WIDTH_DURATION;
> +break;
> +}
>  case WDT_TIMEOUT_STATUS:
>  case WDT_TIMEOUT_CLEAR:
> -case WDT_RESET_WDITH:
>  qemu_log_mask(LOG_UNIMP,
>"%s: uninmplemented write at offset 0x%" HWADDR_PRIx 
> "\n",
>__func__, offset);
> @@ -167,6 +193,7 @@ static void aspeed_wdt_reset(DeviceState *dev)
>  s->regs[WDT_RELOAD_VALUE] = 0x03EF1480;
>  s->regs[WDT_RESTART] = 0;
>  s->regs[WDT_CTRL] = 0;
> +s->regs[WDT_RESET_WIDTH] = 0XFF;
>  
>  timer_del(s->timer);
>  }
> 

Re: [Qemu-devel] [PATCH 0/3] Channel Path realted CRW generation

[Cornelia Huck]

On Tue, 1 Aug 2017 10:12:31 +0800
Dong Jia Shi  wrote:

> Since I only want to expose the minimum information that the guest needs
> to work without serious problem. I think I can also deffer these stuff
> until we have the good chp modelling.

Yes, that's probably the best way to proceed.

[Qemu-devel] Does qemu guest agent support 'guest-exec'?

[Hu, Robert]

Hi,

qemu/scripts/qmp/qemu-ga-client seems only support "cat, fsfreeze, fstrim, 
halt, ifconfig, info, ping, powerdown, reboot, shutdown, suspend".

But from qemu/qga/commands.c seems at least Linux guest should already support 
this. Despite qemu-ga-client, how can I talk to guest-agent in guest to execute 
some program? any other utils?

Best Regards,
Robert Hoo

Re: [Qemu-devel] [PATCH v2 for-2.11 3/3] qemu-iotests: add option to save temp files on error

[Markus Armbruster]

Jeff Cody  writes:

> Now that ./check takes care of cleaning up after each tests, it
> can also selectively not clean up.  Add option to leave all output from
> tests intact if that test encountered an error.
>
> Note: this currently only works for bash tests, as the python tests
> still clean up after themselves manually.

Should we add a TODO comment for that?

Much appreciated work, by the way.  You might want to mention in one of
your commit messages that this is also a step towards running iotests in
parallel.

Another step towards sanity would be making $TEST_DIR instead of
$source_iotests the current working directory for running tests.

Re: [Qemu-devel] [RFC 11/29] migration: new postcopy-pause state

[Peter Xu]

On Mon, Jul 31, 2017 at 08:06:18PM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (pet...@redhat.com) wrote:
> > Introducing a new state "postcopy-paused", which can be used to pause a
> > postcopy migration. It is targeted to support network failures during
> > postcopy migration. Now when network down for postcopy, the source side
> > will not fail the migration. Instead we convert the status into this new
> > paused state, and we will try to wait for a rescue in the future.
> > 
> > Signed-off-by: Peter Xu 
> 
> I think this should probably be split into:
>a) A patch that adds a new state and the entries in query_migrate etc
>b) A patch that wires up the semaphore and the use of the state.

Reasonable.  Let me split it.

> 
> > ---
> >  migration/migration.c  | 78 
> > +++---
> >  migration/migration.h  |  3 ++
> >  migration/trace-events |  1 +
> >  qapi-schema.json   |  5 +++-
> >  4 files changed, 82 insertions(+), 5 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index efee87e..0bc70c8 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -470,6 +470,7 @@ static bool migration_is_setup_or_active(int state)
> >  switch (state) {
> >  case MIGRATION_STATUS_ACTIVE:
> >  case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > +case MIGRATION_STATUS_POSTCOPY_PAUSED:
> >  case MIGRATION_STATUS_SETUP:
> >  return true;
> >  
> > @@ -545,6 +546,7 @@ MigrationInfo *qmp_query_migrate(Error **errp)
> >  case MIGRATION_STATUS_ACTIVE:
> >  case MIGRATION_STATUS_CANCELLING:
> >  case MIGRATION_STATUS_POSTCOPY_ACTIVE:
> > +case MIGRATION_STATUS_POSTCOPY_PAUSED:
> >   /* TODO add some postcopy stats */
> >  info->has_status = true;
> >  info->has_total_time = true;
> > @@ -991,6 +993,8 @@ static void migrate_fd_cleanup(void *opaque)
> >  
> >  notifier_list_notify(_state_notifiers, s);
> >  block_cleanup_parameters(s);
> > +
> > +qemu_sem_destroy(>postcopy_pause_sem);
> >  }
> >  
> >  void migrate_fd_error(MigrationState *s, const Error *error)
> > @@ -1134,6 +1138,7 @@ MigrationState *migrate_init(void)
> >  s->migration_thread_running = false;
> >  error_free(s->error);
> >  s->error = NULL;
> > +qemu_sem_init(>postcopy_pause_sem, 0);
> >  
> >  migrate_set_state(>state, MIGRATION_STATUS_NONE, 
> > MIGRATION_STATUS_SETUP);
> >  
> > @@ -1942,6 +1947,69 @@ static bool postcopy_should_start(MigrationState *s)
> >  }
> >  
> >  /*
> > + * We don't return until we are in a safe state to continue current
> > + * postcopy migration.  Returns true to continue the migration, or
> > + * false to terminate current migration.
> > + */
> > +static bool postcopy_pause(MigrationState *s)
> > +{
> > +assert(s->state == MIGRATION_STATUS_POSTCOPY_ACTIVE);
> 
> I never like asserts on the sending side.

Indeed aborting on source side is dangerous (e.g., source may loss
data). However this is definitely a "valid assertion" - if current
state is not "postcopy-active", we should be in a very strange state.
If we just continue to run the latter codes, imho it is as dangerous
as if we assert() here and stop the program. Even, that may be more
dangerous considering that we don't really know what will happen
next...

> 
> > +migrate_set_state(>state, MIGRATION_STATUS_POSTCOPY_ACTIVE,
> > +  MIGRATION_STATUS_POSTCOPY_PAUSED);
> > +
> > +/* Current channel is possibly broken. Release it. */
> > +assert(s->to_dst_file);
> > +qemu_file_shutdown(s->to_dst_file);
> > +qemu_fclose(s->to_dst_file);
> > +s->to_dst_file = NULL;
> 
> That does scare me a little; I think it's OK, I'm not sure what happens
> to the ->from_dst_file fd and the return-path processing.

For sockets: I think the QIOChannelSocket.fd will be set to -1 during
close, then the return path code will not be able to read from that
channel any more (it'll get -EIO then as well), and it'll pause as
well. If it was blocking at recvmsg(), it should return with a
failure.

But yes, I think there may have possible indeed risk conditions
between the to/from QEMUFiles considering they are sharing the same
channel... Maybe that is a separate problem of "whether QIO channel
codes are thread safe"? I am not sure of it yet, otherwise we may need
some locking mechanism.

> 
> > +/*
> > + * We wait until things fixed up. Then someone will setup the
> > + * status back for us.
> > + */
> > +while (s->state == MIGRATION_STATUS_POSTCOPY_PAUSED) {
> > +qemu_sem_wait(>postcopy_pause_sem);
> > +}
> 
> Something should get written to stderr prior to this, so when we
> find a migration apparently stuck we can tell why.

Yes I think so.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [RFC 01/29] migration: fix incorrect postcopy recved_bitmap

[Alexey Perevalov]

On 08/01/2017 09:02 AM, Peter Xu wrote:

On Tue, Aug 01, 2017 at 08:48:18AM +0300, Alexey Perevalov wrote:

On 08/01/2017 05:11 AM, Peter Xu wrote:

On Mon, Jul 31, 2017 at 05:34:14PM +0100, Dr. David Alan Gilbert wrote:

* Peter Xu (pet...@redhat.com) wrote:

The bitmap setup during postcopy is incorrectly when the pgaes are huge
pages. Fix it.

Signed-off-by: Peter Xu 
---
  migration/postcopy-ram.c | 2 +-
  migration/ram.c  | 8 
  migration/ram.h  | 2 ++
  3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 276ce12..952b73a 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -578,7 +578,7 @@ static int qemu_ufd_copy_ioctl(int userfault_fd, void 
*host_addr,
  ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, _struct);
  }
  if (!ret) {
-ramblock_recv_bitmap_set(host_addr, rb);
+ramblock_recv_bitmap_set_range(rb, host_addr, pagesize / 
getpagesize());

isn't that   pagesize / qemu_target_page_size() ?

Other than that it looks OK.

Yes, I should have fixed this before.

I guess Alexey will handle this change (along with the copied bitmap
series)?  Anyway, I'll fix it as well in my series, until Alexey post
the new version that I can rebase to.  Thanks,


I'll squash it, and I'll resend it today.
Are you agree to add

Signed-off-by: Peter Xu 

to my patch?

Firstly, if you are squashing the patch, fixing the issue that Dave
has pointed out, please feel free to add my R-b on the patch.

Of course I'll take into account David's suggestion.


I don't know whether it would be suitable to add my S-o-b here - since
most of the patch content is written by you, not me. But I'm totally
fine if you want to include that (btw, thanks for the offer :).

So either one R-b or S-o-b is okay to me.  Thanks,



--
Best regards,
Alexey Perevalov

Re: [Qemu-devel] [Qemu devel v6 PATCH 2/5] msf2: Microsemi Smartfusion2 System Register block.

[sundeep subbaraya]

Hi Philippe,

Ping again :)

Thanks,
Sundeep

On Fri, Jul 21, 2017 at 2:50 PM, sundeep subbaraya 
wrote:

> Hi,
>
> Ping
>
> On Thu, Jul 13, 2017 at 7:51 AM, sundeep subbaraya  > wrote:
>
>> Hi Phiiippe,
>>
>> Gentle reminder.
>>
>> Thanks,
>> Sundeep
>>
>>
>> On Mon, Jul 10, 2017 at 1:55 PM, sundeep subbaraya <
>> sundeep.l...@gmail.com> wrote:
>>
>>> Hi Alistair,
>>>
>>> On Fri, Jul 7, 2017 at 10:03 PM, Alistair Francis 
>>> wrote:
>>>
 On Fri, Jul 7, 2017 at 12:08 AM, sundeep subbaraya
  wrote:
 > Hi Alistair,
 >
 > On Wed, Jul 5, 2017 at 11:36 PM, Alistair Francis <
 alistai...@gmail.com>
 > wrote:
 >>
 >> On Sun, Jul 2, 2017 at 9:45 PM, Subbaraya Sundeep
 >>  wrote:
 >> > Added Sytem register block of Smartfusion2.
 >> > This block has PLL registers which are accessed by guest.
 >> >
 >> > Signed-off-by: Subbaraya Sundeep 
 >> > ---
 >> >  hw/misc/Makefile.objs |   1 +
 >> >  hw/misc/msf2-sysreg.c | 200
 >> > ++
 >> >  include/hw/misc/msf2-sysreg.h |  82 +
 >> >  3 files changed, 283 insertions(+)
 >> >  create mode 100644 hw/misc/msf2-sysreg.c
 >> >  create mode 100644 include/hw/misc/msf2-sysreg.h
 >> >
 >> > diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
 >> > index c8b4893..0f52354 100644
 >> > --- a/hw/misc/Makefile.objs
 >> > +++ b/hw/misc/Makefile.objs
 >> > @@ -56,3 +56,4 @@ obj-$(CONFIG_EDU) += edu.o
 >> >  obj-$(CONFIG_HYPERV_TESTDEV) += hyperv_testdev.o
 >> >  obj-$(CONFIG_AUX) += auxbus.o
 >> >  obj-$(CONFIG_ASPEED_SOC) += aspeed_scu.o aspeed_sdmc.o
 >> > +obj-$(CONFIG_MSF2) += msf2-sysreg.o
 >> > diff --git a/hw/misc/msf2-sysreg.c b/hw/misc/msf2-sysreg.c
 >> > new file mode 100644
 >> > index 000..64ee141
 >> > --- /dev/null
 >> > +++ b/hw/misc/msf2-sysreg.c
 >> > @@ -0,0 +1,200 @@
 >> > +/*
 >> > + * System Register block model of Microsemi SmartFusion2.
 >> > + *
 >> > + * Copyright (c) 2017 Subbaraya Sundeep 
 >> > + *
 >> > + * This program is free software; you can redistribute it and/or
 >> > + * modify it under the terms of the GNU General Public License
 >> > + * as published by the Free Software Foundation; either version
 >> > + * 2 of the License, or (at your option) any later version.
 >> > + *
 >> > + * You should have received a copy of the GNU General Public
 License
 >> > along
 >> > + * with this program; if not, see .
 >> > + */
 >> > +
 >> > +#include "hw/misc/msf2-sysreg.h"
 >>
 >> Same #include comment from patch 1.
 >
 >
 > Ok will change.
 >>
 >>
 >> > +
 >> > +#ifndef MSF2_SYSREG_ERR_DEBUG
 >> > +#define MSF2_SYSREG_ERR_DEBUG  0
 >> > +#endif
 >> > +
 >> > +#define DB_PRINT_L(lvl, fmt, args...) do { \
 >> > +if (MSF2_SYSREG_ERR_DEBUG >= lvl) { \
 >> > +qemu_log("%s: " fmt "\n", __func__, ## args); \
 >> > +} \
 >> > +} while (0);
 >> > +
 >> > +#define DB_PRINT(fmt, args...) DB_PRINT_L(1, fmt, ## args)
 >> > +
 >> > +static inline int msf2_divbits(uint32_t div)
 >> > +{
 >> > +int ret = 0;
 >> > +
 >> > +switch (div) {
 >> > +case 1:
 >> > +ret = 0;
 >> > +break;
 >> > +case 2:
 >> > +ret = 1;
 >> > +break;
 >> > +case 4:
 >> > +ret = 2;
 >> > +break;
 >> > +case 8:
 >> > +ret = 4;
 >> > +break;
 >> > +case 16:
 >> > +ret = 5;
 >> > +break;
 >> > +case 32:
 >> > +ret = 6;
 >> > +break;
 >> > +default:
 >> > +break;
 >> > +}
 >> > +
 >> > +return ret;
 >> > +}
 >> > +
 >> > +static void msf2_sysreg_reset(DeviceState *d)
 >> > +{
 >> > +MSF2SysregState *s = MSF2_SYSREG(d);
 >> > +
 >> > +DB_PRINT("RESET");
 >> > +
 >> > +s->regs[MSSDDR_PLL_STATUS_LOW_CR] = 0x021A2358;
 >> > +s->regs[MSSDDR_PLL_STATUS] = 0x3;
 >> > +s->regs[MSSDDR_FACC1_CR] = msf2_divbits(s->apb0div) << 5 |
 >> > +   msf2_divbits(s->apb1div) << 2;
 >> > +}
 >> > +
 >> > +static uint64_t msf2_sysreg_read(void *opaque, hwaddr offset,
 >> > +unsigned size)
 >> > +{
 >> > +MSF2SysregState *s = opaque;
 >> > +offset /= 4;
 >>
 >> Probably best to use a bitshift.
 >
 >
 > Ok will change.
 >>
 >>
 >> > +uint32_t ret = 0;
 >> > +
 >> > +if (offset < ARRAY_SIZE(s->regs)) {
 >> > + 

Re: [Qemu-devel] [RFC 01/29] migration: fix incorrect postcopy recved_bitmap

[Peter Xu]

On Tue, Aug 01, 2017 at 08:48:18AM +0300, Alexey Perevalov wrote:
> On 08/01/2017 05:11 AM, Peter Xu wrote:
> >On Mon, Jul 31, 2017 at 05:34:14PM +0100, Dr. David Alan Gilbert wrote:
> >>* Peter Xu (pet...@redhat.com) wrote:
> >>>The bitmap setup during postcopy is incorrectly when the pgaes are huge
> >>>pages. Fix it.
> >>>
> >>>Signed-off-by: Peter Xu 
> >>>---
> >>>  migration/postcopy-ram.c | 2 +-
> >>>  migration/ram.c  | 8 
> >>>  migration/ram.h  | 2 ++
> >>>  3 files changed, 11 insertions(+), 1 deletion(-)
> >>>
> >>>diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> >>>index 276ce12..952b73a 100644
> >>>--- a/migration/postcopy-ram.c
> >>>+++ b/migration/postcopy-ram.c
> >>>@@ -578,7 +578,7 @@ static int qemu_ufd_copy_ioctl(int userfault_fd, void 
> >>>*host_addr,
> >>>  ret = ioctl(userfault_fd, UFFDIO_ZEROPAGE, _struct);
> >>>  }
> >>>  if (!ret) {
> >>>-ramblock_recv_bitmap_set(host_addr, rb);
> >>>+ramblock_recv_bitmap_set_range(rb, host_addr, pagesize / 
> >>>getpagesize());
> >>isn't that   pagesize / qemu_target_page_size() ?
> >>
> >>Other than that it looks OK.
> >Yes, I should have fixed this before.
> >
> >I guess Alexey will handle this change (along with the copied bitmap
> >series)?  Anyway, I'll fix it as well in my series, until Alexey post
> >the new version that I can rebase to.  Thanks,
> >
> I'll squash it, and I'll resend it today.
> Are you agree to add
> 
> Signed-off-by: Peter Xu 
> 
> to my patch?

Firstly, if you are squashing the patch, fixing the issue that Dave
has pointed out, please feel free to add my R-b on the patch.

I don't know whether it would be suitable to add my S-o-b here - since
most of the patch content is written by you, not me. But I'm totally
fine if you want to include that (btw, thanks for the offer :).

So either one R-b or S-o-b is okay to me.  Thanks,

-- 
Peter Xu