date:20200117


On 1/15/20 1:36 PM, Damien Hedde wrote:

Provide a temporary device_legacy_reset function doing what
device_reset does to prepare for the transition with Resettable
API.

All occurrence of device_reset in the code tree are also replaced
by device_legacy_reset.

The new resettable API has different prototype and semantics
(resetting child buses as well as the specified device). Subsequent
commits will make the changeover for each call site individually; once
that is complete device_legacy_reset() will be removed.

Signed-off-by: Damien Hedde 
Reviewed-by: Peter Maydell 
Reviewed-by: Richard Henderson 
Acked-by: David Gibson 
Acked-by: Cornelia Huck 
---
v7 update: added new occurence due to rebase

Cc: Gerd Hoffmann 
Cc: Paolo Bonzini 
Cc: "Daniel P. Berrangé" 
Cc: Eduardo Habkost 
Cc: Richard Henderson 
Cc: "Michael S. Tsirkin" 
Cc: Marcel Apfelbaum 
Cc: John Snow 
Cc: "Cédric Le Goater" 
Cc: Collin Walling 
Cc: Cornelia Huck 
Cc: David Hildenbrand 
Cc: Halil Pasic 
Cc: Christian Borntraeger 
Cc: Dmitry Fleytman 
Cc: Fam Zheng 
---
  include/hw/qdev-core.h   | 4 ++--
  hw/audio/intel-hda.c | 2 +-
  hw/core/qdev.c   | 6 +++---
  hw/hyperv/hyperv.c   | 2 +-
  hw/i386/microvm.c| 2 +-
  hw/i386/pc.c | 2 +-
  hw/ide/microdrive.c  | 8 
  hw/intc/spapr_xive.c | 2 +-
  hw/ppc/pnv_psi.c | 4 ++--
  hw/ppc/spapr_pci.c   | 2 +-
  hw/ppc/spapr_vio.c   | 2 +-
  hw/s390x/s390-pci-inst.c | 2 +-
  hw/scsi/vmw_pvscsi.c | 2 +-
  hw/sd/omap_mmc.c | 2 +-
  hw/sd/pl181.c| 2 +-
  15 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1518495b1e..b8341b0fb0 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -427,11 +427,11 @@ char *qdev_get_own_fw_dev_path_from_handler(BusState 
*bus, DeviceState *dev);
  void qdev_machine_init(void);
  
  /**

- * @device_reset
+ * device_legacy_reset:
   *
   * Reset a single device (by calling the reset method).
   */
-void device_reset(DeviceState *dev);
+void device_legacy_reset(DeviceState *dev);
  
  void device_class_set_parent_reset(DeviceClass *dc,

 DeviceReset dev_reset,
diff --git a/hw/audio/intel-hda.c b/hw/audio/intel-hda.c
index 6ecd383540..27b71c57cf 100644
--- a/hw/audio/intel-hda.c
+++ b/hw/audio/intel-hda.c
@@ -1087,7 +1087,7 @@ static void intel_hda_reset(DeviceState *dev)
  QTAILQ_FOREACH(kid, >codecs.qbus.children, sibling) {
  DeviceState *qdev = kid->child;
  cdev = HDA_CODEC_DEVICE(qdev);
-device_reset(DEVICE(cdev));
+device_legacy_reset(DEVICE(cdev));
  d->state_sts |= (1 << cdev->cad);
  }
  intel_hda_update_irq(d);
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 9f1753f5cf..8c0c8284c8 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -298,7 +298,7 @@ HotplugHandler *qdev_get_hotplug_handler(DeviceState *dev)
  
  static int qdev_reset_one(DeviceState *dev, void *opaque)

  {
-device_reset(dev);
+device_legacy_reset(dev);
  
  return 0;

  }
@@ -895,7 +895,7 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
  }
  }
  if (dev->hotplugged) {
-device_reset(dev);
+device_legacy_reset(dev);
  }
  dev->pending_deleted_event = false;
  
@@ -1127,7 +1127,7 @@ void device_class_set_parent_unrealize(DeviceClass *dc,

  dc->unrealize = dev_unrealize;
  }
  
-void device_reset(DeviceState *dev)

+void device_legacy_reset(DeviceState *dev)
  {
  DeviceClass *klass = DEVICE_GET_CLASS(dev);
  
diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c

index da8ce82725..8ca3706f5b 100644
--- a/hw/hyperv/hyperv.c
+++ b/hw/hyperv/hyperv.c
@@ -140,7 +140,7 @@ void hyperv_synic_reset(CPUState *cs)
  SynICState *synic = get_synic(cs);
  
  if (synic) {

-device_reset(DEVICE(synic));
+device_legacy_reset(DEVICE(synic));
  }
  }
  
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c

index 827ce29e58..d23485108d 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -370,7 +370,7 @@ static void microvm_machine_reset(MachineState *machine)
  cpu = X86_CPU(cs);
  
  if (cpu->apic_state) {

-device_reset(cpu->apic_state);
+device_legacy_reset(cpu->apic_state);
  }
  }
  }
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 8054bc4147..91c7fdaab1 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1877,7 +1877,7 @@ static void pc_machine_reset(MachineState *machine)
  cpu = X86_CPU(cs);
  
  if (cpu->apic_state) {

-device_reset(cpu->apic_state);
+device_legacy_reset(cpu->apic_state);
  }
  }
  }
diff --git a/hw/ide/microdrive.c b/hw/ide/microdrive.c
index b0272ea14b..6b30e36ed8 100644
--- a/hw/ide/microdrive.c
+++ b/hw/ide/microdrive.c
@@ -173,7 +173,7 @@ static void md_attr_write(PCMCIACardState

Re: [PATCH v7 08/11] hw/core: deprecate old reset functions and introduce new ones


On 1/15/20 1:36 PM, Damien Hedde wrote:

Deprecate device_legacy_reset(), qdev_reset_all() and
qbus_reset_all() to be replaced by new functions
device_cold_reset() and bus_cold_reset() which uses resettable API.

Also introduce resettable_cold_reset_fn() which may be used as a
replacement for qdev_reset_all_fn and qbus_reset_all_fn().

Following patches will be needed to look at legacy reset call sites
and switch to resettable api. The legacy functions will be removed
when unused.

Signed-off-by: Damien Hedde 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Peter Maydell 
Reviewed-by: Richard Henderson 
---
  include/hw/qdev-core.h  | 27 +++
  include/hw/resettable.h |  9 +
  hw/core/bus.c   |  5 +
  hw/core/qdev.c  |  5 +
  hw/core/resettable.c|  5 +
  5 files changed, 51 insertions(+)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 1b4b420617..b84fcc32bf 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -406,6 +406,13 @@ int qdev_walk_children(DeviceState *dev,
 qdev_walkerfn *post_devfn, qbus_walkerfn *post_busfn,
 void *opaque);
  
+/**

+ * @qdev_reset_all:
+ * Reset @dev. See @qbus_reset_all() for more details.
+ *
+ * Note: This function is deprecated and will be removed when it becomes 
unused.
+ * Please use device_cold_reset() now.
+ */
  void qdev_reset_all(DeviceState *dev);
  void qdev_reset_all_fn(void *opaque);
  
@@ -418,10 +425,28 @@ void qdev_reset_all_fn(void *opaque);

   * hard reset means that qbus_reset_all will reset all state of the device.
   * For PCI devices, for example, this will include the base address registers
   * or configuration space.
+ *
+ * Note: This function is deprecated and will be removed when it becomes 
unused.
+ * Please use bus_cold_reset() now.
   */
  void qbus_reset_all(BusState *bus);
  void qbus_reset_all_fn(void *opaque);
  
+/**

+ * device_cold_reset:
+ * Reset device @dev and perform a recursive processing using the resettable
+ * interface. It triggers a RESET_TYPE_COLD.
+ */
+void device_cold_reset(DeviceState *dev);
+
+/**
+ * bus_cold_reset:
+ *
+ * Reset bus @bus and perform a recursive processing using the resettable
+ * interface. It triggers a RESET_TYPE_COLD.
+ */
+void bus_cold_reset(BusState *bus);
+
  /**
   * device_is_in_reset:
   * Return true if the device @dev is currently being reset.
@@ -452,6 +477,8 @@ void qdev_machine_init(void);
   * device_legacy_reset:
   *
   * Reset a single device (by calling the reset method).
+ * Note: This function is deprecated and will be removed when it becomes 
unused.
+ * Please use device_cold_reset() now.
   */
  void device_legacy_reset(DeviceState *dev);
  
diff --git a/include/hw/resettable.h b/include/hw/resettable.h

index 3f90da5b5b..0c1adf49ee 100644
--- a/include/hw/resettable.h
+++ b/include/hw/resettable.h
@@ -221,6 +221,15 @@ bool resettable_is_in_reset(Object *obj);
   */
  void resettable_change_parent(Object *obj, Object *newp, Object *oldp);
  
+/**

+ * resettable_cold_reset_fn:
+ * Helper to call resettable_reset((Object *) opaque, RESET_TYPE_COLD).
+ *
+ * This function is typically useful to register a reset handler with
+ * qemu_register_reset.
+ */
+void resettable_cold_reset_fn(void *opaque);
+
  /**
   * resettable_class_set_parent_phases:
   *
diff --git a/hw/core/bus.c b/hw/core/bus.c
index 2698f715bd..3dc0a825f0 100644
--- a/hw/core/bus.c
+++ b/hw/core/bus.c
@@ -68,6 +68,11 @@ int qbus_walk_children(BusState *bus,
  return 0;
  }
  
+void bus_cold_reset(BusState *bus)

+{
+resettable_reset(OBJECT(bus), RESET_TYPE_COLD);
+}
+
  bool bus_is_in_reset(BusState *bus)
  {
  return resettable_is_in_reset(OBJECT(bus));
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 5fac4cd8fc..a7a7abe569 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -361,6 +361,11 @@ void qbus_reset_all_fn(void *opaque)
  qbus_reset_all(bus);
  }
  
+void device_cold_reset(DeviceState *dev)

+{
+resettable_reset(OBJECT(dev), RESET_TYPE_COLD);
+}
+
  bool device_is_in_reset(DeviceState *dev)
  {
  return resettable_is_in_reset(OBJECT(dev));
diff --git a/hw/core/resettable.c b/hw/core/resettable.c
index e99bb30058..7647ef6863 100644
--- a/hw/core/resettable.c
+++ b/hw/core/resettable.c
@@ -264,6 +264,11 @@ void resettable_change_parent(Object *obj, Object *newp, 
Object *oldp)
  }
  }
  
+void resettable_cold_reset_fn(void *opaque)

+{
+resettable_reset((Object *) opaque, RESET_TYPE_COLD);
+}
+
  void resettable_class_set_parent_phases(ResettableClass *rc,
  ResettableEnterPhase enter,
  ResettableHoldPhase hold,



Tested-by: Philippe Mathieu-Daudé

Re: [PATCH v7 07/11] hw/core/qdev: update hotplug reset regarding resettable


On 1/15/20 1:36 PM, Damien Hedde wrote:

This commit make use of the resettable API to reset the device being
hotplugged when it is realized. Also it ensures it is put in a reset
state coherent with the parent it is plugged into.

Note that there is a difference in the reset. Instead of resetting
only the hotplugged device, we reset also its subtree (switch to
resettable API). This is not expected to be a problem because
sub-buses are just realized too. If a hotplugged device has any
sub-buses it is logical to reset them too at this point.

The recently added should_be_hidden and PCI's partially_hotplugged
mechanisms do not interfere with realize operation:
+ In the should_be_hidden use case, device creation is
delayed.
+ The partially_hotplugged mechanism prevents a device to be
unplugged and unrealized from qdev POV and unrealized.

Signed-off-by: Damien Hedde 
Reviewed-by: Richard Henderson 
---

v7 update: inline resettable_state_clear()

v6 update: clear the reset state before doing any reset. Although it
is apparently highly improbable that a device be realized again after
being unrealized. See here for a discussion about this point and the
partially_hotplugged/shoud_be_hidden cases.
https://lists.nongnu.org/archive/html/qemu-devel/2019-11/msg04897.html
---
  include/hw/resettable.h | 11 +++
  hw/core/qdev.c  | 15 ++-
  2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/hw/resettable.h b/include/hw/resettable.h
index 3c87ab86c7..3f90da5b5b 100644
--- a/include/hw/resettable.h
+++ b/include/hw/resettable.h
@@ -153,6 +153,17 @@ struct ResettableState {
  bool exit_phase_in_progress;
  };
  
+/**

+ * resettable_state_clear:
+ * Clear the state. It puts the state to the initial (zeroed) state required
+ * to reuse an object. Typically used in realize step of base classes
+ * implementing the interface.
+ */
+static inline void resettable_state_clear(ResettableState *state)
+{
+memset(state, 0, sizeof(ResettableState));
+}
+
  /**
   * resettable_reset:
   * Trigger a reset on an object @obj of type @type. @obj must implement
diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 310b87e25a..5fac4cd8fc 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -937,6 +937,12 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
  }
  }
  
+/*

+ * Clear the reset state, in case the object was previously unrealized
+ * with a dirty state.
+ */
+resettable_state_clear(>reset);
+
  QLIST_FOREACH(bus, >child_bus, sibling) {
  object_property_set_bool(OBJECT(bus), true, "realized",
   _err);
@@ -945,7 +951,14 @@ static void device_set_realized(Object *obj, bool value, 
Error **errp)
  }
  }
  if (dev->hotplugged) {
-device_legacy_reset(dev);
+/*
+ * Reset the device, as well as its subtree which, at this point,
+ * should be realized too.
+ */
+resettable_assert_reset(OBJECT(dev), RESET_TYPE_COLD);
+resettable_change_parent(OBJECT(dev), OBJECT(dev->parent_bus),
+ NULL);
+resettable_release_reset(OBJECT(dev), RESET_TYPE_COLD);
  }
  dev->pending_deleted_event = false;
  



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

Re: [PATCH v7 05/11] hw/core/resettable: add support for changing parent


On 1/15/20 1:36 PM, Damien Hedde wrote:

Add a function resettable_change_parent() to do the required
plumbing when changing the parent a of Resettable object.

We need to make sure that the reset state of the object remains
coherent with the reset state of the new parent.

We make the 2 following hypothesis:
+ when an object is put in a parent under reset, the object goes in
reset.
+ when an object is removed from a parent under reset, the object
leaves reset.

The added function avoids any glitch if both old and new parent are
already in reset.

Signed-off-by: Damien Hedde 
Reviewed-by: Richard Henderson 
---
  include/hw/resettable.h | 16 +++
  hw/core/resettable.c| 62 +++--
  hw/core/trace-events|  1 +
  3 files changed, 77 insertions(+), 2 deletions(-)

diff --git a/include/hw/resettable.h b/include/hw/resettable.h
index 58b3df4c22..3c87ab86c7 100644
--- a/include/hw/resettable.h
+++ b/include/hw/resettable.h
@@ -194,6 +194,22 @@ void resettable_release_reset(Object *obj, ResetType type);
   */
  bool resettable_is_in_reset(Object *obj);
  
+/**

+ * resettable_change_parent:
+ * Indicate that the parent of Ressettable @obj is changing from @oldp to 
@newp.
+ * All 3 objects must implement resettable interface. @oldp or @newp may be
+ * NULL.
+ *
+ * This function will adapt the reset state of @obj so that it is coherent
+ * with the reset state of @newp. It may trigger @resettable_assert_reset()
+ * or @resettable_release_reset(). It will do such things only if the reset
+ * state of @newp and @oldp are different.
+ *
+ * When using this function during reset, it must only be called during
+ * a hold phase method. Calling this during enter or exit phase is an error.
+ */
+void resettable_change_parent(Object *obj, Object *newp, Object *oldp);
+
  /**
   * resettable_class_set_parent_phases:
   *
diff --git a/hw/core/resettable.c b/hw/core/resettable.c
index 9133208487..e99bb30058 100644
--- a/hw/core/resettable.c
+++ b/hw/core/resettable.c
@@ -28,12 +28,16 @@ static void resettable_phase_exit(Object *obj, void 
*opaque, ResetType type);
   * enter_phase_in_progress:
   * True if we are currently in reset enter phase.
   *
- * Note: This flag is only used to guarantee (using asserts) that the reset
- * API is used correctly. We can use a global variable because we rely on the
+ * exit_phase_in_progress:
+ * count the number of exit phase we are in.
+ *
+ * Note: These flags are only used to guarantee (using asserts) that the reset
+ * API is used correctly. We can use global variables because we rely on the
   * iothread mutex to ensure only one reset operation is in a progress at a
   * given time.
   */
  static bool enter_phase_in_progress;
+static unsigned exit_phase_in_progress;
  
  void resettable_reset(Object *obj, ResetType type)

  {
@@ -65,7 +69,9 @@ void resettable_release_reset(Object *obj, ResetType type)
  trace_resettable_reset_release_begin(obj, type);
  assert(!enter_phase_in_progress);
  
+exit_phase_in_progress += 1;

  resettable_phase_exit(obj, NULL, type);
+exit_phase_in_progress -= 1;
  
  trace_resettable_reset_release_end(obj);

  }
@@ -206,6 +212,58 @@ static void resettable_phase_exit(Object *obj, void 
*opaque, ResetType type)
  trace_resettable_phase_exit_end(obj, obj_typename, s->count);
  }
  
+/*

+ * resettable_get_count:
+ * Get the count of the Resettable object @obj. Return 0 if @obj is NULL.
+ */
+static uint32_t resettable_get_count(Object *obj)


In patch #3 I suggested to use 'unsigned' instead (and add the new 
ResettableState::count field here with the rest of this current patch).


Using 'unsigned' imply trivial format string update in trace events, so 
regardless:


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


+{
+if (obj) {
+ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+return rc->get_state(obj)->count;
+}
+return 0;
+}
+
+void resettable_change_parent(Object *obj, Object *newp, Object *oldp)
+{
+ResettableClass *rc = RESETTABLE_GET_CLASS(obj);
+ResettableState *s = rc->get_state(obj);
+uint32_t newp_count = resettable_get_count(newp);
+uint32_t oldp_count = resettable_get_count(oldp);
+
+/*
+ * Ensure we do not change parent when in enter or exit phase.
+ * During these phases, the reset subtree being updated is partly in reset
+ * and partly not in reset (it depends on the actual position in
+ * resettable_child_foreach()s). We are not able to tell in which part is a
+ * leaving or arriving device. Thus we cannot set the reset count of the
+ * moving device to the proper value.
+ */
+assert(!enter_phase_in_progress && !exit_phase_in_progress);
+trace_resettable_change_parent(obj, oldp, oldp_count, newp, newp_count);
+
+/*
+ * At most one of the two 'for' loops will be executed below
+ * in order to cope with the difference between the two counts.

Re: [PATCH v7 03/11] hw/core: create Resettable QOM interface


On 1/15/20 1:36 PM, Damien Hedde wrote:

This commit defines an interface allowing multi-phase reset. This aims
to solve a problem of the actual single-phase reset (built in
DeviceClass and BusClass): reset behavior is dependent on the order
in which reset handlers are called. In particular doing external
side-effect (like setting an qemu_irq) is problematic because receiving
object may not be reset yet.

The Resettable interface divides the reset in 3 well defined phases.
To reset an object tree, all 1st phases are executed then all 2nd then
all 3rd. See the comments in include/hw/resettable.h for a more complete
description. The interface defines 3 phases to let the future
possibility of holding an object into reset for some time.

The qdev/qbus reset in DeviceClass and BusClass will be modified in
following commits to use this interface. A mechanism is provided
to allow executing a transitional reset handler in place of the 2nd
phase which is executed in children-then-parent order inside a tree.
This will allow to transition devices and buses smoothly while
keeping the exact current qdev/qbus reset behavior for now.

Documentation will be added in a following commit.

Signed-off-by: Damien Hedde 
Reviewed-by: Richard Henderson 
---

v7 update: un-nest struct ResettablePhases
---
  Makefile.objs   |   1 +
  include/hw/resettable.h | 211 +++
  hw/core/resettable.c| 238 
  hw/core/Makefile.objs   |   1 +
  hw/core/trace-events|  17 +++
  5 files changed, 468 insertions(+)
  create mode 100644 include/hw/resettable.h
  create mode 100644 hw/core/resettable.c

diff --git a/Makefile.objs b/Makefile.objs
index 7c1e50f9d6..9752d549b4 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -191,6 +191,7 @@ trace-events-subdirs += migration
  trace-events-subdirs += net
  trace-events-subdirs += ui
  endif
+trace-events-subdirs += hw/core
  trace-events-subdirs += hw/display
  trace-events-subdirs += qapi
  trace-events-subdirs += qom
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
new file mode 100644
index 00..58b3df4c22
--- /dev/null
+++ b/include/hw/resettable.h
@@ -0,0 +1,211 @@
+/*
+ * Resettable interface header.
+ *
+ * Copyright (c) 2019 GreenSocs SAS
+ *
+ * Authors:
+ *   Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_RESETTABLE_H
+#define HW_RESETTABLE_H
+
+#include "qom/object.h"
+
+#define TYPE_RESETTABLE_INTERFACE "resettable"
+
+#define RESETTABLE_CLASS(class) \
+OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
+
+#define RESETTABLE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
+
+typedef struct ResettableState ResettableState;
+
+/**
+ * ResetType:
+ * Types of reset.
+ *
+ * + Cold: reset resulting from a power cycle of the object.
+ *
+ * TODO: Support has to be added to handle more types. In particular,
+ * ResettableState structure needs to be expanded.
+ */
+typedef enum ResetType {
+RESET_TYPE_COLD,
+} ResetType;
+
+/*
+ * ResettableClass:
+ * Interface for resettable objects.
+ *
+ * See docs/devel/reset.rst for more detailed information about how QEMU models
+ * reset. This whole API must only be used when holding the iothread mutex.
+ *
+ * All objects which can be reset must implement this interface;
+ * it is usually provided by a base class such as DeviceClass or BusClass.
+ * Every Resettable object must maintain some state tracking the
+ * progress of a reset operation by providing a ResettableState structure.
+ * The functions defined in this module take care of updating the
+ * state of the reset.
+ * The base class implementation of the interface provides this
+ * state and implements the associated method: get_state.
+ *
+ * Concrete object implementations (typically specific devices
+ * such as a UART model) should provide the functions
+ * for the phases.enter, phases.hold and phases.exit methods, which
+ * they can set in their class init function, either directly or
+ * by calling resettable_class_set_parent_phases().
+ * The phase methods are guaranteed to only only ever be called once
+ * for any reset event, in the order 'enter', 'hold', 'exit'.
+ * An object will always move quickly from 'enter' to 'hold'
+ * but might remain in 'hold' for an arbitrary period of time
+ * before eventually reset is deasserted and the 'exit' phase is called.
+ * Object implementations should be prepared for functions handling
+ * inbound connections from other devices (such as qemu_irq handler
+ * functions) to be called at any point during reset after their
+ * 'enter' method has been called.
+ *
+ * Users of a resettable object should not call these methods
+ * directly, but instead use the function resettable_reset().
+ *
+ * @phases.enter: This phase is called when the object enters reset.

Re: [PATCH v7 03/11] hw/core: create Resettable QOM interface


On 1/15/20 1:36 PM, Damien Hedde wrote:

This commit defines an interface allowing multi-phase reset. This aims
to solve a problem of the actual single-phase reset (built in
DeviceClass and BusClass): reset behavior is dependent on the order
in which reset handlers are called. In particular doing external
side-effect (like setting an qemu_irq) is problematic because receiving
object may not be reset yet.

The Resettable interface divides the reset in 3 well defined phases.
To reset an object tree, all 1st phases are executed then all 2nd then
all 3rd. See the comments in include/hw/resettable.h for a more complete
description. The interface defines 3 phases to let the future
possibility of holding an object into reset for some time.

The qdev/qbus reset in DeviceClass and BusClass will be modified in
following commits to use this interface. A mechanism is provided
to allow executing a transitional reset handler in place of the 2nd
phase which is executed in children-then-parent order inside a tree.
This will allow to transition devices and buses smoothly while
keeping the exact current qdev/qbus reset behavior for now.

Documentation will be added in a following commit.

Signed-off-by: Damien Hedde 
Reviewed-by: Richard Henderson 
---

v7 update: un-nest struct ResettablePhases
---
  Makefile.objs   |   1 +
  include/hw/resettable.h | 211 +++
  hw/core/resettable.c| 238 
  hw/core/Makefile.objs   |   1 +
  hw/core/trace-events|  17 +++
  5 files changed, 468 insertions(+)
  create mode 100644 include/hw/resettable.h
  create mode 100644 hw/core/resettable.c

diff --git a/Makefile.objs b/Makefile.objs
index 7c1e50f9d6..9752d549b4 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -191,6 +191,7 @@ trace-events-subdirs += migration
  trace-events-subdirs += net
  trace-events-subdirs += ui
  endif
+trace-events-subdirs += hw/core
  trace-events-subdirs += hw/display
  trace-events-subdirs += qapi
  trace-events-subdirs += qom
diff --git a/include/hw/resettable.h b/include/hw/resettable.h
new file mode 100644
index 00..58b3df4c22
--- /dev/null
+++ b/include/hw/resettable.h
@@ -0,0 +1,211 @@
+/*
+ * Resettable interface header.
+ *
+ * Copyright (c) 2019 GreenSocs SAS
+ *
+ * Authors:
+ *   Damien Hedde
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef HW_RESETTABLE_H
+#define HW_RESETTABLE_H
+
+#include "qom/object.h"
+
+#define TYPE_RESETTABLE_INTERFACE "resettable"
+
+#define RESETTABLE_CLASS(class) \
+OBJECT_CLASS_CHECK(ResettableClass, (class), TYPE_RESETTABLE_INTERFACE)
+
+#define RESETTABLE_GET_CLASS(obj) \
+OBJECT_GET_CLASS(ResettableClass, (obj), TYPE_RESETTABLE_INTERFACE)
+
+typedef struct ResettableState ResettableState;
+
+/**
+ * ResetType:
+ * Types of reset.
+ *
+ * + Cold: reset resulting from a power cycle of the object.
+ *
+ * TODO: Support has to be added to handle more types. In particular,
+ * ResettableState structure needs to be expanded.
+ */
+typedef enum ResetType {
+RESET_TYPE_COLD,
+} ResetType;
+
+/*
+ * ResettableClass:
+ * Interface for resettable objects.
+ *
+ * See docs/devel/reset.rst for more detailed information about how QEMU models
+ * reset. This whole API must only be used when holding the iothread mutex.
+ *
+ * All objects which can be reset must implement this interface;
+ * it is usually provided by a base class such as DeviceClass or BusClass.
+ * Every Resettable object must maintain some state tracking the
+ * progress of a reset operation by providing a ResettableState structure.
+ * The functions defined in this module take care of updating the
+ * state of the reset.
+ * The base class implementation of the interface provides this
+ * state and implements the associated method: get_state.
+ *
+ * Concrete object implementations (typically specific devices
+ * such as a UART model) should provide the functions
+ * for the phases.enter, phases.hold and phases.exit methods, which
+ * they can set in their class init function, either directly or
+ * by calling resettable_class_set_parent_phases().
+ * The phase methods are guaranteed to only only ever be called once
+ * for any reset event, in the order 'enter', 'hold', 'exit'.
+ * An object will always move quickly from 'enter' to 'hold'
+ * but might remain in 'hold' for an arbitrary period of time
+ * before eventually reset is deasserted and the 'exit' phase is called.
+ * Object implementations should be prepared for functions handling
+ * inbound connections from other devices (such as qemu_irq handler
+ * functions) to be called at any point during reset after their
+ * 'enter' method has been called.
+ *
+ * Users of a resettable object should not call these methods
+ * directly, but instead use the function resettable_reset().
+ *
+ * @phases.enter: This phase is called when the object enters reset.

Re: [PATCH] audio/dsound: fix invalid parameters error


On 1/17/20 7:26 PM, KJ Liew wrote:

QEMU Windows has broken dsound backend since the rewrite of audio API in
version 4.2.0. Both playback and capture buffers failed to lock with
invalid parameters error.


Fixes: 7fa9754ac88 (dsoundaudio: port to the new audio backend api)

Cc'ing Zoltán who wrote 7fa9754ac88, and Gerd (the maintainer of this file):

  $ ./scripts/get_maintainer.pl -f audio/dsoundaudio.c
  Gerd Hoffmann  (maintainer:Audio)


--- ../orig/qemu-4.2.0/audio/dsoundaudio.c  2019-12-12 10:20:47.0 
-0800
+++ ../qemu-4.2.0/audio/dsoundaudio.c   2020-01-17 08:05:46.783966900 -0800
@@ -53,6 +53,7 @@
  typedef struct {
  HWVoiceOut hw;
  LPDIRECTSOUNDBUFFER dsound_buffer;
+void *last_buf;
  dsound *s;
  } DSoundVoiceOut;
  
@@ -414,10 +415,10 @@

  DSoundVoiceOut *ds = (DSoundVoiceOut *) hw;
  LPDIRECTSOUNDBUFFER dsb = ds->dsound_buffer;
  HRESULT hr;
-DWORD ppos, act_size;
+DWORD ppos, act_size, last_size;
  size_t req_size;
  int err;
-void *ret;
+void *ret, *last_ret;
  
  hr = IDirectSoundBuffer_GetCurrentPosition(dsb, , NULL);

  if (FAILED(hr)) {
@@ -426,17 +427,24 @@
  return NULL;
  }
  
+if (ppos == hw->pos_emul) {

+*size = 0;
+return ds->last_buf;
+}
+
  req_size = audio_ring_dist(ppos, hw->pos_emul, hw->size_emul);
  req_size = MIN(req_size, hw->size_emul - hw->pos_emul);
  
-err = dsound_lock_out(dsb, >info, hw->pos_emul, req_size, , NULL,

-  _size, NULL, false, ds->s);
+err = dsound_lock_out(dsb, >info, hw->pos_emul, req_size, , 
_ret,
+  _size, _size, false, ds->s);
  if (err) {
  dolog("Failed to lock buffer\n");
  *size = 0;
  return NULL;
  }
  
+ds->last_buf = g_realloc(ds->last_buf, act_size);

+memcpy(ds->last_buf, ret, act_size);
  *size = act_size;
  return ret;
  }
@@ -445,6 +453,8 @@
  {
  DSoundVoiceOut *ds = (DSoundVoiceOut *) hw;
  LPDIRECTSOUNDBUFFER dsb = ds->dsound_buffer;
+if (len == 0)
+return 0;
  int err = dsound_unlock_out(dsb, buf, NULL, len, 0);
  
  if (err) {

@@ -508,10 +518,10 @@
  DSoundVoiceIn *ds = (DSoundVoiceIn *) hw;
  LPDIRECTSOUNDCAPTUREBUFFER dscb = ds->dsound_capture_buffer;
  HRESULT hr;
-DWORD cpos, act_size;
+DWORD cpos, act_size, last_size;
  size_t req_size;
  int err;
-void *ret;
+void *ret, *last_ret;
  
  hr = IDirectSoundCaptureBuffer_GetCurrentPosition(dscb, , NULL);

  if (FAILED(hr)) {
@@ -520,11 +530,16 @@
  return NULL;
  }
  
+if (cpos == hw->pos_emul) {

+*size = 0;
+return NULL;
+}
+
  req_size = audio_ring_dist(cpos, hw->pos_emul, hw->size_emul);
  req_size = MIN(req_size, hw->size_emul - hw->pos_emul);
  
-err = dsound_lock_in(dscb, >info, hw->pos_emul, req_size, , NULL,

- _size, NULL, false, ds->s);
+err = dsound_lock_in(dscb, >info, hw->pos_emul, req_size, , 
_ret,
+ _size, _size, false, ds->s);
  if (err) {
  dolog("Failed to lock buffer\n");
  *size = 0;

Re: [PATCH v1] vnc: fix VNC artifacts


On 1/17/20 10:43 PM, Cameron Esfahani via wrote:

I’m new to this process, what are the next steps?


Assuming your patch is in a branch named vncfix_v1:

1/ Start new branch based on the commit previous to your patch:

  - git checkout -b vncfix_v2 vncfix_v1~

2/ Revert the offending patch, explain in commit description why:

  - git revert de3f7de7f4e257

3/ Apply the rest of your patch on top, git-cherry-pick is smart to 
directly use the diff context. Verify the patch is correct and rewrite 
the commit description:


  - git cherry-pick vncfix_v1

4/ Send the 2 patches as a series to the mailing list



Cameron Esfahani
di...@apple.com


On Jan 16, 2020, at 11:47 PM, Gerd Hoffmann  wrote:

On Thu, Jan 16, 2020 at 07:50:58PM -0800, Cameron Esfahani wrote:

Remove VNC optimization to reencode framebuffer update as raw if it's
smaller than the default encoding.  QEMU's implementation was naive and
didn't account for the ZLIB z_stream mutating with each compression.  Just
saving and restoring the output buffer offset wasn't sufficient to "rewind"
the previous encoding.  Considering that ZRLE is never larger than raw and
even though ZLIB can occasionally be fractionally larger than raw, the
overhead of implementing this optimization correctly isn't worth it.


So just revert de3f7de7f4e257 then ...


In my investigation, ZRLE always compresses better than ZLIB so
prioritize ZRLE over ZLIB, even if the client hints that ZLIB is
preferred.


... and make this a separate patch?

cheers,
  Gerd

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4


On 1/17/20 8:23 PM, Richard Henderson wrote:

On 1/17/20 8:50 AM, Philippe Mathieu-Daudé wrote:

On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
 wrote:


The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
or the operation is undefined.  However, real hardware only generates
an unaligned access trap for unaligned mod 4.


This Linux kernel commit seems relevant:

https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164

   /* From: "Jim Hull" 
   I've attached a summary of the change, but basically, for PA 2.0, as
   long as the ",CO" (coherent operation) completer is specified, then the
   16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
   they only require "natural" alignment (4-byte for ldcw, 8-byte for
   ldcd). */


It isn't completely relevant.  We don't implement hppa 2.0.


Oh... It was late and I misread it as:

  if (PA2)
...
  else
... "instead of PA 2.0 ... the 16-byte alignment ... is
relaxed, and insted only require "natural" alignment ..."

  endif



I added a TODO comment for HPPA64, as I said in reply to Dave Anglin elsewhere
in this thread.

Re: [PATCH v3 0/7] gpio: Add GPIO Aggregator/Repeater

2020-01-17 Thread Eugeniu Rosca

Hi Geert,

On Wed, Nov 27, 2019 at 09:42:46AM +0100, Geert Uytterhoeven wrote:
>   - Create aggregators:
> 
> $ echo e6052000.gpio 19,20 \
> > /sys/bus/platform/drivers/gpio-aggregator/new_device
> 
> gpio-aggregator gpio-aggregator.0: gpio 0 => gpio-953 (gpio-aggregator.0)
> gpio-aggregator gpio-aggregator.0: gpio 1 => gpio-954 (gpio-aggregator.0)
> gpiochip_find_base: found new base at 778
> gpio gpiochip8: (gpio-aggregator.0): added GPIO chardev (254:8)
> gpiochip_setup_dev: registered GPIOs 778 to 779 on device: gpiochip8 
> (gpio-aggregator.0)
> 
> $ echo e6052000.gpio 21 e605.gpio 20-22 \
> > /sys/bus/platform/drivers/gpio-aggregator/new_device
> 
> gpio-aggregator gpio-aggregator.1: gpio 0 => gpio-955 (gpio-aggregator.1)
> gpio-aggregator gpio-aggregator.1: gpio 1 => gpio-1012 (gpio-aggregator.1)
> gpio-aggregator gpio-aggregator.1: gpio 2 => gpio-1013 (gpio-aggregator.1)
> gpio-aggregator gpio-aggregator.1: gpio 3 => gpio-1014 (gpio-aggregator.1)
> gpiochip_find_base: found new base at 774
> gpio gpiochip9: (gpio-aggregator.1): added GPIO chardev (254:9)
> gpiochip_setup_dev: registered GPIOs 774 to 777 on device: gpiochip9 
> (gpio-aggregator.1)
> 
>   - Adjust permissions on /dev/gpiochip[89] (optional)
> 
>   - Control LEDs:
> 
> $ gpioset gpiochip8 0=0 1=1 # LED6 OFF, LED7 ON
> $ gpioset gpiochip8 0=1 1=0 # LED6 ON, LED7 OFF
> $ gpioset gpiochip9 0=0 # LED8 OFF
> $ gpioset gpiochip9 0=1 # LED8 ON
> 
>   - Destroy aggregators:
> 
> $ echo gpio-aggregator.0 \
> > /sys/bus/platform/drivers/gpio-aggregator/delete_device
> $ echo gpio-aggregator.1 \
> > /sys/bus/platform/drivers/gpio-aggregator/delete_device

Thanks for describing the test procedure in detail. It helps a lot.

Using similar commands on H3ULCB, I could successfully trigger the
gpiochip6-{12,13} leds on and off. 

The only unexpected thing is seeing below messages (where gpiochip99 and
gpiochip22 are inexisting gpiochip names, mistakenly provided on command
line prior to passing the correct name):

root@rcar-gen3:~# echo gpiochip6 12-13 > 
/sys/bus/platform/drivers/gpio-aggregator/new_device

[  915.572905] gpio-aggregator gpio-aggregator.0: cannot find GPIO chip 
gpiochip99, deferring
[  915.584224] gpio-aggregator gpio-aggregator.2: cannot find GPIO chip 
gpiochip99, deferring
[  915.865281] gpio-aggregator gpio-aggregator.29: cannot find GPIO chip 
gpiochip22, deferring

Obviously, in the above case, due to a typo in the names, the gpio
chips will never be found, no matter how long gpio-aggregator defers
their probing. Unfortunately, the driver will continuously emit those
messages, upon each successfully created/aggregated gpiochip. I built
gpio-aggregator as a loadable module, if that's relevant.

Another comment is that, while the series _does_ allow specifying
gpio lines in the DTS (this would require a common compatible string
in gpio_aggregator_dt_ids[] and in the DTS node) and while those lines
are indeed exposed to userspace, based on my testing, these same gpio
lines are marked as "used/reserved" by the kernel. This means that
operating on those gpio pins from userspace will not be possible.
For instance, gpioget/gpioset return "Device or resource busy":

gpioget: error reading GPIO values: Device or resource busy
gpioset: error setting the GPIO line values: Device or resource busy

I guess Harish will be unhappy about that, as his expectation was that
upon merging gpio-aggregator with gpio-inverter, he will be able to
describe GPIO polarity and names in DTS without "hogging" the pins.
Perhaps this can be supplemented via an add-on patch later on?

For the whole series (leaving the above findings to your discretion):

Reviewed-by: Eugeniu Rosca 
Tested-by: Eugeniu Rosca 

Thanks!

-- 
Best Regards,
Eugeniu

[Bug 1749393] Re: sbrk() not working under qemu-user with a PIE-compiled binary?

Another proposed patch:
https://patchew.org/QEMU/20200117230245.5040-1-richard.hender...@linaro.org/

** Changed in: qemu (Ubuntu)
 Assignee: (unassigned) => Richard Henderson (rth)

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

Title:
  sbrk() not working under qemu-user with a PIE-compiled binary?

Status in QEMU:
  New
Status in qemu package in Ubuntu:
  Confirmed

Bug description:
  In Debian unstable, we recently switched bash to be a PIE-compiled
  binary (for hardening). Unfortunately this resulted in bash being
  broken when run under qemu-user (for all target architectures, host
  being amd64 for me).

  $ sudo chroot /srv/chroots/sid-i386/ qemu-i386-static /bin/bash
  bash: xmalloc: .././shell.c:1709: cannot allocate 10 bytes (0 bytes allocated)

  bash has its own malloc implementation based on sbrk():
  https://git.savannah.gnu.org/cgit/bash.git/tree/lib/malloc/malloc.c

  When we disable this internal implementation and rely on glibc's
  malloc, then everything is fine. But it might be that glibc has a
  fallback when sbrk() is not working properly and it might hide the
  underlying problem in qemu-user.

  This issue has also been reported to the bash upstream author and he 
suggested that the issue might be in qemu-user so I'm opening a ticket here. 
Here's the discussion with the bash upstream author:
  https://lists.gnu.org/archive/html/bug-bash/2018-02/threads.html#00080

  You can find the problematic bash binary in that .deb file:
  
http://snapshot.debian.org/archive/debian/20180206T154716Z/pool/main/b/bash/bash_4.4.18-1_i386.deb

  The version of qemu I have been using is 2.11 (Debian package qemu-
  user-static version 1:2.11+dfsg-1) but I have had reports that the
  problem is reproducible with older versions (back to 2.8 at least).

  Here are the related Debian bug reports:
  https://bugs.debian.org/889869
  https://bugs.debian.org/865599

  It's worth noting that bash used to have this problem (when compiled as a PIE 
binary) even when run directly but then something got fixed in the kernel and 
now the problem only appears when run under qemu-user:
  https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1518483

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

Re: [PATCH] linux-user: Reserve space for brk

2020-01-17 Thread Warner Losh

On Fri, Jan 17, 2020 at 4:03 PM Richard Henderson <
richard.hender...@linaro.org> wrote:

> With bad luck, we can wind up with no space at all for brk,
> which will generally cause the guest malloc to fail.
>
...

> The choice of 16MB is somewhat arbitrary.  It's enough for libc
> to get going, but without being so large that 32-bit guests or
> 32-bit hosts are in danger of running out of virtual address space.
> It is expected that libc will be able to fall back to mmap arenas
> after the limited brk space is exhausted.
>

The biggest abuser of brk() is emacs, so according to one old wag
this should be 80MB. Eighty Megabytes And Continuously Swapping.

Sorry that I don't have anything constructive to add.

Warner

[PATCH] linux-user: Reserve space for brk

With bad luck, we can wind up with no space at all for brk,
which will generally cause the guest malloc to fail.

This bad luck is easier to come by with ET_DYN (PIE) binaries,
where either the stack or the interpreter (ld.so) gets placed
immediately after the main executable.

But there's nothing preventing this same thing from happening
with ET_EXEC (normal) binaries, during probe_guest_base().

In both cases, reserve some extra space via mmap and release
it back to the system after loading the interpreter and
allocating the stack.

The choice of 16MB is somewhat arbitrary.  It's enough for libc
to get going, but without being so large that 32-bit guests or
32-bit hosts are in danger of running out of virtual address space.
It is expected that libc will be able to fall back to mmap arenas
after the limited brk space is exhausted.

Launchpad: https://bugs.launchpad.net/qemu/+bug/1749393
Signed-off-by: Richard Henderson 
---

Note that the LP comments mention the fix for this in the kernel,
and about there being a "guaranteed 128MB gap" for x86_64.  As far
as I can see, this "gap" is part of the unmapped_area() algorithm.
For qemu, this would correspond to mmap_find_vma(), except that,
except when we fall back to mmap_find_vma_reserved(), we are not
100% in control over the allocation.


r~

---
 linux-user/qemu.h|  1 +
 linux-user/elfload.c | 73 +---
 2 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index f6f5fe5fbb..560a68090e 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -35,6 +35,7 @@ struct image_info {
 abi_ulong   end_data;
 abi_ulong   start_brk;
 abi_ulong   brk;
+abi_ulong   reserve_brk;
 abi_ulong   start_mmap;
 abi_ulong   start_stack;
 abi_ulong   stack_limit;
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 07b16cc0f4..2edb5d5b31 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -10,6 +10,7 @@
 #include "qemu/path.h"
 #include "qemu/queue.h"
 #include "qemu/guest-random.h"
+#include "qemu/units.h"
 
 #ifdef _ARCH_PPC64
 #undef ARCH_DLINFO
@@ -2364,24 +2365,51 @@ static void load_elf_image(const char *image_name, int 
image_fd,
 }
 }
 
-load_addr = loaddr;
-if (ehdr->e_type == ET_DYN) {
-/* The image indicates that it can be loaded anywhere.  Find a
-   location that can hold the memory space required.  If the
-   image is pre-linked, LOADDR will be non-zero.  Since we do
-   not supply MAP_FIXED here we'll use that address if and
-   only if it remains available.  */
-load_addr = target_mmap(loaddr, hiaddr - loaddr, PROT_NONE,
-MAP_PRIVATE | MAP_ANON | MAP_NORESERVE,
--1, 0);
-if (load_addr == -1) {
-goto exit_perror;
+if (pinterp_name != NULL) {
+/*
+ * This is the main executable.
+ *
+ * Reserve extra space for brk.
+ * We hold on to this space while placing the interpreter
+ * and the stack, lest they be placed immediately after
+ * the data segment and block allocation from the brk.
+ *
+ * 16MB is chosen as "large enough" without being so large
+ * as to allow the result to not fit with a 32-bit guest on
+ * a 32-bit host.
+ */
+info->reserve_brk = 16 * MiB;
+hiaddr += info->reserve_brk;
+
+if (ehdr->e_type == ET_EXEC) {
+/*
+ * Make sure that the low address does not conflict with
+ * MMAP_MIN_ADDR or the QEMU application itself.
+ */
+probe_guest_base(image_name, loaddr, hiaddr);
 }
-} else if (pinterp_name != NULL) {
-/* This is the main executable.  Make sure that the low
-   address does not conflict with MMAP_MIN_ADDR or the
-   QEMU application itself.  */
-probe_guest_base(image_name, loaddr, hiaddr);
+}
+
+/*
+ * Reserve address space for all of this.
+ *
+ * In the case of ET_EXEC, we supply MAP_FIXED so that we get
+ * exactly the address range that is required.
+ *
+ * Otherwise this is ET_DYN, and we are searching for a location
+ * that can hold the memory space required.  If the image is
+ * pre-linked, LOADDR will be non-zero, and the kernel should
+ * honor that address if it happens to be free.
+ *
+ * In both cases, we will overwrite pages in this range with mappings
+ * from the executable.
+ */
+load_addr = target_mmap(loaddr, hiaddr - loaddr, PROT_NONE,
+MAP_PRIVATE | MAP_ANON | MAP_NORESERVE |
+(ehdr->e_type == ET_EXEC ? MAP_FIXED : 0),
+-1, 0);
+if (load_addr == -1) {
+goto exit_perror;
 }
 load_bias =

Re: [PATCH 036/104] virtiofsd: passthrough_ll: add fd_map to hide file descriptors

On Thu, Dec 12, 2019 at 04:37:56PM +, Dr. David Alan Gilbert (git) wrote:
> From: Stefan Hajnoczi 
> 
> Do not expose file descriptor numbers to clients.  This prevents the
> abuse of internal file descriptors (like stdin/stdout).
> 
> Signed-off-by: Stefan Hajnoczi 
> dgilbert:
>   Added lseek
> ---
>  tools/virtiofsd/passthrough_ll.c | 114 +--
>  1 file changed, 93 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index face8910b0..93e74cce21 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -60,6 +60,7 @@ struct lo_map_elem {
>  union {
>  struct lo_inode *inode;
>  struct lo_dirp *dirp;
> +int fd;
>  ssize_t freelist;
>  };
>  bool in_use;
> @@ -107,6 +108,7 @@ struct lo_data {
>  struct lo_inode root; /* protected by lo->mutex */
>  struct lo_map ino_map; /* protected by lo->mutex */
>  struct lo_map dirp_map; /* protected by lo->mutex */
> +struct lo_map fd_map; /* protected by lo->mutex */
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -236,6 +238,20 @@ static void lo_map_remove(struct lo_map *map, size_t key)
>  map->freelist = key;
>  }
>  
> +/* Assumes lo->mutex is held */
> +static ssize_t lo_add_fd_mapping(fuse_req_t req, int fd)
> +{
> +struct lo_map_elem *elem;
> +
> +elem = lo_map_alloc_elem(_data(req)->fd_map);
> +if (!elem) {
> +return -1;
> +}
> +
> +elem->fd = fd;
> +return elem - lo_data(req)->fd_map.elems;
> +}
> +
>  /* Assumes lo->mutex is held */
>  static ssize_t lo_add_dirp_mapping(fuse_req_t req, struct lo_dirp *dirp)
>  {
> @@ -350,6 +366,22 @@ static int utimensat_empty_nofollow(struct lo_inode 
> *inode,
>  return utimensat(AT_FDCWD, procname, tv, 0);
>  }
>  
> +static int lo_fi_fd(fuse_req_t req, struct fuse_file_info *fi)
> +{
> +struct lo_data *lo = lo_data(req);
> +struct lo_map_elem *elem;
> +
> +pthread_mutex_lock(>mutex);
> +elem = lo_map_get(>fd_map, fi->fh);
> +pthread_mutex_unlock(>mutex);
> +
> +if (!elem) {
> +return -1;
> +}
> +
> +return elem->fd;
> +}
> +
>  static void lo_setattr(fuse_req_t req, fuse_ino_t ino, struct stat *attr,
> int valid, struct fuse_file_info *fi)
>  {
> @@ -358,6 +390,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>  struct lo_inode *inode;
>  int ifd;
>  int res;
> +int fd;
>  
>  inode = lo_inode(req, ino);
>  if (!inode) {
> @@ -367,9 +400,14 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>  
>  ifd = inode->fd;
>  
> +/* If fi->fh is invalid we'll report EBADF later */
> +if (fi) {
> +fd = lo_fi_fd(req, fi);
> +}
> +
>  if (valid & FUSE_SET_ATTR_MODE) {
>  if (fi) {
> -res = fchmod(fi->fh, attr->st_mode);
> +res = fchmod(fd, attr->st_mode);
>  } else {
>  sprintf(procname, "/proc/self/fd/%i", ifd);
>  res = chmod(procname, attr->st_mode);
> @@ -389,7 +427,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>  }
>  if (valid & FUSE_SET_ATTR_SIZE) {
>  if (fi) {
> -res = ftruncate(fi->fh, attr->st_size);
> +res = ftruncate(fd, attr->st_size);
>  } else {
>  sprintf(procname, "/proc/self/fd/%i", ifd);
>  res = truncate(procname, attr->st_size);
> @@ -419,7 +457,7 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>  }
>  
>  if (fi) {
> -res = futimens(fi->fh, tv);
> +res = futimens(fd, tv);
>  } else {
>  res = utimensat_empty_nofollow(inode, tv);
>  }
> @@ -1079,7 +1117,18 @@ static void lo_create(fuse_req_t req, fuse_ino_t 
> parent, const char *name,
>  lo_restore_cred();
>  
>  if (!err) {
> -fi->fh = fd;
> +ssize_t fh;
> +
> +pthread_mutex_lock(>mutex);
> +fh = lo_add_fd_mapping(req, fd);
> +pthread_mutex_unlock(>mutex);
> +if (fh == -1) {
> +close(fd);
> +fuse_reply_err(req, ENOMEM);
> +return;
> +}
> +
> +fi->fh = fh;
>  err = lo_do_lookup(req, parent, name, );
>  }
>  if (lo->cache == CACHE_NEVER) {
> @@ -1123,6 +1172,7 @@ static void lo_fsyncdir(fuse_req_t req, fuse_ino_t ino, 
> int datasync,
>  static void lo_open(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info 
> *fi)
>  {
>  int fd;
> +ssize_t fh;
>  char buf[64];
>  struct lo_data *lo = lo_data(req);
>  
> @@ -1158,7 +1208,16 @@ static void lo_open(fuse_req_t req, fuse_ino_t ino, 
> struct fuse_file_info *fi)
>  return (void)fuse_reply_err(req, errno);
>  }
>  
> -fi->fh = fd;
> +pthread_mutex_lock(>mutex);
> +

[Bug 1860053] Re: Possible lack of precision when calling clock_gettime via vDSO on user mode ppc64le

2020-01-17 Thread Fabiano Rosas

Since there is no vdso, the Go code goes into the syscall fallback:

MOVDruntime·vdsoClockgettimeSym(SB), R12// Check for VDSO availability
CMP R12, R0
BEQ fallback
(...)
fallback:
ADD $32, R1, R4
SYSCALL $SYS_clock_gettime
MOVD32(R1), R3
MOVD48(R1), R5
JMP finish

But upon inspection, it seems the offset while loading R5 is not
correct:

in QEMU's clock_gettime implementation: 
(gdb) p/x *host_ts
$8 = {tv_sec = 0x9225f, tv_nsec = 0x375f74ee}

in the Go runtime:
(gdb) p/x *($r1 + 48)
$6 = 0x388c8
(gdb) p/x *($r1 + 40)
$7 = 0x375f74ee

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

Title:
  Possible lack of precision when calling clock_gettime via vDSO on user
  mode ppc64le

Status in QEMU:
  New

Bug description:
  Occurs on QEMU v4.2.0 run on docker (via the qemu-user-static:v4.2.0-2
  image) on an AMD64 Ubuntu 18.04.3 LTS machine provided by travis-
  ci.org.

  From golang's https://github.com/golang/go/issues/36592:

  It was discovered that golang's time.NewTicker() and time.Sleep()
  malfunction when a compiled application was run via QEMU's ppc64le
  emulator in user mode.

  The methods did not malfunction on actual PowerPC hardware or when the
  same golang application was compiled for golang's arm, arm64 or 386
  targets and was run via user mode QEMU on the same system.

  Curiously, the methods also worked when the program was compiled under
  go 1.11, but do malfunction in go 1.12 and 1.13.

  It was identified the change in behaviour was most likely attributable to 
golang switching to using vSDO for calling clock_gettime() on PowerPC 64 
architectures in 1.12. I.E:
  https://github.com/golang/go/commit/dbd8af74723d2c98cbdcc70f7e2801f69b57ac5b

  We therefore suspect there may be a bug in QEMU's user-mode emulation
  of ppc64le as relates to vDSO calls to clock_gettime().

  The nature of the malfunction of time.NewTicker() and time.Sleep() is
  such that sleeps or ticks with a granularity of less than one second
  do not appear to be possible (they all revert to 1 second
  sleeps/ticks). Could it be that the nanoseconds field of
  clock_gettime() is getting lost in the vDSO version but not in the
  syscall? Or some other issue calling these methods via vDSO?

  Thanks in advance.

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

Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl

2020-01-17 Thread Alexandre Belloni

Hi,

Quick answers to your very good questions below:

On 17/01/2020 21:50:34+0100, Aleksandar Markovic wrote:
> Alexandre (and Arnd too, or any other person knowledgeable in the area),
> 
> I just need to clarify a couple of details with you, please.
> 
> Firstly, here is what man page rtc(4) says:
> 
> "The /dev/rtc (or /dev/rtc0, /dev/rtc1, etc.) device can be opened
> only once (until it is closed) and it is read-only. On read(2) and
> select(2) the calling process is blocked until the next interrupt from
> that RTC is received. Following the interrupt, the process can read a
> long integer, of which the least significant byte contains a bit mask
> encoding the types of interrupt that occurred, while the remaining 3
> bytes contain the number of interrupts since the last read(2)."
> 
> So, it looks read() will always return only 4 bytes of useful info
> (regardless of host being 32-bit/64-bit).
> 
> My questions are:
> 
> - Is the description in man page genuinely accurate?
> 

It is accurate. It is a mask of:

#define RTC_IRQF 0x80   /* Any of the following is active */
#define RTC_PF 0x40 /* Periodic interrupt */
#define RTC_AF 0x20 /* Alarm interrupt */
#define RTC_UF 0x10 /* Update interrupt for 1Hz RTC */

Which will most likely be RTC_IRQF | RTC_AF.

> - To me (but I am really an outsider to using RTC in applications),
> this feature (blocking read()/select()) even looks very nice and
> convenient, in all fairness. But I would like to ask you: Is this
> feature used rarely or frequently by other libraries/tools/etc.? In
> other words, is the feature "obscure" or "crucial" part of RTC kernel
> support? Or, something in between?
> 

Nobody is actually using the return value.

> - Does MC146818 support this feature?
> 

This feature is implemented in the RTC core so it is supported by all
RTCs that have alarms.

Internally, the value is an unsigned long and it is casted properly by
put_user in rtc_dev_read:

https://elixir.bootlin.com/linux/v5.4/source/drivers/rtc/dev.c#L178

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 034/104] virtiofsd: passthrough_ll: add ino_map to hide lo_inode pointers

On Thu, Dec 12, 2019 at 04:37:54PM +, Dr. David Alan Gilbert (git) wrote:
> From: Stefan Hajnoczi 
> 
> Do not expose lo_inode pointers to clients.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tools/virtiofsd/passthrough_ll.c | 144 ---
>  1 file changed, 114 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 0a94c3e1f2..fd1d88bddf 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -57,8 +57,8 @@
>  
>  #define HAVE_POSIX_FALLOCATE 1
>  /*
> - * We are re-using pointers to our `struct lo_inode` and `struct
> - * lo_dirp` elements as inodes. This means that we must be able to
> + * We are re-using pointers to our `struct lo_inode`
> + * elements as inodes. This means that we must be able to
>   * store uintptr_t values in a fuse_ino_t variable. The following
>   * incantation checks this condition at compile time.
>   */
> @@ -76,7 +76,7 @@ struct _uintptr_to_must_hold_fuse_ino_t_dummy_struct {
>  
>  struct lo_map_elem {
>  union {
> -/* Element values will go here... */
> +struct lo_inode *inode;
>  ssize_t freelist;
>  };
>  bool in_use;
> @@ -97,6 +97,7 @@ struct lo_inode {
>  ino_t ino;
>  dev_t dev;
>  uint64_t refcount; /* protected by lo->mutex */
> +fuse_ino_t fuse_ino;
>  };
>  
>  struct lo_cred {
> @@ -121,6 +122,7 @@ struct lo_data {
>  int cache;
>  int timeout_set;
>  struct lo_inode root; /* protected by lo->mutex */
> +struct lo_map ino_map; /* protected by lo->mutex */
>  };
>  
>  static const struct fuse_opt lo_opts[] = {
> @@ -145,14 +147,14 @@ static struct lo_data *lo_data(fuse_req_t req)
>  return (struct lo_data *)fuse_req_userdata(req);
>  }
>  
> -__attribute__((unused)) static void lo_map_init(struct lo_map *map)
> +static void lo_map_init(struct lo_map *map)
>  {
>  map->elems = NULL;
>  map->nelems = 0;
>  map->freelist = -1;
>  }
>  
> -__attribute__((unused)) static void lo_map_destroy(struct lo_map *map)
> +static void lo_map_destroy(struct lo_map *map)
>  {
>  free(map->elems);
>  }
> @@ -183,8 +185,7 @@ static int lo_map_grow(struct lo_map *map, size_t 
> new_nelems)
>  return 1;
>  }
>  
> -__attribute__((unused)) static struct lo_map_elem *
> -lo_map_alloc_elem(struct lo_map *map)
> +static struct lo_map_elem *lo_map_alloc_elem(struct lo_map *map)
>  {
>  struct lo_map_elem *elem;
>  
> @@ -200,8 +201,7 @@ lo_map_alloc_elem(struct lo_map *map)
>  return elem;
>  }
>  
> -__attribute__((unused)) static struct lo_map_elem *
> -lo_map_reserve(struct lo_map *map, size_t key)
> +static struct lo_map_elem *lo_map_reserve(struct lo_map *map, size_t key)
>  {
>  ssize_t *prev;
>  
> @@ -222,8 +222,7 @@ lo_map_reserve(struct lo_map *map, size_t key)
>  return NULL;
>  }
>  
> -__attribute__((unused)) static struct lo_map_elem *
> -lo_map_get(struct lo_map *map, size_t key)
> +static struct lo_map_elem *lo_map_get(struct lo_map *map, size_t key)
>  {
>  if (key >= map->nelems) {
>  return NULL;
> @@ -234,8 +233,7 @@ lo_map_get(struct lo_map *map, size_t key)
>  return >elems[key];
>  }
>  
> -__attribute__((unused)) static void lo_map_remove(struct lo_map *map,
> -  size_t key)
> +static void lo_map_remove(struct lo_map *map, size_t key)
>  {
>  struct lo_map_elem *elem;
>  
> @@ -254,18 +252,40 @@ __attribute__((unused)) static void 
> lo_map_remove(struct lo_map *map,
>  map->freelist = key;
>  }
>  
> +/* Assumes lo->mutex is held */
> +static ssize_t lo_add_inode_mapping(fuse_req_t req, struct lo_inode *inode)
> +{
> +struct lo_map_elem *elem;
> +
> +elem = lo_map_alloc_elem(_data(req)->ino_map);
> +if (!elem) {
> +return -1;
> +}
> +
> +elem->inode = inode;
> +return elem - lo_data(req)->ino_map.elems;
> +}
> +
>  static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino)
>  {
> -if (ino == FUSE_ROOT_ID) {
> -return _data(req)->root;
> -} else {
> -return (struct lo_inode *)(uintptr_t)ino;
> +struct lo_data *lo = lo_data(req);
> +struct lo_map_elem *elem;
> +
> +pthread_mutex_lock(>mutex);
> +elem = lo_map_get(>ino_map, ino);
> +pthread_mutex_unlock(>mutex);
> +
> +if (!elem) {
> +return NULL;
>  }
> +
> +return elem->inode;
>  }
>  
>  static int lo_fd(fuse_req_t req, fuse_ino_t ino)
>  {
> -return lo_inode(req, ino)->fd;
> +struct lo_inode *inode = lo_inode(req, ino);
> +return inode ? inode->fd : -1;
>  }
>  
>  static bool lo_debug(fuse_req_t req)
> @@ -337,10 +357,18 @@ static void lo_setattr(fuse_req_t req, fuse_ino_t ino, 
> struct stat *attr,
>  {
>  int saverr;
>  char procname[64];
> -struct lo_inode *inode = lo_inode(req, ino);
> -int ifd = inode->fd;
> +struct lo_inode *inode;
> +int ifd;
>  int

Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl

2020-01-17 Thread Arnd Bergmann

On Fri, Jan 17, 2020 at 9:50 PM Aleksandar Markovic
 wrote:

> Alexandre (and Arnd too, or any other person knowledgeable in the area),
>
> I just need to clarify a couple of details with you, please.
>
> Firstly, here is what man page rtc(4) says:
>
> "The /dev/rtc (or /dev/rtc0, /dev/rtc1, etc.) device can be opened
> only once (until it is closed) and it is read-only. On read(2) and
> select(2) the calling process is blocked until the next interrupt from
> that RTC is received. Following the interrupt, the process can read a
> long integer, of which the least significant byte contains a bit mask
> encoding the types of interrupt that occurred, while the remaining 3
> bytes contain the number of interrupts since the last read(2)."
>
> So, it looks read() will always return only 4 bytes of useful info
> (regardless of host being 32-bit/64-bit).

It says "long integer", which is 64-bit on a 64-bit machine.

> My questions are:
>
> - Is the description in man page genuinely accurate?

Starting with linux-2.6.18, there is another possibility: If an
application asks for exactly four bytes on a 64-bit kernel,
it gets the lower four bytes, as it would on a 32-bit kernel.

This is a hack that was introduced for running 32-bit compat
tasks.

For any other size less than sizeof(long), the kernel reports
an EINVAL error, and for anything larger or equal to sizeof(long)
it attempts to output a long word.

> - To me (but I am really an outsider to using RTC in applications),
> this feature (blocking read()/select()) even looks very nice and
> convenient, in all fairness. But I would like to ask you: Is this
> feature used rarely or frequently by other libraries/tools/etc.? In
> other words, is the feature "obscure" or "crucial" part of RTC kernel
> support? Or, something in between?

> - Does MC146818 support this feature?

No idea, I'll leave these for Alexandre or someone else to answer.

  Arnd

Re: [PATCH 033/104] virtiofsd: passthrough_ll: add lo_map for ino/fh indirection

On Thu, Dec 12, 2019 at 04:37:53PM +, Dr. David Alan Gilbert (git) wrote:
> From: Stefan Hajnoczi 
> 
> A layer of indirection is needed because passthrough_ll cannot expose
> pointers or file descriptor numbers to untrusted clients.  Malicious
> clients could send invalid pointers or file descriptors in order to
> crash or exploit the file system daemon.
> 
> lo_map provides an integer key->value mapping.  This will be used for
> ino and fh fields in the patches that follow.
> 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tools/virtiofsd/passthrough_ll.c | 124 +++
>  1 file changed, 124 insertions(+)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c 
> b/tools/virtiofsd/passthrough_ll.c
> index 0188cd9ad6..0a94c3e1f2 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -74,6 +74,21 @@ struct _uintptr_to_must_hold_fuse_ino_t_dummy_struct {
>  };
>  #endif
>  
> +struct lo_map_elem {
> +union {
> +/* Element values will go here... */
> +ssize_t freelist;
> +};
> +bool in_use;
> +};
> +
> +/* Maps FUSE fh or ino values to internal objects */
> +struct lo_map {
> +struct lo_map_elem *elems;
> +size_t nelems;
> +ssize_t freelist;
> +};
> +
>  struct lo_inode {
>  struct lo_inode *next; /* protected by lo->mutex */
>  struct lo_inode *prev; /* protected by lo->mutex */
> @@ -130,6 +145,115 @@ static struct lo_data *lo_data(fuse_req_t req)
>  return (struct lo_data *)fuse_req_userdata(req);
>  }
>  
> +__attribute__((unused)) static void lo_map_init(struct lo_map *map)
> +{
> +map->elems = NULL;
> +map->nelems = 0;
> +map->freelist = -1;
> +}
> +
> +__attribute__((unused)) static void lo_map_destroy(struct lo_map *map)
> +{
> +free(map->elems);
> +}
> +
> +static int lo_map_grow(struct lo_map *map, size_t new_nelems)
> +{
> +struct lo_map_elem *new_elems;
> +size_t i;
> +
> +if (new_nelems <= map->nelems) {
> +return 1;
> +}
> +
> +new_elems = realloc(map->elems, sizeof(map->elems[0]) * new_nelems);
> +if (!new_elems) {
> +return 0;
> +}
> +
> +for (i = map->nelems; i < new_nelems; i++) {
> +new_elems[i].freelist = i + 1;
> +new_elems[i].in_use = false;
> +}
> +new_elems[new_nelems - 1].freelist = -1;
> +
> +map->elems = new_elems;
> +map->freelist = map->nelems;
> +map->nelems = new_nelems;
> +return 1;
> +}
> +
> +__attribute__((unused)) static struct lo_map_elem *
> +lo_map_alloc_elem(struct lo_map *map)
> +{
> +struct lo_map_elem *elem;
> +
> +if (map->freelist == -1 && !lo_map_grow(map, map->nelems + 256)) {
> +return NULL;
> +}
> +
> +elem = >elems[map->freelist];
> +map->freelist = elem->freelist;
> +
> +elem->in_use = true;
> +
> +return elem;
> +}
> +
> +__attribute__((unused)) static struct lo_map_elem *
> +lo_map_reserve(struct lo_map *map, size_t key)
> +{
> +ssize_t *prev;
> +
> +if (!lo_map_grow(map, key + 1)) {
> +return NULL;
> +}
> +
> +for (prev = >freelist; *prev != -1;
> + prev = >elems[*prev].freelist) {
> +if (*prev == key) {
> +struct lo_map_elem *elem = >elems[key];
> +
> +*prev = elem->freelist;
> +elem->in_use = true;
> +return elem;
> +}
> +}
> +return NULL;
> +}
> +
> +__attribute__((unused)) static struct lo_map_elem *
> +lo_map_get(struct lo_map *map, size_t key)
> +{
> +if (key >= map->nelems) {
> +return NULL;
> +}
> +if (!map->elems[key].in_use) {
> +return NULL;
> +}
> +return >elems[key];
> +}
> +
> +__attribute__((unused)) static void lo_map_remove(struct lo_map *map,
> +  size_t key)
> +{
> +struct lo_map_elem *elem;
> +
> +if (key >= map->nelems) {
> +return;
> +}
> +
> +elem = >elems[key];
> +if (!elem->in_use) {
> +return;
> +}
> +
> +elem->in_use = false;
> +
> +elem->freelist = map->freelist;
> +map->freelist = key;
> +}
> +
>  static struct lo_inode *lo_inode(fuse_req_t req, fuse_ino_t ino)
>  {
>  if (ino == FUSE_ROOT_ID) {

Looks good to me.

Reviewed-by: Masayoshi Mizuma

Re: [PATCH v1] vnc: fix VNC artifacts

2020-01-17 Thread Cameron Esfahani via

I’m new to this process, what are the next steps?

Cameron Esfahani
di...@apple.com

> On Jan 16, 2020, at 11:47 PM, Gerd Hoffmann  wrote:
> 
> On Thu, Jan 16, 2020 at 07:50:58PM -0800, Cameron Esfahani wrote:
>> Remove VNC optimization to reencode framebuffer update as raw if it's
>> smaller than the default encoding.  QEMU's implementation was naive and
>> didn't account for the ZLIB z_stream mutating with each compression.  Just
>> saving and restoring the output buffer offset wasn't sufficient to "rewind"
>> the previous encoding.  Considering that ZRLE is never larger than raw and
>> even though ZLIB can occasionally be fractionally larger than raw, the
>> overhead of implementing this optimization correctly isn't worth it.
> 
> So just revert de3f7de7f4e257 then ...
> 
>> In my investigation, ZRLE always compresses better than ZLIB so
>> prioritize ZRLE over ZLIB, even if the client hints that ZLIB is
>> preferred.
> 
> ... and make this a separate patch?
> 
> cheers,
>  Gerd
> 
>

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

2020-01-17 Thread John David Anglin

This is not related to the patch but there is one other corner issue with the 
load and clear instructions.

When the target register is GR0, the instruction may be implemented as a normal 
load and clear
which clears memory, or it may be aliased to the equivalent-sized load 
instruction, in which case
it behaves exactly like that prefetch instruction and does not clear the data 
in memory.

See page 6-12 on PA 2.0 architecture.

Don't know what was actually done.

Regards,
Dave

-- 
John David Anglin  dave.ang...@bell.net

help

2020-01-17 Thread Fairouz Fakhfakh

Hello,
I would like to use qemu to install a virtual machine (VM) in Ubuntu.
I need a minimal VM  which contains only jdk (openjdk).

Can someone help me, please.
Thank you in advance.

Re: [PATCH 039/104] virtiofsd: Plumb fuse_bufvec through to do_write_buf

On Thu, Dec 12, 2019 at 04:37:59PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Let fuse_session_process_buf_int take a fuse_bufvec * instead of a
> fuse_buf;  and then through to do_write_buf - where in the best
> case it can pass that straight through to op.write_buf without copying
> (other than skipping a header).
> 
> Signed-off-by: Dr. David Alan Gilbert 
> Signed-off-by: Stefan Hajnoczi 
> ---
>  tools/virtiofsd/fuse_i.h|  2 +-
>  tools/virtiofsd/fuse_lowlevel.c | 61 ++---
>  tools/virtiofsd/fuse_virtio.c   |  3 +-
>  3 files changed, 44 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/virtiofsd/fuse_i.h b/tools/virtiofsd/fuse_i.h
> index cb1ca70ffa..d0679508cd 100644
> --- a/tools/virtiofsd/fuse_i.h
> +++ b/tools/virtiofsd/fuse_i.h
> @@ -119,7 +119,7 @@ int fuse_send_reply_iov_nofree(fuse_req_t req, int error, 
> struct iovec *iov,
>  void fuse_free_req(fuse_req_t req);
>  
>  void fuse_session_process_buf_int(struct fuse_session *se,
> -  const struct fuse_buf *buf,
> +  struct fuse_bufvec *bufv,
>struct fuse_chan *ch);
>  
>  
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index bea092b454..7d33bbf539 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -1006,11 +1006,12 @@ static void do_write(fuse_req_t req, fuse_ino_t 
> nodeid, const void *inarg)
>  }
>  
>  static void do_write_buf(fuse_req_t req, fuse_ino_t nodeid, const void 
> *inarg,
> - const struct fuse_buf *ibuf)
> + struct fuse_bufvec *ibufv)
>  {
>  struct fuse_session *se = req->se;
> -struct fuse_bufvec bufv = {
> -.buf[0] = *ibuf,
> +struct fuse_bufvec *pbufv = ibufv;
> +struct fuse_bufvec tmpbufv = {
> +.buf[0] = ibufv->buf[0],
>  .count = 1,
>  };
>  struct fuse_write_in *arg = (struct fuse_write_in *)inarg;
> @@ -1020,22 +1021,31 @@ static void do_write_buf(fuse_req_t req, fuse_ino_t 
> nodeid, const void *inarg,
>  fi.fh = arg->fh;
>  fi.writepage = arg->write_flags & FUSE_WRITE_CACHE;
>  
> -fi.lock_owner = arg->lock_owner;
> -fi.flags = arg->flags;
> -if (!(bufv.buf[0].flags & FUSE_BUF_IS_FD)) {
> -bufv.buf[0].mem = PARAM(arg);
> -}
> -
> -bufv.buf[0].size -=
> -sizeof(struct fuse_in_header) + sizeof(struct fuse_write_in);
> -if (bufv.buf[0].size < arg->size) {
> -fuse_log(FUSE_LOG_ERR, "fuse: do_write_buf: buffer size too 
> small\n");
> -fuse_reply_err(req, EIO);
> -return;
> +if (ibufv->count == 1) {
> +fi.lock_owner = arg->lock_owner;
> +fi.flags = arg->flags;
> +if (!(tmpbufv.buf[0].flags & FUSE_BUF_IS_FD)) {
> +tmpbufv.buf[0].mem = PARAM(arg);
> +}
> +tmpbufv.buf[0].size -=
> +sizeof(struct fuse_in_header) + sizeof(struct fuse_write_in);
> +if (tmpbufv.buf[0].size < arg->size) {
> +fuse_log(FUSE_LOG_ERR,
> + "fuse: do_write_buf: buffer size too small\n");
> +fuse_reply_err(req, EIO);
> +return;
> +}
> +tmpbufv.buf[0].size = arg->size;
> +pbufv = 
> +} else {
> +/*
> + *  Input bufv contains the headers in the first element
> + * and the data in the rest, we need to skip that first element
> + */
> +ibufv->buf[0].size = 0;
>  }
> -bufv.buf[0].size = arg->size;
>  
> -se->op.write_buf(req, nodeid, , arg->offset, );
> +se->op.write_buf(req, nodeid, pbufv, arg->offset, );
>  }
>  
>  static void do_flush(fuse_req_t req, fuse_ino_t nodeid, const void *inarg)
> @@ -2027,13 +2037,24 @@ static const char *opname(enum fuse_opcode opcode)
>  void fuse_session_process_buf(struct fuse_session *se,
>const struct fuse_buf *buf)
>  {
> -fuse_session_process_buf_int(se, buf, NULL);
> +struct fuse_bufvec bufv = { .buf[0] = *buf, .count = 1 };
> +fuse_session_process_buf_int(se, , NULL);
>  }
>  
> +/*
> + * Restriction:
> + *   bufv is normally a single entry buffer, except for a write
> + *   where (if it's in memory) then the bufv may be multiple entries,
> + *   where the first entry contains all headers and subsequent entries
> + *   contain data
> + *   bufv shall not use any offsets etc to make the data anything
> + *   other than contiguous starting from 0.
> + */
>  void fuse_session_process_buf_int(struct fuse_session *se,
> -  const struct fuse_buf *buf,
> +  struct fuse_bufvec *bufv,
>struct fuse_chan *ch)
>  {
> +const struct fuse_buf *buf = bufv->buf;
>  struct fuse_in_header *in;
>  const void *inarg;
>  struct fuse_req *req;
> @@

Re: [PATCH 08/12] linux-user: Add support for setting alsa timer enhanced read using ioctl

2020-01-17 Thread Aleksandar Markovic

On Thu, Jan 16, 2020 at 1:00 PM Arnd Bergmann  wrote:
>
> On Thu, Jan 16, 2020 at 12:27 PM Aleksandar Markovic
>  wrote:
> > On Thursday, January 16, 2020, Aleksandar Markovic 
> >  wrote:
> >> On Wednesday, January 15, 2020, Laurent Vivier  wrote:
> >>> Le 15/01/2020 à 20:17, Filip Bozuta a écrit :
> >>> > On 15.1.20. 17:37, Arnd Bergmann wrote:
> >>> >> On Wed, Jan 15, 2020 at 5:32 PM Laurent Vivier  
> >>> >> wrote:
> >>> >>> Le 15/01/2020 à 17:18, Arnd Bergmann a écrit :
> >>>  On Wed, Jan 15, 2020 at 4:53 PM Filip Bozuta
> >>>   wrote:
> >>> > This patch implements functionality of following ioctl:
> >>> >
> >>> > SNDRV_TIMER_IOCTL_TREAD - Setting enhanced time read
> >>> >
> >>> >  Sets enhanced time read which is used for reading time with
> >>> > timestamps
> >>> >  and events. The third ioctl's argument is a pointer to an
> >>> > 'int'. Enhanced
> >>> >  reading is set if the third argument is different than 0,
> >>> > otherwise normal
> >>> >  time reading is set.
> >>> >
> >>> > Implementation notes:
> >>> >
> >>> >  Because the implemented ioctl has 'int' as its third argument,
> >>> > the
> >>> >  implementation was straightforward.
> >>> >
> >>> > Signed-off-by: Filip Bozuta 
> >>>  I think this one is wrong when you go between 32-bit and 64-bit
> >>> >>> kernel uses an "int" and "int" is always 32bit.
> >>> >>> The problem is most likely with timespec I think.
> >>> >>>
> >>>  targets, and it gets worse with the kernel patches that just got
> >>>  merged for linux-5.5, which extends the behavior to deal with
> >>>  64-bit time_t on 32-bit architectures.
> >>> 
> >>>  Please have a look at
> >>>  https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/log/?h=80fe7430c70859
> >>> 
> >>> >>> Yes, we already had the same kind of problem with SIOCGSTAMP and
> >>> >>> SIOCGSTAMPNS.
> >>> >>>
> >>> >>> Do the kernel patches add new ioctl numbers to differentiate 32bit and
> >>> >>> 64bit time_t?
> >>> >> Yes, though SNDRV_TIMER_IOCTL_TREAD is worse: the ioctl argument
> >>> >> is a pure 'int' that decides what format you get when you 'read' from 
> >>> >> the
> >>> >> same file descriptor.
> >>> >>
> >>> >> For emulating 64-bit on 32-bit kernels, you have to use the new
> >>> >> SNDRV_TIMER_IOCTL_TREAD64, and for emulating 32-bit on
> >>> >> 64-bit kernels, you probably have to return -ENOTTY to
> >>> >> SNDRV_TIMER_IOCTL_TREAD_OLD unless you also want to
> >>> >> emulate the read() behavior.
> >>> >> When a 32-bit process calls SNDRV_TIMER_IOCTL_TREAD64,
> >>> >> you can translate that into the 64-bit
> >>> >> SNDRV_TIMER_IOCTL_TREAD_OLD.
> >>> >
> >>> > Thank you for bringing this up to my attention. Unfortunately i have
> >>> > some duties of academic nature in next month so i won't have much time
> >>> > fix this bug. I will try to fix this as soon as possible.
> >>>
> >>> Could you at least to try to have a mergeable series before you have to
> >>> stop to work on this?
> >>>
> >>> You can only manage the case before the change reported by Arnd (I will
> >>> manage the new case myself later).
>
> >>
> >> Sorry for interjecting myself into this discussion, but I just want to let 
> >> you know about some related practicalities.
> >>
> >> Filip is a student that is from time to time (usually between two exam 
> >> seasons) an intern in our company, working 4 hours each work day. He spent 
> >> his internship in different teams in prevous years, and, from around 
> >> mid-October 2019, was appointed to QEMU team. After some introductory 
> >> tasks, he was assigned his main task: linux-user support for RTCs and ALSA 
> >> timers. This series is the result of his work, and, to my great pleasure, 
> >> is virtually entirely his independant work. I am positive he can complete 
> >> the series by himself, even in the light of additional complexities 
> >> mentioned in this thread.
> >>
> >> However, his exam season just started (Jan. 15th), and lasts till Feb. 
> >> 15th. Our policy, in general, is not to burden the students during exam 
> >> seasons, and that is why we can't expect prompt updates from him for the 
> >> time being.
> >>
> >> In view of this, Laurent, please take Filip's status into consideration. 
> >> As far as mergeability is concerned, my impression is that patches 1-6 and 
> >> 13 are ready for merging, while patches 7-12 would require some additional 
> >> (netlink-support-like) work, that would unfortunately be possible only 
> >> after Feb. 15th.
> >>
>
> > Laurent, hi again.
> >
> > I am not completely familiar with all details of Filip's work, since, as I 
> > said, he had
> > large degree of independance (which was intentional, and is a desired and 
> > good
> > thing IMHO), but taking a closer look, a question starting lingering: Do we 
> > need
> > special handling od read() even for RTC devices - not only ALSA timer 
> >

RE: [PATCH v2] uas: fix super speed bMaxPacketSize0

2020-01-17 Thread fys

I am okay with this patch.  Thank you.

Ben

> -Original Message-
> From: Gerd Hoffmann 
> Sent: Friday, January 17, 2020 12:37 AM
> To: qemu-devel@nongnu.org
> Cc: f...@fysnet.net; Gerd Hoffmann 
> Subject: [PATCH v2] uas: fix super speed bMaxPacketSize0
> 
> For usb2 bMaxPacketSize0 is "n", for usb3 it is "1 << n", so it must be 9 not 
> 64
> ...
> 
> rom "Universal Serial Bus 3.1 Specification":
> 
>If the device is operating at Gen X speed, the bMaxPacketSize0
>field shall be set to 09H indicating a 512-byte maximum packet.
>An Enhanced SuperSpeed device shall not support any other maximum
>packet sizes for the default control pipe (endpoint 0) control
>endpoint.
> 
> We now announce a 512-byte maximum packet.
> 
> Fixes: 89a453d4a5c ("uas-uas: usb3 streams")
> Reported-by: Benjamin David Lunt 
> Signed-off-by: Gerd Hoffmann 
> Reviewed-by: Philippe Mathieu-Daudé 
> ---
>  hw/usb/dev-uas.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/dev-uas.c b/hw/usb/dev-uas.c index
> 6d6d1073b907..1bc4dd4fafb8 100644
> --- a/hw/usb/dev-uas.c
> +++ b/hw/usb/dev-uas.c
> @@ -303,7 +303,7 @@ static const USBDescDevice desc_device_high = {
> 
>  static const USBDescDevice desc_device_super = {
>  .bcdUSB= 0x0300,
> -.bMaxPacketSize0   = 64,
> +.bMaxPacketSize0   = 9,
>  .bNumConfigurations= 1,
>  .confs = (USBDescConfig[]) {
>  {
> --
> 2.18.1

[Bug 1859920] Re: daemoniz not working on MacOS

2020-01-17 Thread JS

I mixed up some thing in #2 above. Please ignore it and use following:

---

I tried with following and it work:

qemu-system-x86_64 -no-user-config -nodefaults -name u64d01 -M
q35,accel=hvf,usb=off,vmport=off -cpu host -smp 4 -m 8192 -overcommit
mem-lock=off -overcommit cpu-pm=off -rtc base=utc,clock=host -device
virtio-blk-pci,drive=ssd1 -drive
id=ssd1,file=/Users/js/code/vm/qemu/u64s01.qcow2,if=none,format=qcow2
-device virtio-net-pci,netdev=nic1,mac=52:54:98:76:54:33 -netdev
user,id=nic1,ipv4=on,ipv6=on,hostname=u64d01,hostfwd=tcp::-:22
-daemonize -display none -device virtio-tablet-pci -device virtio-vga
-show-cursor

The difference from my original command:

(1) removed -audiodev
(2) added -display none

So (1) and (2) together allow -daemonize work correctly.

Other observation during testing:

- If I only do (1), but not (2):

  - The command will not exit. I can break it with ctrl-c.
  - A qemu-system-x86_64 process is created in background, but I cannot ssh 
into the it. I have use 'kill' to kill it.

- If I don't do (1), regardless of (2), I get following errors(as in my
bug description):

objc[1962]: +[NSNumber initialize] may have been in progress in another thread 
when fork() was called.
objc[1962]: +[NSNumber initialize] may have been in progress in another thread 
when fork() was called. We cannot safely call it or ignore it in the fork() 
child process. Crashing instead. Set a breakpoint on 
objc_initializeAfterForkError to debug.

Not sure if above observations are expected or unhandled error.

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

Title:
  daemoniz not working on MacOS

Status in QEMU:
  Incomplete

Bug description:
  OS: MacOS Catalina 10.15.2
  Qemu install via brew: brew install qemu

  qemu-system-x86_64 -version
  QEMU emulator version 4.2.50 (v4.2.0-13-g084a398bf8-dirty)
  Copyright (c) 2003-2019 Fabrice Bellard and the QEMU Project developers

  ---

  Start Ubuntu Desktop 18.04 client as follow:

  IMG_CD=$HOME/Downloads/iso/ubuntu-18.04.3-desktop-amd64.iso
  IMG_FILE=$HOME/code/vm/qemu/u64d01.qcow2
  MAC_ADDR=xx:xx:xx:xx:xx:xx

  qemu-system-x86_64 \
  -no-user-config -nodefaults \
  -show-cursor \
  -name u64d01 \
  -M q35,accel=hvf,usb=off,vmport=off \
  -cpu host -smp 4 -m 2048 \
  -overcommit mem-lock=off \
  -overcommit cpu-pm=off \
  -rtc base=utc,clock=host \
  \
  -device virtio-tablet-pci \
  -device virtio-vga \
  \
  -device virtio-blk-pci,drive=ssd1 \
  -drive id=ssd1,file=$IMG_FILE,if=none,format=qcow2 \
  \
  -device virtio-net-pci,netdev=nic1,mac=$MAC_ADDR \
  -netdev user,id=nic1,ipv4=on,ipv6=on,hostname=u64d01,hostfwd=tcp::-:22 \
  \
  -device ich9-intel-hda,id=snd,msi=on \
  -device hda-output,id=snd-codec0,bus=snd.0,cad=0,audiodev=snd0 \
  -audiodev coreaudio,id=snd0,out.buffer-count=1 \
  \
  -daemonize

  Give following error:

  objc[3432]: +[NSNumber initialize] may have been in progress in another 
thread when fork() was called.
  objc[3432]: +[NSNumber initialize] may have been in progress in another 
thread when fork() was called. We cannot safely call it or ignore it in the 
fork() child process. Crashing instead. Set a breakpoint on 
objc_initializeAfterForkError to debug.

  
  I checked "ps -ef|grep qemu" before and after the command, there was no qemu 
process running.

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

[Bug 1859920] Re: daemoniz not working on MacOS

2020-01-17 Thread JS

I tried with following and it work:

qemu-system-x86_64 -no-user-config -nodefaults -name u64d01 -M
q35,accel=hvf,usb=off,vmport=off -cpu host -smp 4 -m 8192 -overcommit
mem-lock=off -overcommit cpu-pm=off -rtc base=utc,clock=host -device
virtio-blk-pci,drive=ssd1 -drive
id=ssd1,file=/Users/js/code/vm/qemu/u64s01.qcow2,if=none,format=qcow2
-device virtio-net-pci,netdev=nic1,mac=52:54:98:76:54:33 -netdev
user,id=nic1,ipv4=on,ipv6=on,hostname=u64d01,hostfwd=tcp::-:22
-daemonize -display none -device virtio-tablet-pci -device virtio-vga
-show-cursor

The difference from my original command:
(1) removed -audiodev
(2) added -display none

So (1) and (2) together allow -daemonize work correctly.

Other observation during testing:

- If I only do (1), but not (2), the command will not exit, I cannot ssh
into the vm. So it seems vm initialization is stuck. I can ctrl-c to
break it.

- If I don't do (2), regardless of (1), I get following errors:
objc[1962]: +[NSNumber initialize] may have been in progress in another thread 
when fork() was called.
objc[1962]: +[NSNumber initialize] may have been in progress in another thread 
when fork() was called. We cannot safely call it or ignore it in the fork() 
child process. Crashing instead. Set a breakpoint on 
objc_initializeAfterForkError to debug.

Not sure if above observations are expected or unhandled error.

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

Title:
  daemoniz not working on MacOS

Status in QEMU:
  Incomplete

Bug description:
  OS: MacOS Catalina 10.15.2
  Qemu install via brew: brew install qemu

  qemu-system-x86_64 -version
  QEMU emulator version 4.2.50 (v4.2.0-13-g084a398bf8-dirty)
  Copyright (c) 2003-2019 Fabrice Bellard and the QEMU Project developers

  ---

  Start Ubuntu Desktop 18.04 client as follow:

  IMG_CD=$HOME/Downloads/iso/ubuntu-18.04.3-desktop-amd64.iso
  IMG_FILE=$HOME/code/vm/qemu/u64d01.qcow2
  MAC_ADDR=xx:xx:xx:xx:xx:xx

  qemu-system-x86_64 \
  -no-user-config -nodefaults \
  -show-cursor \
  -name u64d01 \
  -M q35,accel=hvf,usb=off,vmport=off \
  -cpu host -smp 4 -m 2048 \
  -overcommit mem-lock=off \
  -overcommit cpu-pm=off \
  -rtc base=utc,clock=host \
  \
  -device virtio-tablet-pci \
  -device virtio-vga \
  \
  -device virtio-blk-pci,drive=ssd1 \
  -drive id=ssd1,file=$IMG_FILE,if=none,format=qcow2 \
  \
  -device virtio-net-pci,netdev=nic1,mac=$MAC_ADDR \
  -netdev user,id=nic1,ipv4=on,ipv6=on,hostname=u64d01,hostfwd=tcp::-:22 \
  \
  -device ich9-intel-hda,id=snd,msi=on \
  -device hda-output,id=snd-codec0,bus=snd.0,cad=0,audiodev=snd0 \
  -audiodev coreaudio,id=snd0,out.buffer-count=1 \
  \
  -daemonize

  Give following error:

  objc[3432]: +[NSNumber initialize] may have been in progress in another 
thread when fork() was called.
  objc[3432]: +[NSNumber initialize] may have been in progress in another 
thread when fork() was called. We cannot safely call it or ignore it in the 
fork() child process. Crashing instead. Set a breakpoint on 
objc_initializeAfterForkError to debug.

  
  I checked "ps -ef|grep qemu" before and after the command, there was no qemu 
process running.

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

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

2020-01-17 Thread Helge Deller

On 17.01.20 20:23, Richard Henderson wrote:
> On 1/17/20 8:50 AM, Philippe Mathieu-Daudé wrote:
>> On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
>>  wrote:
>>>
>>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>>> or the operation is undefined.  However, real hardware only generates
>>> an unaligned access trap for unaligned mod 4.
>>
>> This Linux kernel commit seems relevant:
>>
>> https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164
>>
>>   /* From: "Jim Hull" 
>>   I've attached a summary of the change, but basically, for PA 2.0, as
>>   long as the ",CO" (coherent operation) completer is specified, then the
>>   16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>>   they only require "natural" alignment (4-byte for ldcw, 8-byte for
>>   ldcd). */
>
> It isn't completely relevant.  We don't implement hppa 2.0.
>
> I added a TODO comment for HPPA64, as I said in reply to Dave Anglin elsewhere
> in this thread.

I agree. A TODO comment should be sufficient for now.

Thanks!
Helge

Re: [PATCH] qemu-img: Add --target-is-zero to convert

2020-01-17 Thread no-reply

Patchew URL: 
https://patchew.org/QEMU/20200117103434.1363985-1-david.edmond...@oracle.com/



Hi,

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

Message-id: 20200117103434.1363985-1-david.edmond...@oracle.com
Type: series
Subject: [PATCH] qemu-img: Add --target-is-zero to convert

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

From https://github.com/patchew-project/qemu
 * [new tag] 
patchew/20200117103434.1363985-1-david.edmond...@oracle.com -> 
patchew/20200117103434.1363985-1-david.edmond...@oracle.com
Switched to a new branch 'test'
71f2a28 qemu-img: Add --target-is-zero to convert

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

total: 1 errors, 0 warnings, 62 lines checked

Commit 71f2a28bebbe (qemu-img: Add --target-is-zero to convert) has style 
problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH] qapi: Fix code generation with Python 3.5

2020-01-17 Thread John Snow




On 1/17/20 2:07 AM, Markus Armbruster wrote:
> John Snow  writes:
> 
>> On 1/16/20 3:25 PM, Markus Armbruster wrote:
>>> Recent commit 3e7fb5811b "qapi: Fix code generation for empty modules"
>>> modules" switched QAPISchema.visit() from
>>>
>>> for entity in self._entity_list:
>>>
>>> effectively to
>>>
>>> for mod in self._module_dict.values():
>>> for entity in mod._entity_list:
>>>
>>> Visits in the same order as long as .values() is in insertion order.
>>> That's the case only for Python 3.6 and later.  Before, it's in some
>>> arbitrary order, which results in broken generated code.
>>>
>>> Fix by making self._module_dict an OrderedDict rather than a dict.
>>>
>>> Fixes: 3e7fb5811baab213dcc7149c3aa69442d683c26c
>>> Signed-off-by: Markus Armbruster 
>>> ---
>>>  scripts/qapi/schema.py | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>>> index 0bfc5256fb..5100110fa2 100644
>>> --- a/scripts/qapi/schema.py
>>> +++ b/scripts/qapi/schema.py
>>> @@ -795,7 +795,7 @@ class QAPISchema(object):
>>>  self.docs = parser.docs
>>>  self._entity_list = []
>>>  self._entity_dict = {}
>>> -self._module_dict = {}
>>> +self._module_dict = OrderedDict()
>>>  self._schema_dir = os.path.dirname(fname)
>>>  self._make_module(None) # built-ins
>>>  self._make_module(fname)
>>>
>>
>> This problem has bitten me *many* times. I'm wondering if there's a
>> prescription that isn't just "Wait until we can stipulate 3.6+".
> 
> No clue.
> 
> 3.5 EOL is scheduled for 2020-09-13.
> https://devguide.python.org/#status-of-python-branches
> 
> We support 3.5 because we support Debian 9.
> 
> We'd normally drop support for Debian 9 two years after Debian 10,
> i.e. July 2021.  Assuming Debian supports it that far.  Whether they can
> truly support Python 3.5 after uptstream EOL seems doubtful.
> 

We should decide whether we consider Debian LTS to be adequately
supported, yes-or-no.

We should use a rule of "two years after successor, or End-of-Support,
whichever comes first."

For Debian, is end of support three years after it comes out, or is it
when the LTS is EOL?

In this specific case, do we trust that Debian 9 LTS will continue to
patch Python3.5 all the way up until July 2021?

--js

Re: [PATCH 2/4] linux-user: Use `qemu_log' for strace

On Tue, Jan 14, 2020 at 2:56 AM Alex Bennée  wrote:
> Could we cut out the middle-man and just qemu_add_log(LOG_STRACE) here
> and drop the enable_strace variable.

This code has been re-factored based on Laurent's comments. In v2
`enable_strace` is still needed to avoid QEMU_LOG/-d overwriting the
mask set by -strace.

> Please don't drop the brace - c.f. CODING_STYLE.rst

Thanks for the catch!

> Side note, print_taken_signal might want to consider using the
> qemu_log_lock() functionality to keep sequential qemu_log's together. I
> suspect you might want to do the same for syscalls.

This is likely true, but outside the scope of this patch (this patch
preserves the current bad behavior). When I get some spare time I'll
try and address this in a follow up patch series.

Re: [PATCH 2/4] linux-user: Use `qemu_log' for strace

On Tue, Jan 14, 2020 at 1:09 AM Laurent Vivier  wrote:
> This is really a special case as the flags are all given at the same
> time, could you use directly qemu_set_log() in main()?

Fixed in v2.

> > +/* Remove (subtract) the given log flags from the current log mask. */
> > +static inline void qemu_del_log(int log_flags)
> > +{
> > +qemu_set_log(qemu_loglevel & ~(log_flags));
> > +}
>
> Unused and unneeded.

Removed in v2. Also removed `qemu_add_log`.

Re: [PATCH 1/4] linux-user: Use `qemu_log' for non-strace logging

On Tue, Jan 14, 2020 at 2:43 AM Alex Bennée  wrote:
>
> As Laurent said I think LOG_UNIMP is perfectly fine for stuff we haven't
> done. I don't think any of the cases warrant LOG_GUEST_ERROR.

I've replaced `LOG_USER` with `LOG_UNIMP`. Didn't catch that mask at
first, LOG_UNIMP is indeed a better fit for most of these.

> I'm not sure want to bother with this. I know we like to avoid
> regression but isn't this all debug log stuff? If we must keep it can we
> invert the variable to save the initialisation.

I'm all for removing it. I agree that it would be pretty odd for
someone to depend on debug log output in this way. I've removed this
in v2. It is a backwards incompatibility though, so I'm happy to
re-add the fix if people want it.

> I mean we jumped through hoops to maintain backwards compatibility and
> then added new output? Also LOG_STRACE doesn't exist yet.

This is leftover debug statements. I've removed it.

> I'm not sure we shouldn't just be asserting this case above. The
> comments imply it is a bug on our part. The rest look like good cases
> for LOG_UNIMP.

Switched to an assert in v2.

[PATCH v2 4/4] bsd-user: Replace gemu_log with qemu_log

gemu_log is an old logging mechanism used to implement strace logging in
the bsd-user tree. It logs directly to stderr and cannot easily be
redirected. This change instead causes strace to log via the qemu_log
subsystem which has fine-grained logging control, and a centralized
mechanism for log redirection. bsd-user does not currently implement any
logging redirection options, or log masking options, but this change
brings it more in line with the linux-user tree.

Signed-off-by: Josh Kunz 
---
 bsd-user/main.c| 13 ++---
 bsd-user/qemu.h|  2 --
 bsd-user/strace.c  | 32 +++-
 bsd-user/syscall.c | 31 +++
 4 files changed, 36 insertions(+), 42 deletions(-)

diff --git a/bsd-user/main.c b/bsd-user/main.c
index 770c2b267a..2ca975d52c 100644
--- a/bsd-user/main.c
+++ b/bsd-user/main.c
@@ -55,15 +55,6 @@ enum BSDType bsd_type;
by remapping the process stack directly at the right place */
 unsigned long x86_stack_size = 512 * 1024;
 
-void gemu_log(const char *fmt, ...)
-{
-va_list ap;
-
-va_start(ap, fmt);
-vfprintf(stderr, fmt, ap);
-va_end(ap);
-}
-
 #if defined(TARGET_I386)
 int cpu_get_pic_interrupt(CPUX86State *env)
 {
@@ -845,7 +836,7 @@ int main(int argc, char **argv)
 } else if (!strcmp(r, "singlestep")) {
 singlestep = 1;
 } else if (!strcmp(r, "strace")) {
-do_strace = 1;
+qemu_add_log(LOG_STRACE);
 } else if (!strcmp(r, "trace")) {
 g_free(trace_file);
 trace_file = trace_opt_parse(optarg);
@@ -917,7 +908,7 @@ int main(int argc, char **argv)
 thread_cpu = cpu;
 
 if (getenv("QEMU_STRACE")) {
-do_strace = 1;
+qemu_add_log(LOG_STRACE);
 }
 
 target_environ = envlist_to_environ(envlist, NULL);
diff --git a/bsd-user/qemu.h b/bsd-user/qemu.h
index 09e8aed9c7..5762e3a6e5 100644
--- a/bsd-user/qemu.h
+++ b/bsd-user/qemu.h
@@ -152,7 +152,6 @@ abi_long do_netbsd_syscall(void *cpu_env, int num, abi_long 
arg1,
 abi_long do_openbsd_syscall(void *cpu_env, int num, abi_long arg1,
 abi_long arg2, abi_long arg3, abi_long arg4,
 abi_long arg5, abi_long arg6);
-void gemu_log(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 extern THREAD CPUState *thread_cpu;
 void cpu_loop(CPUArchState *env);
 char *target_strerror(int err);
@@ -188,7 +187,6 @@ print_openbsd_syscall(int num,
   abi_long arg1, abi_long arg2, abi_long arg3,
   abi_long arg4, abi_long arg5, abi_long arg6);
 void print_openbsd_syscall_ret(int num, abi_long ret);
-extern int do_strace;
 
 /* signal.c */
 void process_pending_signals(CPUArchState *cpu_env);
diff --git a/bsd-user/strace.c b/bsd-user/strace.c
index fa66fe1ee2..6ee1510555 100644
--- a/bsd-user/strace.c
+++ b/bsd-user/strace.c
@@ -23,8 +23,6 @@
 
 #include "qemu.h"
 
-int do_strace;
-
 /*
  * Utility functions
  */
@@ -36,17 +34,17 @@ static void print_sysctl(const struct syscallname *name, 
abi_long arg1,
 uint32_t i;
 int32_t *namep;
 
-gemu_log("%s({ ", name->name);
+qemu_log("%s({ ", name->name);
 namep = lock_user(VERIFY_READ, arg1, sizeof(int32_t) * arg2, 1);
 if (namep) {
 int32_t *p = namep;
 
 for (i = 0; i < (uint32_t)arg2; i++) {
-gemu_log("%d ", tswap32(*p++));
+qemu_log("%d ", tswap32(*p++));
 }
 unlock_user(namep, arg1, 0);
 }
-gemu_log("}, %u, 0x" TARGET_ABI_FMT_lx ", 0x" TARGET_ABI_FMT_lx ", 0x"
+qemu_log("}, %u, 0x" TARGET_ABI_FMT_lx ", 0x" TARGET_ABI_FMT_lx ", 0x"
 TARGET_ABI_FMT_lx ", 0x" TARGET_ABI_FMT_lx ")",
 (uint32_t)arg2, arg3, arg4, arg5, arg6);
 }
@@ -62,7 +60,7 @@ static void print_execve(const struct syscallname *name, 
abi_long arg1,
 if (s == NULL) {
 return;
 }
-gemu_log("%s(\"%s\",{", name->name, s);
+qemu_log("%s(\"%s\",{", name->name, s);
 unlock_user(s, arg1, 0);
 
 for (arg_ptr_addr = arg2; ; arg_ptr_addr += sizeof(abi_ulong)) {
@@ -78,11 +76,11 @@ static void print_execve(const struct syscallname *name, 
abi_long arg1,
 break;
 }
 if ((s = lock_user_string(arg_addr))) {
-gemu_log("\"%s\",", s);
+qemu_log("\"%s\",", s);
 unlock_user(s, arg_addr, 0);
 }
 }
-gemu_log("NULL})");
+qemu_log("NULL})");
 }
 
 static void print_ioctl(const struct syscallname *name,
@@ -90,7 +88,7 @@ static void print_ioctl(const struct syscallname *name,
 abi_long arg5, abi_long arg6)
 {
 /* Decode the ioctl request */
-gemu_log("%s(%d, 0x%0lx { IO%s%s GRP:0x%x('%c') CMD:%d LEN:%d }, 0x"
+qemu_log("%s(%d, 0x%0lx { IO%s%s GRP:0x%x('%c') CMD:%d LEN:%d }, 0x"
 TARGET_ABI_FMT_lx ", ...)",
 name->name,
 (int)arg1,
@@ -111,9 +109,9 @@ static void print_ioctl(const struct syscallname *name,
 static void

[PATCH v2 1/4] linux-user: Use `qemu_log' for non-strace logging

Since most calls to `gemu_log` are actually logging unimplemented features,
this change replaces most non-strace calls to `gemu_log` with calls to
`qemu_log_mask(LOG_UNIMP, ...)`.  This allows the user to easily log to
a file, and to mask out these log messages if they desire.

Note: This change is slightly backwards incompatible, since now these
"unimplemented" log messages will not be logged by default.

Signed-off-by: Josh Kunz 
---
 linux-user/arm/cpu_loop.c |  5 ++--
 linux-user/fd-trans.c | 55 +--
 linux-user/syscall.c  | 55 +--
 linux-user/vm86.c |  3 ++-
 4 files changed, 69 insertions(+), 49 deletions(-)

diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index 1fae90c6df..cf618daa1c 100644
--- a/linux-user/arm/cpu_loop.c
+++ b/linux-user/arm/cpu_loop.c
@@ -349,8 +349,9 @@ void cpu_loop(CPUARMState *env)
 env->regs[0] = cpu_get_tls(env);
 break;
 default:
-gemu_log("qemu: Unsupported ARM syscall: 0x%x\n",
- n);
+qemu_log_mask(LOG_UNIMP,
+  "qemu: Unsupported ARM syscall: 
0x%x\n",
+  n);
 env->regs[0] = -TARGET_ENOSYS;
 break;
 }
diff --git a/linux-user/fd-trans.c b/linux-user/fd-trans.c
index 9b92386abf..c0687c52e6 100644
--- a/linux-user/fd-trans.c
+++ b/linux-user/fd-trans.c
@@ -514,7 +514,8 @@ static abi_long host_to_target_data_bridge_nlattr(struct 
nlattr *nlattr,
 u32[1] = tswap32(u32[1]); /* optmask */
 break;
 default:
-gemu_log("Unknown QEMU_IFLA_BR type %d\n", nlattr->nla_type);
+qemu_log_mask(LOG_UNIMP, "Unknown QEMU_IFLA_BR type %d\n",
+  nlattr->nla_type);
 break;
 }
 return 0;
@@ -577,7 +578,8 @@ static abi_long 
host_to_target_slave_data_bridge_nlattr(struct nlattr *nlattr,
 case QEMU_IFLA_BRPORT_BRIDGE_ID:
 break;
 default:
-gemu_log("Unknown QEMU_IFLA_BRPORT type %d\n", nlattr->nla_type);
+qemu_log_mask(LOG_UNIMP, "Unknown QEMU_IFLA_BRPORT type %d\n",
+  nlattr->nla_type);
 break;
 }
 return 0;
@@ -605,7 +607,8 @@ static abi_long host_to_target_data_tun_nlattr(struct 
nlattr *nlattr,
 *u32 = tswap32(*u32);
 break;
 default:
-gemu_log("Unknown QEMU_IFLA_TUN type %d\n", nlattr->nla_type);
+qemu_log_mask(LOG_UNIMP, "Unknown QEMU_IFLA_TUN type %d\n",
+  nlattr->nla_type);
 break;
 }
 return 0;
@@ -652,7 +655,8 @@ static abi_long host_to_target_data_linkinfo_nlattr(struct 
nlattr *nlattr,
   NULL,
 
host_to_target_data_tun_nlattr);
 } else {
-gemu_log("Unknown QEMU_IFLA_INFO_KIND %s\n", li_context->name);
+qemu_log_mask(LOG_UNIMP, "Unknown QEMU_IFLA_INFO_KIND %s\n",
+  li_context->name);
 }
 break;
 case QEMU_IFLA_INFO_SLAVE_DATA:
@@ -663,12 +667,13 @@ static abi_long 
host_to_target_data_linkinfo_nlattr(struct nlattr *nlattr,
   NULL,

host_to_target_slave_data_bridge_nlattr);
 } else {
-gemu_log("Unknown QEMU_IFLA_INFO_SLAVE_KIND %s\n",
+qemu_log_mask(LOG_UNIMP, "Unknown QEMU_IFLA_INFO_SLAVE_KIND %s\n",
  li_context->slave_name);
 }
 break;
 default:
-gemu_log("Unknown host QEMU_IFLA_INFO type: %d\n", nlattr->nla_type);
+qemu_log_mask(LOG_UNIMP, "Unknown host QEMU_IFLA_INFO type: %d\n",
+  nlattr->nla_type);
 break;
 }
 
@@ -690,7 +695,8 @@ static abi_long host_to_target_data_inet_nlattr(struct 
nlattr *nlattr,
 }
 break;
 default:
-gemu_log("Unknown host AF_INET type: %d\n", nlattr->nla_type);
+qemu_log_mask(LOG_UNIMP, "Unknown host AF_INET type: %d\n",
+  nlattr->nla_type);
 }
 return 0;
 }
@@ -741,7 +747,8 @@ static abi_long host_to_target_data_inet6_nlattr(struct 
nlattr *nlattr,
 }
 break;
 default:
-gemu_log("Unknown host AF_INET6 type: %d\n", nlattr->nla_type);
+qemu_log_mask(LOG_UNIMP, "Unknown host AF_INET6 type: %d\n",
+  nlattr->nla_type);
 }
 return 0;
 }
@@ -759,7 +766,8 @@ static abi_long host_to_target_data_spec_nlattr(struct 
nlattr *nlattr,
   NULL,
  host_to_target_data_inet6_nlattr);
 default:
-gemu_log("Unknown host AF_SPEC type:

[PATCH v2 2/4] linux-user: Use `qemu_log' for strace

This change switches linux-user strace logging to use the newer `qemu_log`
logging subsystem rather than the older `gemu_log` (notice the "g")
logger. `qemu_log` has several advantages, namely that it allows logging
to a file, and provides a more unified interface for configuration
of logging (via the QEMU_LOG environment variable or options).

This change introduces a new log mask: `LOG_STRACE` which is used for
logging of user-mode strace messages.

Signed-off-by: Josh Kunz 
---
 include/qemu/log.h   |   2 +
 linux-user/main.c|  30 ++-
 linux-user/qemu.h|   1 -
 linux-user/signal.c  |   2 +-
 linux-user/strace.c  | 479 ++-
 linux-user/syscall.c |  13 +-
 util/log.c   |   2 +
 7 files changed, 278 insertions(+), 251 deletions(-)

diff --git a/include/qemu/log.h b/include/qemu/log.h
index e0f4e40628..f4724f7330 100644
--- a/include/qemu/log.h
+++ b/include/qemu/log.h
@@ -62,6 +62,8 @@ static inline bool qemu_log_separate(void)
 #define CPU_LOG_TB_OP_IND  (1 << 16)
 #define CPU_LOG_TB_FPU (1 << 17)
 #define CPU_LOG_PLUGIN (1 << 18)
+/* LOG_STRACE is used for user-mode strace logging. */
+#define LOG_STRACE (1 << 19)
 
 /* Lock output for a series of related logs.  Since this is not needed
  * for a single qemu_log / qemu_log_mask / qemu_log_mask_and_addr, we
diff --git a/linux-user/main.c b/linux-user/main.c
index fba833aac9..8f1d07cdd6 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -60,6 +60,19 @@ unsigned long mmap_min_addr;
 unsigned long guest_base;
 int have_guest_base;
 
+/*
+ * Used to implement backwards-compatibility for the `-strace`, and
+ * QEMU_STRACE options. Without this, the QEMU_LOG can be overwritten by
+ * -strace, or vice versa.
+ */
+static bool enable_strace;
+
+/*
+ * The last log mask given by the user in an environment variable or argument.
+ * Used to support command line arguments overriding environment variables.
+ */
+static int last_log_mask;
+
 /*
  * When running 32-on-64 we should make sure we can fit all of the possible
  * guest address space into a contiguous chunk of virtual host memory.
@@ -223,15 +236,11 @@ static void handle_arg_help(const char *arg)
 
 static void handle_arg_log(const char *arg)
 {
-int mask;
-
-mask = qemu_str_to_log_mask(arg);
-if (!mask) {
+last_log_mask = qemu_str_to_log_mask(arg);
+if (!last_log_mask) {
 qemu_print_log_usage(stdout);
 exit(EXIT_FAILURE);
 }
-qemu_log_needs_buffers();
-qemu_set_log(mask);
 }
 
 static void handle_arg_dfilter(const char *arg)
@@ -375,7 +384,7 @@ static void handle_arg_singlestep(const char *arg)
 
 static void handle_arg_strace(const char *arg)
 {
-do_strace = 1;
+enable_strace = true;
 }
 
 static void handle_arg_version(const char *arg)
@@ -629,6 +638,7 @@ int main(int argc, char **argv, char **envp)
 int i;
 int ret;
 int execfd;
+int log_mask;
 unsigned long max_reserved_va;
 
 error_init(argv[0]);
@@ -661,6 +671,12 @@ int main(int argc, char **argv, char **envp)
 
 optind = parse_args(argc, argv);
 
+log_mask = last_log_mask | (enable_strace ? LOG_STRACE : 0);
+if (log_mask) {
+qemu_log_needs_buffers();
+qemu_set_log(log_mask);
+}
+
 if (!trace_init_backends()) {
 exit(1);
 }
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index f6f5fe5fbb..02c6890c8a 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -385,7 +385,6 @@ void print_syscall_ret(int num, abi_long arg1);
  * --- SIGSEGV {si_signo=SIGSEGV, si_code=SI_KERNEL, si_addr=0} ---
  */
 void print_taken_signal(int target_signum, const target_siginfo_t *tinfo);
-extern int do_strace;
 
 /* signal.c */
 void process_pending_signals(CPUArchState *cpu_env);
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 5ca6d62b15..818d867b7b 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -864,7 +864,7 @@ static void handle_pending_signal(CPUArchState *cpu_env, 
int sig,
 handler = sa->_sa_handler;
 }
 
-if (do_strace) {
+if (unlikely(qemu_loglevel_mask(LOG_STRACE))) {
 print_taken_signal(sig, >info);
 }
 
diff --git a/linux-user/strace.c b/linux-user/strace.c
index 3d4d684450..4f7130b2ff 100644
--- a/linux-user/strace.c
+++ b/linux-user/strace.c
@@ -12,8 +12,6 @@
 #include 
 #include "qemu.h"
 
-int do_strace=0;
-
 struct syscallname {
 int nr;
 const char *name;
@@ -80,7 +78,7 @@ print_ipc_cmd(int cmd)
 {
 #define output_cmd(val) \
 if( cmd == val ) { \
-gemu_log(#val); \
+qemu_log(#val); \
 return; \
 }
 
@@ -120,7 +118,7 @@ if( cmd == val ) { \
 output_cmd( IPC_RMID );
 
 /* Some value we don't recognize */
-gemu_log("%d",cmd);
+qemu_log("%d", cmd);
 }
 
 static void
@@ -151,7 +149,7 @@ print_signal(abi_ulong arg, int last)
 print_raw_param("%ld", arg, last);
 return;
 }
-gemu_log("%s%s", signal_name, get_comma(last));
+

[PATCH v2 0/4] migration: Replace gemu_log with qemu_log

Summary of v2 changes:
  * Removed backwards-compatibility code for non-strace log statements.
  * Removed new qemu_log interface for adding or removing fields from
the log mask.
  * Removed LOG_USER and converted all uses (except one) to LOG_UNIMP.
* One gemu_log statement was converted to an assert.
  * Some style cleanup.

The linux-user and bsd-user trees both widely use a function called
`gemu_log` (notice the 'g') for miscellaneous and strace logging. This
function predates the newer `qemu_log` function, and has a few drawbacks
compared to `qemu_log`:

  1. Always logs to `stderr`, no logging redirection.
  2. "Miscellaneous" logging cannot be disabled, so it may mix with guest
 logging.
  3. Inconsistency with other parts of the QEMU codebase, and a
 confusing name.

The second issue is especially troubling because it can interfere with
programs that expect to communicate via stderr.

This change introduces one new logging masks to the `qemu_log` subsystem
to support its use for user-mode logging: the `LOG_STRACE` mask for
strace-specific logging. Further, it replaces all existing uses of
`gemu_log` with the appropriate `qemu_log_mask(LOG_{UNIMP,STRACE}, ...)`
based on the log message.

Backwards incompatibility:
  * Log messages for unimplemented user-mode features are no longer
logged by default. They have to be enabled by setting the LOG_UNIMP
mask.
  * Log messages for strace/unimplemented user-mode features may be
redirected based on `-D`, instead of always logging to stderr.

Tested:
  * Built with clang 9 and g++ 8.3
  * `make check` run with clang 9 build 
  * Verified:
* QEMU_STRACE/-strace still works for linux-user
  * `make vm-build-netbsd EXTRA_CONFIGURE_OPTS="--disable-system"`
passed.

Not tested:
  * Build/logging with bsd-user. I do not have easy access to a BSD system.

Josh Kunz (4):
  linux-user: Use `qemu_log' for non-strace logging
  linux-user: Use `qemu_log' for strace
  linux-user: remove gemu_log from the linux-user tree
  bsd-user: Replace gemu_log with qemu_log

 bsd-user/main.c   |  13 +-
 bsd-user/qemu.h   |   2 -
 bsd-user/strace.c |  32 ++-
 bsd-user/syscall.c|  31 ++-
 include/qemu/log.h|   2 +
 linux-user/arm/cpu_loop.c |   5 +-
 linux-user/fd-trans.c |  55 +++--
 linux-user/main.c |  39 ++--
 linux-user/qemu.h |   2 -
 linux-user/signal.c   |   2 +-
 linux-user/strace.c   | 479 +++---
 linux-user/syscall.c  |  68 +++---
 linux-user/vm86.c |   3 +-
 util/log.c|   2 +
 14 files changed, 383 insertions(+), 352 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog

[PATCH v2 3/4] linux-user: remove gemu_log from the linux-user tree

Now that all uses have been migrated to `qemu_log' it is no longer
needed.

Signed-off-by: Josh Kunz 
---
 linux-user/main.c | 9 -
 linux-user/qemu.h | 1 -
 2 files changed, 10 deletions(-)

diff --git a/linux-user/main.c b/linux-user/main.c
index 8f1d07cdd6..22578b1633 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -112,15 +112,6 @@ const char *qemu_uname_release;
by remapping the process stack directly at the right place */
 unsigned long guest_stack_size = 8 * 1024 * 1024UL;
 
-void gemu_log(const char *fmt, ...)
-{
-va_list ap;
-
-va_start(ap, fmt);
-vfprintf(stderr, fmt, ap);
-va_end(ap);
-}
-
 #if defined(TARGET_I386)
 int cpu_get_pic_interrupt(CPUX86State *env)
 {
diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 02c6890c8a..329b409e65 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -210,7 +210,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 abi_long arg2, abi_long arg3, abi_long arg4,
 abi_long arg5, abi_long arg6, abi_long arg7,
 abi_long arg8);
-void gemu_log(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 extern __thread CPUState *thread_cpu;
 void cpu_loop(CPUArchState *env);
 const char *target_strerror(int err);
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

On 1/17/20 8:50 AM, Philippe Mathieu-Daudé wrote:
> On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
>  wrote:
>>
>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>> or the operation is undefined.  However, real hardware only generates
>> an unaligned access trap for unaligned mod 4.
> 
> This Linux kernel commit seems relevant:
> 
> https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164
> 
>   /* From: "Jim Hull" 
>   I've attached a summary of the change, but basically, for PA 2.0, as
>   long as the ",CO" (coherent operation) completer is specified, then the
>   16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
>   they only require "natural" alignment (4-byte for ldcw, 8-byte for
>   ldcd). */

It isn't completely relevant.  We don't implement hppa 2.0.

I added a TODO comment for HPPA64, as I said in reply to Dave Anglin elsewhere
in this thread.


r~

Re: [PATCH 026/104] virtiofsd: Fast path for virtio read

On Thu, Dec 12, 2019 at 04:37:46PM +, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" 
> 
> Readv the data straight into the guests buffer.
> 
> Signed-off-by: Dr. David Alan Gilbert 
> With fix by:
> Signed-off-by: Eryu Guan 
> ---
>  tools/virtiofsd/fuse_lowlevel.c |   5 +
>  tools/virtiofsd/fuse_virtio.c   | 159 
>  tools/virtiofsd/fuse_virtio.h   |   4 +
>  3 files changed, 168 insertions(+)
> 
> diff --git a/tools/virtiofsd/fuse_lowlevel.c b/tools/virtiofsd/fuse_lowlevel.c
> index c2b114cf5b..5f80625652 100644
> --- a/tools/virtiofsd/fuse_lowlevel.c
> +++ b/tools/virtiofsd/fuse_lowlevel.c
> @@ -475,6 +475,11 @@ static int fuse_send_data_iov_fallback(struct 
> fuse_session *se,
>  return fuse_send_msg(se, ch, iov, iov_count);
>  }
>  
> +if (fuse_lowlevel_is_virtio(se) && buf->count == 1 &&
> +buf->buf[0].flags == (FUSE_BUF_IS_FD | FUSE_BUF_FD_SEEK)) {
> +return virtio_send_data_iov(se, ch, iov, iov_count, buf, len);
> +}
> +
>  abort(); /* Will have taken vhost path */
>  return 0;
>  }
> diff --git a/tools/virtiofsd/fuse_virtio.c b/tools/virtiofsd/fuse_virtio.c
> index c33e0f7e8c..146cd3f702 100644
> --- a/tools/virtiofsd/fuse_virtio.c
> +++ b/tools/virtiofsd/fuse_virtio.c
> @@ -230,6 +230,165 @@ err:
>  return ret;
>  }
>  
> +/*
> + * Callback from fuse_send_data_iov_* when it's virtio and the buffer
> + * is a single FD with FUSE_BUF_IS_FD | FUSE_BUF_FD_SEEK
> + * We need send the iov and then the buffer.
> + * Return 0 on success
> + */
> +int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
> + struct iovec *iov, int count, struct fuse_bufvec 
> *buf,
> + size_t len)
> +{
> +int ret = 0;
> +VuVirtqElement *elem;
> +VuVirtq *q;
> +
> +assert(count >= 1);
> +assert(iov[0].iov_len >= sizeof(struct fuse_out_header));
> +
> +struct fuse_out_header *out = iov[0].iov_base;
> +/* TODO: Endianness! */
> +
> +size_t iov_len = iov_size(iov, count);
> +size_t tosend_len = iov_len + len;
> +
> +out->len = tosend_len;
> +
> +fuse_log(FUSE_LOG_DEBUG, "%s: count=%d len=%zd iov_len=%zd\n", __func__,
> + count, len, iov_len);
> +
> +/* unique == 0 is notification which we don't support */
> +assert(out->unique);
> +
> +/* For virtio we always have ch */
> +assert(ch);
> +assert(!ch->qi->reply_sent);
> +elem = ch->qi->qe;
> +q = >qi->virtio_dev->dev.vq[ch->qi->qidx];
> +
> +/* The 'in' part of the elem is to qemu */
> +unsigned int in_num = elem->in_num;
> +struct iovec *in_sg = elem->in_sg;
> +size_t in_len = iov_size(in_sg, in_num);
> +fuse_log(FUSE_LOG_DEBUG, "%s: elem %d: with %d in desc of length %zd\n",
> + __func__, elem->index, in_num, in_len);
> +
> +/*
> + * The elem should have room for a 'fuse_out_header' (out from fuse)
> + * plus the data based on the len in the header.
> + */
> +if (in_len < sizeof(struct fuse_out_header)) {
> +fuse_log(FUSE_LOG_ERR, "%s: elem %d too short for out_header\n",
> + __func__, elem->index);

> +ret = -E2BIG;

The ret should be positive value, right?

   ret = E2BIG;

> +goto err;
> +}
> +if (in_len < tosend_len) {
> +fuse_log(FUSE_LOG_ERR, "%s: elem %d too small for data len %zd\n",
> + __func__, elem->index, tosend_len);

> +ret = -E2BIG;

   ret = E2BIG;

> +goto err;
> +}
> +
> +/* TODO: Limit to 'len' */
> +
> +/* First copy the header data from iov->in_sg */
> +copy_iov(iov, count, in_sg, in_num, iov_len);
> +
> +/*
> + * Build a copy of the the in_sg iov so we can skip bits in it,
> + * including changing the offsets
> + */

> +struct iovec *in_sg_cpy = calloc(sizeof(struct iovec), in_num);

   assert(in_sg_cpy) should be here? in case calloc() fails...

> +memcpy(in_sg_cpy, in_sg, sizeof(struct iovec) * in_num);
> +/* These get updated as we skip */
> +struct iovec *in_sg_ptr = in_sg_cpy;
> +int in_sg_cpy_count = in_num;
> +
> +/* skip over parts of in_sg that contained the header iov */
> +size_t skip_size = iov_len;
> +
> +size_t in_sg_left = 0;
> +do {
> +while (skip_size != 0 && in_sg_cpy_count) {
> +if (skip_size >= in_sg_ptr[0].iov_len) {
> +skip_size -= in_sg_ptr[0].iov_len;
> +in_sg_ptr++;
> +in_sg_cpy_count--;
> +} else {
> +in_sg_ptr[0].iov_len -= skip_size;
> +in_sg_ptr[0].iov_base += skip_size;
> +break;
> +}
> +}
> +
> +int i;
> +for (i = 0, in_sg_left = 0; i < in_sg_cpy_count; i++) {
> +in_sg_left += in_sg_ptr[i].iov_len;
> +}
> +fuse_log(FUSE_LOG_DEBUG,
> + "%s: after

Re: [PATCH RFC 12/12] migration/rdma: only register the virt-ram block for MultiRDMA

* Zhimin Feng (fengzhim...@huawei.com) wrote:
> From: fengzhimin 
> 
> The virt-ram block is sent by MultiRDMA, so we only register it for
> MultiRDMA channels and main channel don't register the virt-ram block.
> 
> Signed-off-by: fengzhimin 

You can't specialise on the name of the RAMBlock like that.
'mach-virt.ram' is the name specific to just the main ram on just
aarch's machine type;  for example the name on x86 is completely
different and if you use NUMA or hotplug etc it would also be different
on aarch.

Is there a downside to also registering the mach-virt.ram on the main
channel?

Dave

> ---
>  migration/rdma.c | 140 +--
>  1 file changed, 112 insertions(+), 28 deletions(-)
> 
> diff --git a/migration/rdma.c b/migration/rdma.c
> index 0a150099e2..1477fd509b 100644
> --- a/migration/rdma.c
> +++ b/migration/rdma.c
> @@ -618,7 +618,9 @@ const char *print_wrid(int wrid);
>  static int qemu_rdma_exchange_send(RDMAContext *rdma, RDMAControlHeader 
> *head,
> uint8_t *data, RDMAControlHeader *resp,
> int *resp_idx,
> -   int (*callback)(RDMAContext *rdma));
> +   int (*callback)(RDMAContext *rdma,
> +   uint8_t id),
> +   uint8_t id);
>  
>  static inline uint64_t ram_chunk_index(const uint8_t *start,
> const uint8_t *host)
> @@ -1198,24 +1200,81 @@ static int qemu_rdma_alloc_qp(RDMAContext *rdma)
>  return 0;
>  }
>  
> -static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma)
> +/*
> + * Parameters:
> + *@id == UNUSED_ID :
> + *This means that we register memory for the main RDMA channel,
> + *the main RDMA channel don't register the mach-virt.ram block
> + *when we use multiRDMA method to migrate.
> + *
> + *@id == 0 or id == 1 or ... :
> + *This means that we register memory for the multiRDMA channels,
> + *the multiRDMA channels only register memory for the mach-virt.ram
> + *block when we use multiRDAM method to migrate.
> + */
> +static int qemu_rdma_reg_whole_ram_blocks(RDMAContext *rdma, uint8_t id)
>  {
>  int i;
>  RDMALocalBlocks *local = >local_ram_blocks;
>  
> -for (i = 0; i < local->nb_blocks; i++) {
> -local->block[i].mr =
> -ibv_reg_mr(rdma->pd,
> -local->block[i].local_host_addr,
> -local->block[i].length,
> -IBV_ACCESS_LOCAL_WRITE |
> -IBV_ACCESS_REMOTE_WRITE
> -);
> -if (!local->block[i].mr) {
> -perror("Failed to register local dest ram block!\n");
> -break;
> +if (migrate_use_multiRDMA()) {
> +if (id == UNUSED_ID) {
> +for (i = 0; i < local->nb_blocks; i++) {
> +/* main RDMA channel don't register the mach-virt.ram block 
> */
> +if (strcmp(local->block[i].block_name, "mach-virt.ram") == 
> 0) {
> +continue;
> +}
> +
> +local->block[i].mr =
> +ibv_reg_mr(rdma->pd,
> +local->block[i].local_host_addr,
> +local->block[i].length,
> +IBV_ACCESS_LOCAL_WRITE |
> +IBV_ACCESS_REMOTE_WRITE
> +);
> +if (!local->block[i].mr) {
> +perror("Failed to register local dest ram block!\n");
> +break;
> +}
> +rdma->total_registrations++;
> +}
> +} else {
> +for (i = 0; i < local->nb_blocks; i++) {
> +/*
> + * The multiRDAM channels only register
> + * the mach-virt.ram block
> + */
> +if (strcmp(local->block[i].block_name, "mach-virt.ram") == 
> 0) {
> +local->block[i].mr =
> +ibv_reg_mr(rdma->pd,
> +local->block[i].local_host_addr,
> +local->block[i].length,
> +IBV_ACCESS_LOCAL_WRITE |
> +IBV_ACCESS_REMOTE_WRITE
> +);
> +if (!local->block[i].mr) {
> +perror("Failed to register local dest ram block!\n");
> +break;
> +}
> +rdma->total_registrations++;
> +}
> +}
> +}
> +} else {
> +for (i = 0; i < local->nb_blocks; i++) {
> +local->block[i].mr =
> +ibv_reg_mr(rdma->pd,
> +local->block[i].local_host_addr,
> +local->block[i].length,
> +

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

On Fri, Jan 17, 2020 at 2:53 AM Richard Henderson
 wrote:
>
> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
> or the operation is undefined.  However, real hardware only generates
> an unaligned access trap for unaligned mod 4.

This Linux kernel commit seems relevant:

https://github.com/torvalds/linux/commit/14e256c107304#diff-e85862c7227599cb24e36494f75948d5R159-R164

  /* From: "Jim Hull" 
  I've attached a summary of the change, but basically, for PA 2.0, as
  long as the ",CO" (coherent operation) completer is specified, then the
  16-byte alignment requirement for ldcw and ldcd is relaxed, and instead
  they only require "natural" alignment (4-byte for ldcw, 8-byte for
  ldcd). */

(I couldn't find the original post on
http://lists.parisc-linux.org/pipermail/parisc-linux/)

Maybe you can point to it in the description.

> Match real hardware, but diagnose with GUEST_ERROR a violation of the
> specification.
>
> Reported-by: Helge Deller 
> Suggested-by: John David Anglin 
> Signed-off-by: Richard Henderson 
> ---
>
> Helge, can you please test this against your failing kernel?
> You will of course want to add -D logfile -d guest_errors to
> you qemu command-line.
>
>
> r~
>
> ---
>  target/hppa/helper.h| 2 ++
>  target/hppa/op_helper.c | 9 +
>  target/hppa/translate.c | 6 +-
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
> index 38d834ef6b..2d483aab58 100644
> --- a/target/hppa/helper.h
> +++ b/target/hppa/helper.h
> @@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, 
> env, tl, tr)
>  DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
>  DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>
> +DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
> +
>  DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
>
>  DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
> index f0516e81f1..345cef2c08 100644
> --- a/target/hppa/op_helper.c
> +++ b/target/hppa/op_helper.c
> @@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, 
> target_ulong addr,
>  do_stby_e(env, addr, val, true, GETPC());
>  }
>
> +void HELPER(ldc_check)(target_ulong addr)
> +{
> +if (unlikely(addr & 0xf)) {
> +qemu_log_mask(LOG_GUEST_ERROR,
> +  "Undefined ldc to address unaligned mod 16: "
> +  TARGET_FMT_lx "\n", addr);
> +}
> +}
> +
>  target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
>uint32_t level, uint32_t want)
>  {
> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
> index 2f8d407a82..669381dc1d 100644
> --- a/target/hppa/translate.c
> +++ b/target/hppa/translate.c
> @@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)
>
>  static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>  {
> -MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
> +MemOp mop = MO_TE | MO_ALIGN | a->size;
>  TCGv_reg zero, dest, ofs;
>  TCGv_tl addr;
>
> @@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>
>  form_gva(ctx, , , a->b, a->x, a->scale ? a->size : 0,
>   a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
> +
> +gen_helper_ldc_check(addr);
>  zero = tcg_const_reg(0);
>  tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
> +tcg_temp_free(zero);
> +
>  if (a->m) {
>  save_gpr(ctx, a->b, ofs);
>  }
> --
> 2.20.1
>
>

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

On Fri, 17 Jan 2020 at 18:29, Guenter Roeck  wrote:
>
> On Fri, Jan 17, 2020 at 01:48:06PM +, Peter Maydell wrote:
> > On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  wrote:
> > >
> > > The Exynos4210 serial driver uses an interrupt line to signal if receive
> > > data is available. Connect that interrupt with the DMA controller's
> > > 'peripheral busy' gpio pin.

> > Rather than having the uart and pl330 pointers be locals,
> > they should be fields in Exynos4210State. (Otherwise technically
> > we leak them, though this is unnoticeable in practice because there's
> > no way to destroy an Exynos4210State.)
> >
> Out of curiosity: Is that a new leak because they are now tied together,
> or is it an existing leak ? I don't find many DeviceState entries
> in xxxState structures, so find it difficult to determine if/when/why
> there is such a leak.

Yes, it's an existing leak, though it's more of a conceptual leak
than one that valgrind would actually complain about. (The object
isn't totally unreachable because there will be references to it
in the QOM tree. Keeping track of your child objects only becomes
really important if the device is hot-pluggable, because if you
can hot-unplug the device you get a real leak if it hasn't cleaned
up its child objects.)

Aside: I think this code is written this way because although it's
a container device it has been incorrectly written against
the pattern of a board model. Originally board models did not
have any "this is the board" struct that they could keep things
in, so the only way to write board model code that created,
configured and wired up devices was to have it do it in a
way that didn't keep track of the things it created once the
board init function returned. This code is part of an SoC
"container" device, so it does have a state struct that it
could use to hold either embedded device structs or simply
pointers to device objects, but the code is written like the old
board-model code. (These days even board models have a suitable
state struct they can use.)

include/hw/arm/armsse.h is an example of a device state struct
with a lot of embedded device state structs in it. (Not all device
state structs have names ending "State".)

thanks
-- PMM

Re: [PATCH 072/104] virtiofsd: passthrough_ll: fix refcounting on remove/rename

* Miklos Szeredi (mszer...@redhat.com) wrote:
> On Thu, Jan 16, 2020 at 5:45 PM Dr. David Alan Gilbert
>  wrote:
> >
> > * Misono Tomohiro (misono.tomoh...@jp.fujitsu.com) wrote:
> > > > From: Miklos Szeredi 
> > > >
> > > > Signed-off-by: Miklos Szeredi 
> > >
> > > I'm not familiar with qemu convention but shouldn't we put
> > > at least one line of description like linux kernel?
> >
> > Miklos: would you like to suggest a better commit message?
> 
> Hmm, the patch doesn't really make sense, since the looked up inode is not 
> used.
> 
> Not sure what happened here, this seems to be for supporting shared
> versions, and these changes are part of commit 06f78a397f00
> ("virtiofsd: add initial support for shared versions") in our gitlab
> qemu tree.  Was this intentionally split out?

I think the reason I kept this here is because Stefan's
  'introduce inode refcount to prevent use-after-free'

does use the the inodes in lo_rename; is it also dependent on having
done the in lo_rmdir and lo_unlink?

Dave


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

Re: [PATCH 4/6] hw/char/exynos4210_uart: Implement receive FIFO

On Fri, 17 Jan 2020 at 18:21, Guenter Roeck  wrote:
> Doesn't that mean that the .post_load function is missing even today,
> and that it should call exynos4210_uart_update_parameters() ?

Yes, it should, so that's an existing bug. (I think you'll only
notice an ill effect from that if you have wired the QEMU emulated
serial port through to a real host hardware serial port, though.)

thanks
-- PMM

Re: [PATCH v3 2/5] migration: Create MigrationState active field

2020-01-17 Thread Juan Quintela

"Dr. David Alan Gilbert"  wrote:
> * Juan Quintela (quint...@redhat.com) wrote:
>> Right now, there is no easy way to dectect if we have already
>> cancelled/finished/failed a migration.  This field is setup to true
>> when we start a migration, and it is set to false as soon as we stop
>> it.
>> 
>> It fixes a real bug, in ram_save_iterate() we call functions that
>> wrote to the channel even if we know that migration has stopped for
>> any reason.  This gives problems with multifd because we need to
>> synchronize various semoaphores that we don't want to take.
>> 
>> Signed-off-by: Juan Quintela 
>
> Why can't you use migration_is_active() in the ram.c case?
> My preference would be just to stick with something derived
> from the state rather than tacking another state bit on.

My plan was to go the other way around.
We need to use the state with atomics, I wanted a single way of deciding
if we are/or not in the middle of a migration.  Just now it is too
confusing on my opinion.

>> ---
>>  migration/migration.c | 5 +
>>  migration/migration.h | 5 +
>>  migration/ram.c   | 2 +-
>>  migration/savevm.c| 2 ++
>>  4 files changed, 13 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 990bff00c0..60bc8710b6 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -1583,6 +1583,8 @@ static void migrate_fd_cancel(MigrationState *s)
>>  QEMUFile *f = migrate_get_current()->to_dst_file;
>>  trace_migrate_fd_cancel();
>>  
>> +s->active = false;
>> +
>>  if (s->rp_state.from_dst_file) {
>>  /* shutdown the rp socket, so causing the rp thread to shutdown */
>>  qemu_file_shutdown(s->rp_state.from_dst_file);
>> @@ -2834,6 +2836,7 @@ static void migration_completion(MigrationState *s)
>>  }
>>  
>>  if (!migrate_colo_enabled()) {
>> +s->active = false;
>>  migrate_set_state(>state, current_active_state,
>>MIGRATION_STATUS_COMPLETED);
>
> You've not always got these two the same way around - i.e. do you change
> the state first or do you set the active state first?  I think it needs
> to be consistent.

Ok.

Thanks, Juan.

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

On 1/17/20 7:01 AM, Helge Deller wrote:
> Maybe adding something like (if TARGET_32BIT...) now would make sense, so we 
> don't get it
> wrong when 64bit gets added?

I'll add a "TODO: HPPA64" comment.


r~

Re: [PATCH 2/6] hw/arm/exynos4210: Fix DMA initialization

On Fri, 17 Jan 2020 at 18:07, Guenter Roeck  wrote:
>
> Hi Peter,
>
> On Fri, Jan 17, 2020 at 01:30:19PM +, Peter Maydell wrote:
> > On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  wrote:
> > >
> > > First parameter to exynos4210_get_irq() is not the SPI port number,
> > > but the interrupt group number. Interrupt groups are 20 for mdma
> > > and 21 for pdma. Interrupts are not inverted. Controllers support 32
> > > events (pdma) or 31 events (mdma). Events must all be routed to a single
> > > interrupt line. Set other parameters as documented in Exynos4210 
> > > datasheet,
> > > section 8 (DMA controller).
> > >
> > > Fixes: 59520dc65e ("hw/arm/exynos4210: Add DMA support for the 
> > > Exynos4210")
> > > Signed-off-by: Guenter Roeck 
> > > ---
> > >  hw/arm/exynos4210.c | 24 +++-
> > >  1 file changed, 19 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> > > index 77fbe1baab..c7b5c587b1 100644
> > > --- a/hw/arm/exynos4210.c
> > > +++ b/hw/arm/exynos4210.c
> > > @@ -166,17 +166,31 @@ static uint64_t exynos4210_calc_affinity(int cpu)
> > >  return (0x9 << ARM_AFF1_SHIFT) | cpu;
> > >  }
> > >
> > > -static void pl330_create(uint32_t base, qemu_irq irq, int nreq)
> > > +static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int 
> > > nevents,
> > > + int width)
> > >  {
> > >  SysBusDevice *busdev;
> > >  DeviceState *dev;
> > > +int i;
> > >
> > >  dev = qdev_create(NULL, "pl330");
> > > +qdev_prop_set_uint8(dev, "num_events", nevents);
> > > +qdev_prop_set_uint8(dev, "num_chnls",  8);
> > >  qdev_prop_set_uint8(dev, "num_periph_req",  nreq);
> > > +
> > > +qdev_prop_set_uint8(dev, "wr_cap", 4);
> > > +qdev_prop_set_uint8(dev, "wr_q_dep", 8);
> > > +qdev_prop_set_uint8(dev, "rd_cap", 4);
> > > +qdev_prop_set_uint8(dev, "rd_q_dep", 8);
> > > +qdev_prop_set_uint8(dev, "data_width", width);
> > > +qdev_prop_set_uint16(dev, "data_buffer_dep", width);
> > >  qdev_init_nofail(dev);
> > >  busdev = SYS_BUS_DEVICE(dev);
> > >  sysbus_mmio_map(busdev, 0, base);
> > > -sysbus_connect_irq(busdev, 0, irq);
> > > +sysbus_connect_irq(busdev, 0, irq); /* abort irq line */
> > > +for (i = 0; i < nevents; i++) {
> > > +sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
> > > +}
> >
> > It isn't valid to connect multiple qemu_irq outputs to a single
> > input like this. If the hardware logically ORs the irq lines
> > together then you need to instantiate and wire up a TYPE_OR_IRQ
> > device (include/hw/or-irq.h) to do that. Unfortunately QEMU
> > doesn't catch accidental wiring of a qemu_irq to multiple
> > inputs, and it will even sort-of seem to work: the bug is that
> > if two inputs go high, and then one goes low, the destination
> > will get a "signal went low" call even though the first input
> > should still be holding the line high.
> >
> Makes sense. Unfortunately, it isn't easy for the non-initiated to
> figure out how to wire this up. There are several examples, all
> do it differently, and I am having difficulties understanding it.
> I'll try to do it, but it may take a while.

I'd suggest following how hw/arm/armsse.c does it. This is
assuming we need one OR gate for each PL330. Roughly:

 * #include "hw/or-irq.h" in include/hw/arm/exynos4210.h
 * new field in the Exynos4210State struct "qemu_or_irq
pl330_irq_orgate[NUM_PL330S];"
 * TYPE_EXYNOS4210_SOC will need an instance_init, so add
   ".instance_init = exynos4210_init," to the exynos4210_info definition
 * all the instance_init needs to do is call object_initialize_child()
   for each OR gate, something like:
  static void exynos4210_init(Object *obj)
  {
  Exynos4210State *s = EXYNOS4210_SOC(obj);
  int i;

  for (i = 0; i < ARRAY_SIZE(s->pl330_irq_orgate); i++) {
  char *name = g_strdup_printf("pl330-irq-orgate%d", i);
  qemu_or_irq *orgate = >pl330_irq_orgate[i];

  object_initialize_child(obj, name, orgate, sizeof(*orgate),
  TYPE_OR_IRQ, _abort, NULL);
  g_free(name);
  }
  }
  * in exynos4210_realize() you need another loop; the loop body
should set the number of input lines and realize the object:
object_property_set_int(OBJECT(>pl330_irq_orgate[i]),
num_lines,
"num-lines", );
if (err) {
error_propagate(errp, err);
return;
}
object_property_set_bool(OBJECT(>pl330_irq_orgate[i]), true,
 "realized", );
if (err) {
error_propagate(errp, err);
return;
}

where num_lines is I think going to be 33, 33, and 2 (1 + the
value of nevents for that pl330).
It doesn't really matter exactly where in the realize function
we do this as long as the realize of the or-gate

Re: [PATCH 6/6] hw/arm/exynos4210: Connect serial port DMA busy signals with pl330

On Fri, Jan 17, 2020 at 01:48:06PM +, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  wrote:
> >
> > The Exynos4210 serial driver uses an interrupt line to signal if receive
> > data is available. Connect that interrupt with the DMA controller's
> > 'peripheral busy' gpio pin.
> >
> > Signed-off-by: Guenter Roeck 
> > ---
> >  hw/arm/exynos4210.c | 39 ++-
> >  1 file changed, 26 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> > index c7b5c587b1..498d79ebb2 100644
> > --- a/hw/arm/exynos4210.c
> > +++ b/hw/arm/exynos4210.c
> > @@ -166,8 +166,8 @@ static uint64_t exynos4210_calc_affinity(int cpu)
> >  return (0x9 << ARM_AFF1_SHIFT) | cpu;
> >  }
> >
> > -static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int 
> > nevents,
> > - int width)
> > +static DeviceState *pl330_create(uint32_t base, qemu_irq irq, int nreq,
> > + int nevents, int width)
> >  {
> >  SysBusDevice *busdev;
> >  DeviceState *dev;
> > @@ -191,6 +191,7 @@ static void pl330_create(uint32_t base, qemu_irq irq, 
> > int nreq, int nevents,
> >  for (i = 0; i < nevents; i++) {
> >  sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
> >  }
> > +return dev;
> >  }
> >
> >  static void exynos4210_realize(DeviceState *socdev, Error **errp)
> > @@ -199,7 +200,7 @@ static void exynos4210_realize(DeviceState *socdev, 
> > Error **errp)
> >  MemoryRegion *system_mem = get_system_memory();
> >  qemu_irq gate_irq[EXYNOS4210_NCPUS][EXYNOS4210_IRQ_GATE_NINPUTS];
> >  SysBusDevice *busdev;
> > -DeviceState *dev;
> > +DeviceState *dev, *uart[4], *pl330[3];
> 
> Rather than having the uart and pl330 pointers be locals,
> they should be fields in Exynos4210State. (Otherwise technically
> we leak them, though this is unnoticeable in practice because there's
> no way to destroy an Exynos4210State.)
> 
Out of curiosity: Is that a new leak because they are now tied together,
or is it an existing leak ? I don't find many DeviceState entries
in xxxState structures, so find it difficult to determine if/when/why
there is such a leak.

Thanks,
Guenter

[Bug 1860053] Re: Possible lack of precision when calling clock_gettime via vDSO on user mode ppc64le

QEMU does not implement any vDSO, so this cannot be the explanation.

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

Title:
  Possible lack of precision when calling clock_gettime via vDSO on user
  mode ppc64le

Status in QEMU:
  New

Bug description:
  Occurs on QEMU v4.2.0 run on docker (via the qemu-user-static:v4.2.0-2
  image) on an AMD64 Ubuntu 18.04.3 LTS machine provided by travis-
  ci.org.

  From golang's https://github.com/golang/go/issues/36592:

  It was discovered that golang's time.NewTicker() and time.Sleep()
  malfunction when a compiled application was run via QEMU's ppc64le
  emulator in user mode.

  The methods did not malfunction on actual PowerPC hardware or when the
  same golang application was compiled for golang's arm, arm64 or 386
  targets and was run via user mode QEMU on the same system.

  Curiously, the methods also worked when the program was compiled under
  go 1.11, but do malfunction in go 1.12 and 1.13.

  It was identified the change in behaviour was most likely attributable to 
golang switching to using vSDO for calling clock_gettime() on PowerPC 64 
architectures in 1.12. I.E:
  https://github.com/golang/go/commit/dbd8af74723d2c98cbdcc70f7e2801f69b57ac5b

  We therefore suspect there may be a bug in QEMU's user-mode emulation
  of ppc64le as relates to vDSO calls to clock_gettime().

  The nature of the malfunction of time.NewTicker() and time.Sleep() is
  such that sleeps or ticks with a granularity of less than one second
  do not appear to be possible (they all revert to 1 second
  sleeps/ticks). Could it be that the nanoseconds field of
  clock_gettime() is getting lost in the vDSO version but not in the
  syscall? Or some other issue calling these methods via vDSO?

  Thanks in advance.

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

Re: [PATCH 4/6] hw/char/exynos4210_uart: Implement receive FIFO

Hi Peter,

On Fri, Jan 17, 2020 at 01:42:54PM +, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  wrote:
> 
> The subject just says "implement receive FIFO", but the
> existing code clearly has an "Exynos4210UartFIFO   rx"
> which it does some storing and retrieving data from.
> Could the patch be more accurately described as something
> like "Implement interrupts for rx FIFO level triggers and
> timeouts" ?
> 
Sure, no problem.

> > Signed-off-by: Guenter Roeck 
> > ---
> >  hw/char/exynos4210_uart.c | 120 ++
> >  hw/char/trace-events  |   3 +-
> >  2 files changed, 97 insertions(+), 26 deletions(-)
> >
> > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
> > index fb7a3ebd09..61134a7bdc 100644
> > --- a/hw/char/exynos4210_uart.c
> > +++ b/hw/char/exynos4210_uart.c
> > @@ -24,6 +24,7 @@
> >  #include "migration/vmstate.h"
> >  #include "qemu/error-report.h"
> >  #include "qemu/module.h"
> > +#include "qemu/timer.h"
> >  #include "chardev/char-fe.h"
> >  #include "chardev/char-serial.h"
> >
> > @@ -118,6 +119,7 @@ static const Exynos4210UartReg exynos4210_uart_regs[] = 
> > {
> >  #define ULCON_STOP_BIT_SHIFT  1
> >
> >  /* UART Tx/Rx Status */
> > +#define UTRSTAT_Rx_TIMEOUT  0x8
> >  #define UTRSTAT_TRANSMITTER_EMPTY   0x4
> >  #define UTRSTAT_Tx_BUFFER_EMPTY 0x2
> >  #define UTRSTAT_Rx_BUFFER_DATA_READY0x1
> > @@ -147,6 +149,9 @@ typedef struct Exynos4210UartState {
> >  ";
> >  Exynos4210UartFIFO   tx;
> >
> > +QEMUTimer *fifo_timeout_timer;
> > +uint64_t wordtime;/* word time in ns */
> 
> You need to do something on incoming migration to handle
> the new fields. This probably looks like a .post_load function
> that calculates the wordtime based on the register values
> that have been set by the incoming migration and set the
> QEMUTimer appropriately.
> 
Doesn't that mean that the .post_load function is missing even today,
and that it should call exynos4210_uart_update_parameters() ?

Thanks,
Guenter

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4


On 1/17/20 6:30 PM, Richard Henderson wrote:

On 1/17/20 6:13 AM, Philippe Mathieu-Daudé wrote:

-    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
+    MemOp mop = MO_TE | MO_ALIGN | a->size;



Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.

Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?


The "UL" part is also being added by a->size.  This code was written this way
in preparation for the 64-bit ldc, and the bug was not noticable because we
don't have that yet.


Ah I missed the a->size.

So on 32-bit the hw trap doesn't trap on unaligned 16, but traps on 
unaligned 4.


On 64-bit we don't know yet, but IIUC we expect to not trap on unaligned 8.

Looking at target/hppa/insns.decode:

   t b x disp sp m scale size

@ldstx  .. b:5 x:5 sp:2 scale:1 ... m:1 t:5  disp=0
@ldim5  .. b:5 . sp:2 . t:5 \
 disp=%im5_16 x=0 scale=0 m=%ma_to_m

ldc 11 . . .. . 1 -- 0111  ..   @ldim5 
size=2
ldc 11 . . .. . 0 -- 0111  ..   @ldstx 
size=2


We have a->size = 2 = MO_32 = MO_UL.

So do you plan to add LDCD from PA2.0 using size=3, OK.

From "exec/memop.h":

 * MO_ALIGN supposes the alignment size is the size of a memory access.
 *
 * There are three options:
 * - unaligned access permitted (MO_UNALN).
 * - an alignment to the size of an access (MO_ALIGN);

Ah so with LDCW we already access a word, so MO_ALIGN -> MO_ALIGN_4.

With LDLD we'll have MO_ALIGN -> MO_ALIGN_8.

Now "MemOp mop = MO_TE | MO_ALIGN | a->size;" makes sense!

The power of your decodetree script amazed me again :)

Reviewed-by: Philippe Mathieu-Daudé

Re: qemu-x86_64, buster /sbin/ldconfig and setup_arg_pages (a mind dump)

On 1/17/20 7:33 AM, Alex Bennée wrote:
> So what do we take away from this?
> 
>  * we need testcases to exercise the memory layout of dynamic binaries
>  * "special" dynamic binaries can break our careful memory layout
>  * I feel as though I've trodden on a nest of vipers
> 
> Does any of this track with you? What is different about ldconfig that
> breaks our memory placement?

We've been through this bug before, but never came to a resolution.  See

https://bugs.launchpad.net/qemu/+bug/1749393
https://lists.nongnu.org/archive/html/qemu-devel/2018-03/msg04700.html


r~

Re: [PATCH 2/6] hw/arm/exynos4210: Fix DMA initialization

Hi Peter,

On Fri, Jan 17, 2020 at 01:30:19PM +, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  wrote:
> >
> > First parameter to exynos4210_get_irq() is not the SPI port number,
> > but the interrupt group number. Interrupt groups are 20 for mdma
> > and 21 for pdma. Interrupts are not inverted. Controllers support 32
> > events (pdma) or 31 events (mdma). Events must all be routed to a single
> > interrupt line. Set other parameters as documented in Exynos4210 datasheet,
> > section 8 (DMA controller).
> >
> > Fixes: 59520dc65e ("hw/arm/exynos4210: Add DMA support for the Exynos4210")
> > Signed-off-by: Guenter Roeck 
> > ---
> >  hw/arm/exynos4210.c | 24 +++-
> >  1 file changed, 19 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/arm/exynos4210.c b/hw/arm/exynos4210.c
> > index 77fbe1baab..c7b5c587b1 100644
> > --- a/hw/arm/exynos4210.c
> > +++ b/hw/arm/exynos4210.c
> > @@ -166,17 +166,31 @@ static uint64_t exynos4210_calc_affinity(int cpu)
> >  return (0x9 << ARM_AFF1_SHIFT) | cpu;
> >  }
> >
> > -static void pl330_create(uint32_t base, qemu_irq irq, int nreq)
> > +static void pl330_create(uint32_t base, qemu_irq irq, int nreq, int 
> > nevents,
> > + int width)
> >  {
> >  SysBusDevice *busdev;
> >  DeviceState *dev;
> > +int i;
> >
> >  dev = qdev_create(NULL, "pl330");
> > +qdev_prop_set_uint8(dev, "num_events", nevents);
> > +qdev_prop_set_uint8(dev, "num_chnls",  8);
> >  qdev_prop_set_uint8(dev, "num_periph_req",  nreq);
> > +
> > +qdev_prop_set_uint8(dev, "wr_cap", 4);
> > +qdev_prop_set_uint8(dev, "wr_q_dep", 8);
> > +qdev_prop_set_uint8(dev, "rd_cap", 4);
> > +qdev_prop_set_uint8(dev, "rd_q_dep", 8);
> > +qdev_prop_set_uint8(dev, "data_width", width);
> > +qdev_prop_set_uint16(dev, "data_buffer_dep", width);
> >  qdev_init_nofail(dev);
> >  busdev = SYS_BUS_DEVICE(dev);
> >  sysbus_mmio_map(busdev, 0, base);
> > -sysbus_connect_irq(busdev, 0, irq);
> > +sysbus_connect_irq(busdev, 0, irq); /* abort irq line */
> > +for (i = 0; i < nevents; i++) {
> > +sysbus_connect_irq(busdev, i + 1, irq); /* event irq lines */
> > +}
> 
> It isn't valid to connect multiple qemu_irq outputs to a single
> input like this. If the hardware logically ORs the irq lines
> together then you need to instantiate and wire up a TYPE_OR_IRQ
> device (include/hw/or-irq.h) to do that. Unfortunately QEMU
> doesn't catch accidental wiring of a qemu_irq to multiple
> inputs, and it will even sort-of seem to work: the bug is that
> if two inputs go high, and then one goes low, the destination
> will get a "signal went low" call even though the first input
> should still be holding the line high.
> 
Makes sense. Unfortunately, it isn't easy for the non-initiated to
figure out how to wire this up. There are several examples, all
do it differently, and I am having difficulties understanding it.
I'll try to do it, but it may take a while.

Thanks,
Guenter

Re: [PULL 00/15] target-arm queue

On Fri, 17 Jan 2020 at 14:28, Peter Maydell  wrote:
>
> Latest arm queue, a mixed bag of features and bug fixes.
>
> thanks
> -- PMM
>
> The following changes since commit cbf01142b2aef0c0b4e995cecd7e79d342bbc47e:
>
>   Merge remote-tracking branch 'remotes/rth/tags/pull-tcg-20200115' into 
> staging (2020-01-17 12:13:17 +)
>
> are available in the Git repository at:
>
>   https://git.linaro.org/people/pmaydell/qemu-arm.git 
> tags/pull-target-arm-20200117-1
>
> for you to fetch changes up to 1a1fbc6cbb34c26d43d8360c66c1d21681af14a9:
>
>   target/arm: Set ISSIs16Bit in make_issinfo (2020-01-17 14:27:16 +)
>
> 
> Add model of the Netduino Plus 2 board
> Some allwinner-a10 code cleanup
> New test cases for cubieboard
> target/arm/arm-semi: fix SYS_OPEN to return nonzero filehandle
> i.MX: add an emulation for RNGC device
> target/arm: adjust program counter for wfi exception in AArch32
> arm/gicv3: update virtual irq state after IAR register read
> Set IL bit correctly for syndrome information for data aborts
>

Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

[PATCH v2 7/7] tests/bios-tables-test: Update arm/virt memhp test

Since we now have both pc-dimm and nvdimm support, update
test_acpi_virt_tcg_memhp() to include those.

Signed-off-by: Shameer Kolothum 
---
 tests/data/acpi/virt/NFIT.memhp | 0
 tests/data/acpi/virt/SSDT.memhp | 0
 tests/qtest/bios-tables-test.c  | 9 +++--
 3 files changed, 7 insertions(+), 2 deletions(-)
 create mode 100644 tests/data/acpi/virt/NFIT.memhp
 create mode 100644 tests/data/acpi/virt/SSDT.memhp

diff --git a/tests/data/acpi/virt/NFIT.memhp b/tests/data/acpi/virt/NFIT.memhp
new file mode 100644
index 00..e69de29bb2
diff --git a/tests/data/acpi/virt/SSDT.memhp b/tests/data/acpi/virt/SSDT.memhp
new file mode 100644
index 00..e69de29bb2
diff --git a/tests/qtest/bios-tables-test.c b/tests/qtest/bios-tables-test.c
index f1ac2d7e96..695d2e7fac 100644
--- a/tests/qtest/bios-tables-test.c
+++ b/tests/qtest/bios-tables-test.c
@@ -913,12 +913,17 @@ static void test_acpi_virt_tcg_memhp(void)
 };
 
 data.variant = ".memhp";
-test_acpi_one(" -cpu cortex-a57"
+test_acpi_one(" -machine nvdimm=on"
+  " -cpu cortex-a57"
   " -m 256M,slots=3,maxmem=1G"
   " -object memory-backend-ram,id=ram0,size=128M"
   " -object memory-backend-ram,id=ram1,size=128M"
   " -numa node,memdev=ram0 -numa node,memdev=ram1"
-  " -numa dist,src=0,dst=1,val=21",
+  " -numa dist,src=0,dst=1,val=21"
+  " -object memory-backend-ram,id=ram2,size=128M"
+  " -object memory-backend-ram,id=nvm0,size=128M"
+  " -device pc-dimm,id=dimm0,memdev=ram2,node=0"
+  " -device nvdimm,id=dimm1,memdev=nvm0,node=1",
   );
 
 free_test_data();
-- 
2.17.1

[PATCH v2 3/7] nvdimm: Use configurable ACPI IO base and size

From: Kwangwoo Lee 

This patch makes IO base and size configurable to create NPIO AML for
ACPI NFIT. Since a different architecture like AArch64 does not use
port-mapped IO, a configurable IO base is required to create correct
mapping of ACPI IO address and size.

Signed-off-by: Kwangwoo Lee 
Signed-off-by: Eric Auger 
Signed-off-by: Shameer Kolothum 
---
 hw/acpi/nvdimm.c| 32 ++--
 hw/i386/acpi-build.c|  6 ++
 hw/i386/acpi-build.h|  3 +++
 hw/i386/pc_piix.c   |  2 ++
 hw/i386/pc_q35.c|  2 ++
 include/hw/mem/nvdimm.h |  3 +++
 6 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 5e7b8318d0..c28c665b13 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -926,11 +926,13 @@ void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, 
DeviceState *dev)
 }
 
 void nvdimm_init_acpi_state(NVDIMMState *state, MemoryRegion *io,
+struct AcpiGenericAddress dsm_io,
 FWCfgState *fw_cfg, Object *owner)
 {
+state->dsm_io = dsm_io;
 memory_region_init_io(>io_mr, owner, _dsm_ops, state,
-  "nvdimm-acpi-io", NVDIMM_ACPI_IO_LEN);
-memory_region_add_subregion(io, NVDIMM_ACPI_IO_BASE, >io_mr);
+  "nvdimm-acpi-io", dsm_io.bit_width >> 3);
+memory_region_add_subregion(io, dsm_io.address, >io_mr);
 
 state->dsm_mem = g_array_new(false, true /* clear */, 1);
 acpi_data_push(state->dsm_mem, sizeof(NvdimmDsmIn));
@@ -959,13 +961,15 @@ void nvdimm_init_acpi_state(NVDIMMState *state, 
MemoryRegion *io,
 
 #define NVDIMM_QEMU_RSVD_UUID   "648B9CF2-CDA1-4312-8AD9-49C4AF32BD62"
 
-static void nvdimm_build_common_dsm(Aml *dev)
+static void nvdimm_build_common_dsm(Aml *dev,
+NVDIMMState *nvdimm_state)
 {
 Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
 Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
 Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
 Aml *whilectx, *offset;
 uint8_t byte_list[1];
+AmlRegionSpace rs;
 
 method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
 uuid = aml_arg(0);
@@ -976,9 +980,16 @@ static void nvdimm_build_common_dsm(Aml *dev)
 
 aml_append(method, aml_store(aml_name(NVDIMM_ACPI_MEM_ADDR), dsm_mem));
 
+if (nvdimm_state->dsm_io.space_id == AML_AS_SYSTEM_IO) {
+rs = AML_SYSTEM_IO;
+} else {
+rs = AML_SYSTEM_MEMORY;
+}
+
 /* map DSM memory and IO into ACPI namespace. */
-aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, AML_SYSTEM_IO,
-   aml_int(NVDIMM_ACPI_IO_BASE), NVDIMM_ACPI_IO_LEN));
+aml_append(method, aml_operation_region(NVDIMM_DSM_IOPORT, rs,
+   aml_int(nvdimm_state->dsm_io.address),
+   nvdimm_state->dsm_io.bit_width >> 3));
 aml_append(method, aml_operation_region(NVDIMM_DSM_MEMORY,
AML_SYSTEM_MEMORY, dsm_mem, sizeof(NvdimmDsmIn)));
 
@@ -993,7 +1004,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
 field = aml_field(NVDIMM_DSM_IOPORT, AML_DWORD_ACC, AML_NOLOCK,
   AML_PRESERVE);
 aml_append(field, aml_named_field(NVDIMM_DSM_NOTIFY,
-   NVDIMM_ACPI_IO_LEN * BITS_PER_BYTE));
+   nvdimm_state->dsm_io.bit_width));
 aml_append(method, field);
 
 /*
@@ -1290,7 +1301,8 @@ static void nvdimm_build_nvdimm_devices(Aml *root_dev, 
uint32_t ram_slots)
 }
 
 static void nvdimm_build_ssdt(GArray *table_offsets, GArray *table_data,
-  BIOSLinker *linker, GArray *dsm_dma_area,
+  BIOSLinker *linker,
+  NVDIMMState *nvdimm_state,
   uint32_t ram_slots)
 {
 Aml *ssdt, *sb_scope, *dev;
@@ -1318,7 +1330,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, 
GArray *table_data,
  */
 aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0012")));
 
-nvdimm_build_common_dsm(dev);
+nvdimm_build_common_dsm(dev, nvdimm_state);
 
 /* 0 is reserved for root device. */
 nvdimm_build_device_dsm(dev, 0);
@@ -1337,7 +1349,7 @@ static void nvdimm_build_ssdt(GArray *table_offsets, 
GArray *table_data,
NVDIMM_ACPI_MEM_ADDR);
 
 bios_linker_loader_alloc(linker,
- NVDIMM_DSM_MEM_FILE, dsm_dma_area,
+ NVDIMM_DSM_MEM_FILE, nvdimm_state->dsm_mem,
  sizeof(NvdimmDsmIn), false /* high memory */);
 bios_linker_loader_add_pointer(linker,
 ACPI_BUILD_TABLE_FILE, mem_addr_offset, sizeof(uint32_t),
@@ -1359,7 +1371,7 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray 
*table_data,
 return;
 }
 
-nvdimm_build_ssdt(table_offsets, table_data, linker, state->dsm_mem,
+

[PATCH v2 5/7] hw/arm/virt: Add nvdimm hotplug support

This adds support for nvdimm hotplug events through GED
and enables nvdimm for the arm/virt. Now Guests with ACPI
can have both cold and hot plug of nvdimms.

Hot removal functionality is not yet supported.

Signed-off-by: Shameer Kolothum 
Reviewed-by: Igor Mammedov 
---
 docs/specs/acpi_hw_reduced_hotplug.rst |  1 +
 hw/acpi/generic_event_device.c | 13 +
 hw/arm/virt.c  | 16 +++-
 include/hw/acpi/generic_event_device.h |  1 +
 4 files changed, 26 insertions(+), 5 deletions(-)

diff --git a/docs/specs/acpi_hw_reduced_hotplug.rst 
b/docs/specs/acpi_hw_reduced_hotplug.rst
index 911a98255b..e3abe975bf 100644
--- a/docs/specs/acpi_hw_reduced_hotplug.rst
+++ b/docs/specs/acpi_hw_reduced_hotplug.rst
@@ -63,6 +63,7 @@ GED IO interface (4 byte access)
 bits:
0: Memory hotplug event
1: System power down event
+   2: NVDIMM hotplug event
 2-31: Reserved
 
 **write_access:**
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 9cee90cc70..ad1b684304 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -16,6 +16,7 @@
 #include "hw/acpi/generic_event_device.h"
 #include "hw/irq.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/mem/nvdimm.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "qemu/error-report.h"
@@ -23,6 +24,7 @@
 static const uint32_t ged_supported_events[] = {
 ACPI_GED_MEM_HOTPLUG_EVT,
 ACPI_GED_PWR_DOWN_EVT,
+ACPI_GED_NVDIMM_HOTPLUG_EVT,
 };
 
 /*
@@ -110,6 +112,11 @@ void build_ged_aml(Aml *table, const char *name, 
HotplugHandler *hotplug_dev,
aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
   aml_int(0x80)));
 break;
+case ACPI_GED_NVDIMM_HOTPLUG_EVT:
+aml_append(if_ctx,
+   aml_notify(aml_name("\\_SB.NVDR"),
+  aml_int(0x80)));
+break;
 default:
 /*
  * Please make sure all the events in ged_supported_events[]
@@ -175,7 +182,11 @@ static void acpi_ged_device_plug_cb(HotplugHandler 
*hotplug_dev,
 AcpiGedState *s = ACPI_GED(hotplug_dev);
 
 if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
+if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) {
+nvdimm_acpi_plug_cb(hotplug_dev, dev);
+} else {
 acpi_memory_plug_cb(hotplug_dev, >memhp_state, dev, errp);
+}
 } else {
 error_setg(errp, "virt: device plug request for unsupported device"
" type: %s", object_get_typename(OBJECT(dev)));
@@ -192,6 +203,8 @@ static void acpi_ged_send_event(AcpiDeviceIf *adev, 
AcpiEventStatusBits ev)
 sel = ACPI_GED_MEM_HOTPLUG_EVT;
 } else if (ev & ACPI_POWER_DOWN_STATUS) {
 sel = ACPI_GED_PWR_DOWN_EVT;
+} else if (ev & ACPI_NVDIMM_HOTPLUG_STATUS) {
+sel = ACPI_GED_NVDIMM_HOTPLUG_EVT;
 } else {
 /* Unknown event. Return without generating interrupt. */
 warn_report("GED: Unsupported event %d. No irq injected", ev);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7987c8f5b8..5ea2584491 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -543,6 +543,10 @@ static inline DeviceState 
*create_acpi_ged(VirtMachineState *vms)
 event |= ACPI_GED_MEM_HOTPLUG_EVT;
 }
 
+if (ms->nvdimms_state->is_enabled) {
+event |= ACPI_GED_NVDIMM_HOTPLUG_EVT;
+}
+
 dev = qdev_create(NULL, TYPE_ACPI_GED);
 qdev_prop_set_uint32(dev, "ged-event", event);
 
@@ -1928,19 +1932,20 @@ static void virt_memory_pre_plug(HotplugHandler 
*hotplug_dev, DeviceState *dev,
  Error **errp)
 {
 VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+const MachineState *ms = MACHINE(hotplug_dev);
 const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-if (is_nvdimm) {
-error_setg(errp, "nvdimm is not yet supported");
-return;
-}
-
 if (!vms->acpi_dev) {
 error_setg(errp,
"memory hotplug is not enabled: missing acpi-ged device");
 return;
 }
 
+if (is_nvdimm && !ms->nvdimms_state->is_enabled) {
+error_setg(errp, "nvdimm is not enabled: missing 'nvdimm' in '-M'");
+return;
+}
+
 pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
 }
 
@@ -2071,6 +2076,7 @@ static void virt_machine_class_init(ObjectClass *oc, void 
*data)
 hc->plug = virt_machine_device_plug_cb;
 hc->unplug_request = virt_machine_device_unplug_request_cb;
 mc->numa_mem_supported = true;
+mc->nvdimm_supported = true;
 mc->auto_enable_numa_with_memhp = true;
 }
 
diff --git a/include/hw/acpi/generic_event_device.h 
b/include/hw/acpi/generic_event_device.h
index d157eac088..9eb86ca4fd 100644
--- a/include/hw/acpi/generic_event_device.h
+++

[PATCH v2 4/7] hw/arm/virt: Add nvdimm hot-plug infrastructure

From: Kwangwoo Lee 

Prepare pre-plug and plug handlers for NVDIMM support.
Please note nvdimm_support is not yet enabled.

Signed-off-by: Kwangwoo Lee 
Signed-off-by: Eric Auger 
Signed-off-by: Shameer Kolothum 
---
 hw/arm/Kconfig   |  1 +
 hw/arm/virt-acpi-build.c |  6 ++
 hw/arm/virt.c| 19 +++
 hw/mem/Kconfig   |  2 +-
 include/hw/arm/virt.h|  1 +
 5 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index c6e7782580..851dd81289 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -24,6 +24,7 @@ config ARM_VIRT
 select DIMM
 select ACPI_MEMORY_HOTPLUG
 select ACPI_HW_REDUCED
+select ACPI_NVDIMM
 
 config CHEETAH
 bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index bd5f771e9b..c51eae549e 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -44,6 +44,7 @@
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
+#include "hw/mem/nvdimm.h"
 #include "sysemu/numa.h"
 #include "sysemu/reset.h"
 #include "kvm_arm.h"
@@ -839,6 +840,11 @@ void virt_acpi_build(VirtMachineState *vms, 
AcpiBuildTables *tables)
 }
 }
 
+if (ms->nvdimms_state->is_enabled) {
+nvdimm_build_acpi(table_offsets, tables_blob, tables->linker,
+  ms->nvdimms_state, ms->ram_slots);
+}
+
 if (its_class_name() && !vmc->no_its) {
 acpi_add_table(table_offsets, tables_blob);
 build_iort(tables_blob, tables->linker, vms);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 39ab5f47e0..7987c8f5b8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -143,6 +143,7 @@ static const MemMapEntry base_memmap[] = {
 [VIRT_SMMU] =   { 0x0905, 0x0002 },
 [VIRT_PCDIMM_ACPI] ={ 0x0907, MEMORY_HOTPLUG_IO_LEN },
 [VIRT_ACPI_GED] =   { 0x0908, ACPI_GED_EVT_SEL_LEN },
+[VIRT_NVDIMM_ACPI] ={ 0x0909, NVDIMM_ACPI_IO_LEN},
 [VIRT_MMIO] =   { 0x0a00, 0x0200 },
 /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
 [VIRT_PLATFORM_BUS] =   { 0x0c00, 0x0200 },
@@ -1749,6 +1750,18 @@ static void machvirt_init(MachineState *machine)
 
 create_platform_bus(vms);
 
+if (machine->nvdimms_state->is_enabled) {
+const struct AcpiGenericAddress arm_virt_nvdimm_acpi_dsmio = {
+.space_id = AML_AS_SYSTEM_MEMORY,
+.address = vms->memmap[VIRT_NVDIMM_ACPI].base,
+.bit_width = NVDIMM_ACPI_IO_LEN << 3
+};
+
+nvdimm_init_acpi_state(machine->nvdimms_state, sysmem,
+   arm_virt_nvdimm_acpi_dsmio,
+   vms->fw_cfg, OBJECT(vms));
+}
+
 vms->bootinfo.ram_size = machine->ram_size;
 vms->bootinfo.nb_cpus = smp_cpus;
 vms->bootinfo.board_id = -1;
@@ -1936,6 +1949,8 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
 {
 HotplugHandlerClass *hhc;
 VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+MachineState *ms = MACHINE(hotplug_dev);
+bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 Error *local_err = NULL;
 
 pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), _err);
@@ -1943,6 +1958,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
 goto out;
 }
 
+if (is_nvdimm) {
+nvdimm_plug(ms->nvdimms_state);
+}
+
 hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
 hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, _abort);
 out:
diff --git a/hw/mem/Kconfig b/hw/mem/Kconfig
index 620fd4cb59..0d5f8f321a 100644
--- a/hw/mem/Kconfig
+++ b/hw/mem/Kconfig
@@ -8,4 +8,4 @@ config MEM_DEVICE
 config NVDIMM
 bool
 default y
-depends on PC
+depends on PC || ARM_VIRT
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 38f0c33c77..9c9eaaa89d 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -79,6 +79,7 @@ enum {
 VIRT_SECURE_MEM,
 VIRT_PCDIMM_ACPI,
 VIRT_ACPI_GED,
+VIRT_NVDIMM_ACPI,
 VIRT_LOWMEMMAP_LAST,
 };
 
-- 
2.17.1

[PATCH v2 1/7] exec: Fix for qemu_ram_resize() callback

If ACPI blob length modifications happens after the initial
virt_acpi_build() call, and the changed blob length is within
the PAGE size boundary, then the revised size is not seen by
the firmware on Guest reboot. The is because in the
virt_acpi_build_update() -> acpi_ram_update() -> qemu_ram_resize()
path, qemu_ram_resize() uses used_length (ram_block size which is
aligned to PAGE size) and the "resize callback" to update the size
seen by firmware is not getting invoked.

Hence make sure callback is called if the new size is different
from original requested size.

Signed-off-by: Shameer Kolothum 
---
Please find the previous discussions on this issue here,
https://patchwork.kernel.org/patch/11174947/

But this one attempts a different solution to fix it by introducing
req_length var to RAMBlock struct. 

---
 exec.c  | 36 +++-
 include/exec/ram_addr.h |  5 +++--
 2 files changed, 26 insertions(+), 15 deletions(-)

diff --git a/exec.c b/exec.c
index d4b769d0d4..9ce33992f8 100644
--- a/exec.c
+++ b/exec.c
@@ -2123,16 +2123,18 @@ static int memory_try_enable_merging(void *addr, size_t 
len)
  * resize callback to update device state and/or add assertions to detect
  * misuse, if necessary.
  */
-int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize, Error **errp)
+int qemu_ram_resize(RAMBlock *block, ram_addr_t size, Error **errp)
 {
-assert(block);
+ram_addr_t newsize;
 
-newsize = HOST_PAGE_ALIGN(newsize);
+assert(block);
 
-if (block->used_length == newsize) {
+if (block->req_length == size) {
 return 0;
 }
 
+newsize = HOST_PAGE_ALIGN(size);
+
 if (!(block->flags & RAM_RESIZEABLE)) {
 error_setg_errno(errp, EINVAL,
  "Length mismatch: %s: 0x" RAM_ADDR_FMT
@@ -2149,13 +2151,19 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t 
newsize, Error **errp)
 return -EINVAL;
 }
 
-cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
-block->used_length = newsize;
-cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
-DIRTY_CLIENTS_ALL);
-memory_region_set_size(block->mr, newsize);
+block->req_length = size;
+
+if (newsize != block->used_length) {
+cpu_physical_memory_clear_dirty_range(block->offset,
+  block->used_length);
+block->used_length = newsize;
+cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
+DIRTY_CLIENTS_ALL);
+memory_region_set_size(block->mr, newsize);
+}
+
 if (block->resized) {
-block->resized(block->idstr, newsize, block->host);
+block->resized(block->idstr, block->req_length, block->host);
 }
 return 0;
 }
@@ -2412,16 +2420,18 @@ RAMBlock *qemu_ram_alloc_internal(ram_addr_t size, 
ram_addr_t max_size,
   MemoryRegion *mr, Error **errp)
 {
 RAMBlock *new_block;
+ram_addr_t newsize;
 Error *local_err = NULL;
 
-size = HOST_PAGE_ALIGN(size);
+newsize = HOST_PAGE_ALIGN(size);
 max_size = HOST_PAGE_ALIGN(max_size);
 new_block = g_malloc0(sizeof(*new_block));
 new_block->mr = mr;
 new_block->resized = resized;
-new_block->used_length = size;
+new_block->req_length = size;
+new_block->used_length = newsize;
 new_block->max_length = max_size;
-assert(max_size >= size);
+assert(max_size >= newsize);
 new_block->fd = -1;
 new_block->page_size = qemu_real_host_page_size;
 new_block->host = host;
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 5adebb0bc7..fd13082224 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -31,8 +31,9 @@ struct RAMBlock {
 uint8_t *host;
 uint8_t *colo_cache; /* For colo, VM's ram cache */
 ram_addr_t offset;
-ram_addr_t used_length;
-ram_addr_t max_length;
+ram_addr_t req_length; /* Original requested size, used if RAM_RESIZEABLE 
*/
+ram_addr_t used_length; /* aligned to qemu_host_page_size */
+ram_addr_t max_length; /*  aligned to qemu_host_page_size */
 void (*resized)(const char*, uint64_t length, void *host);
 uint32_t flags;
 /* Protected by iothread lock.  */
-- 
2.17.1

[PATCH v2 6/7] tests: Update ACPI tables list for upcoming arm/virt test changes

This is in preparation to update test_acpi_virt_tcg_memhp()
with pc-dimm and nvdimm. Update the bios-tables-test-allowed-diff.h
with the affected ACPI tables so that "make check" doesn't fail.

Signed-off-by: Shameer Kolothum 
---
 tests/qtest/bios-tables-test-allowed-diff.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/qtest/bios-tables-test-allowed-diff.h 
b/tests/qtest/bios-tables-test-allowed-diff.h
index eb8bae1407..862c49e675 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1,3 +1,6 @@
 /* List of comma-separated changed AML files to ignore */
 "tests/data/acpi/pc/SSDT.dimmpxm",
 "tests/data/acpi/q35/SSDT.dimmpxm",
+"tests/data/acpi/virt/DSDT.memhp",
+"tests/data/acpi/virt/SSDT.memhp",
+"tests/data/acpi/virt/NFIT.memhp",
-- 
2.17.1

[PATCH v2 0/7] ARM virt: Add NVDIMM support

This series adds NVDIMM support to arm/virt platform.
The series reuses some of the patches posted by Eric
in his earlier attempt here[1].

Patch #1 is a fix to the Guest reboot issue on NVDIMM
hot add case described here[2] and patch #2 is another
fix to the nvdimm aml issue discussed here[3].

I have done a basic sanity testing of NVDIMM deviecs
with Guest booting with both ACPI and DT. Further testing
is always welcome.

Please let me know your feedback.

Thanks,
Shameer

[1] https://patchwork.kernel.org/cover/10830777/
[2] https://patchwork.kernel.org/patch/11154757/
[3] https://patchwork.kernel.org/cover/11174959/

v1 --> v2
 -Reworked patch #1 and now fix is inside qemu_ram_resize().
 -Added patch #2 to fix the nvdim aml issue.
 -Dropped support to DT cold plug.
 -Updated test_acpi_virt_tcg_memhp() with pc-dimm and nvdimms(patch #7)

Kwangwoo Lee (2):
  nvdimm: Use configurable ACPI IO base and size
  hw/arm/virt: Add nvdimm hot-plug infrastructure

Shameer Kolothum (5):
  exec: Fix for qemu_ram_resize() callback
  hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output  buffer  length
  hw/arm/virt: Add nvdimm hotplug support
  tests: Update ACPI tables list for upcoming arm/virt test changes
  tests/bios-tables-test: Update arm/virt memhp test

 docs/specs/acpi_hw_reduced_hotplug.rst  |  1 +
 exec.c  | 36 +++
 hw/acpi/generic_event_device.c  | 13 
 hw/acpi/nvdimm.c| 68 +
 hw/arm/Kconfig  |  1 +
 hw/arm/virt-acpi-build.c|  6 ++
 hw/arm/virt.c   | 35 +--
 hw/i386/acpi-build.c|  6 ++
 hw/i386/acpi-build.h|  3 +
 hw/i386/pc_piix.c   |  2 +
 hw/i386/pc_q35.c|  2 +
 hw/mem/Kconfig  |  2 +-
 include/exec/ram_addr.h |  5 +-
 include/hw/acpi/generic_event_device.h  |  1 +
 include/hw/arm/virt.h   |  1 +
 include/hw/mem/nvdimm.h |  3 +
 tests/data/acpi/virt/NFIT.memhp |  0
 tests/data/acpi/virt/SSDT.memhp |  0
 tests/qtest/bios-tables-test-allowed-diff.h |  5 ++
 tests/qtest/bios-tables-test.c  |  9 ++-
 20 files changed, 163 insertions(+), 36 deletions(-)
 create mode 100644 tests/data/acpi/virt/NFIT.memhp
 create mode 100644 tests/data/acpi/virt/SSDT.memhp

-- 
2.17.1

[PATCH v2 2/7] hw/acpi/nvdimm: Fix for NVDIMM incorrect DSM output buffer length

As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
the Buffer Field <= to the size of an Integer (in bits), it will
be treated as an integer. Moreover, the integer size depends on
DSDT tables revision number. If revision number is < 2, integer
size is 32 bits, otherwise it is 64 bits. Current NVDIMM common
DSM aml code (NCAL) uses CreateField() for creating DSM output
buffer. This creates an issue in arm/virt platform where DSDT
revision number is 2 and results in DSM buffer with a wrong
size(8 bytes) gets returned when actual length is < 8 bytes.
This causes guest kernel to report,

"nfit ACPI0012:00: found a zero length table '0' parsing nfit"

In order to fix this, aml code is now modified such that it builds
the DSM output buffer in a byte by byte fashion when length is
smaller than Integer size.

Suggested-by: Igor Mammedov 
Signed-off-by: Shameer Kolothum 
---
Please find the previous discussion on this here,
https://patchwork.kernel.org/cover/11174959/

---
 hw/acpi/nvdimm.c| 36 +++--
 tests/qtest/bios-tables-test-allowed-diff.h |  2 ++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c
index 9fdad6dc3f..5e7b8318d0 100644
--- a/hw/acpi/nvdimm.c
+++ b/hw/acpi/nvdimm.c
@@ -964,6 +964,7 @@ static void nvdimm_build_common_dsm(Aml *dev)
 Aml *method, *ifctx, *function, *handle, *uuid, *dsm_mem, *elsectx2;
 Aml *elsectx, *unsupport, *unpatched, *expected_uuid, *uuid_invalid;
 Aml *pckg, *pckg_index, *pckg_buf, *field, *dsm_out_buf, *dsm_out_buf_size;
+Aml *whilectx, *offset;
 uint8_t byte_list[1];
 
 method = aml_method(NVDIMM_COMMON_DSM, 5, AML_SERIALIZED);
@@ -1117,13 +1118,42 @@ static void nvdimm_build_common_dsm(Aml *dev)
 /* RLEN is not included in the payload returned to guest. */
 aml_append(method, aml_subtract(aml_name(NVDIMM_DSM_OUT_BUF_SIZE),
aml_int(4), dsm_out_buf_size));
+
+/*
+ * As per ACPI spec 6.3, Table 19-419 Object Conversion Rules, if
+ * the Buffer Field <= to the size of an Integer (in bits), it will
+ * be treated as an integer. Moreover, the integer size depends on
+ * DSDT tables revision number. If revision number is < 2, integer
+ * size is 32 bits, otherwise it is 64 bits.
+ * Because of this CreateField() canot be used if RLEN < Integer Size.
+ * Hence build dsm_out_buf byte by byte.
+ */
+ifctx = aml_if(aml_lless(dsm_out_buf_size, aml_sizeof(aml_int(0;
+offset = aml_local(2);
+aml_append(ifctx, aml_store(aml_int(0), offset));
+aml_append(ifctx, aml_name_decl("TBUF", aml_buffer(1, NULL)));
+aml_append(ifctx, aml_store(aml_buffer(0, NULL), dsm_out_buf));
+
+whilectx = aml_while(aml_lless(offset, dsm_out_buf_size));
+/* Copy 1 byte at offset from ODAT to temporary buffer(TBUF). */
+aml_append(whilectx, aml_store(aml_derefof(aml_index(
+   aml_name(NVDIMM_DSM_OUT_BUF), offset)),
+   aml_index(aml_name("TBUF"), aml_int(0;
+aml_append(whilectx, aml_concatenate(dsm_out_buf, aml_name("TBUF"),
+ dsm_out_buf));
+aml_append(whilectx, aml_increment(offset));
+aml_append(ifctx, whilectx);
+
+aml_append(ifctx, aml_return(dsm_out_buf));
+aml_append(method, ifctx);
+
+/* If RLEN >= Integer size, just use CreateField() operator */
 aml_append(method, aml_store(aml_shiftleft(dsm_out_buf_size, aml_int(3)),
  dsm_out_buf_size));
 aml_append(method, aml_create_field(aml_name(NVDIMM_DSM_OUT_BUF),
aml_int(0), dsm_out_buf_size, "OBUF"));
-aml_append(method, aml_concatenate(aml_buffer(0, NULL), aml_name("OBUF"),
-   dsm_out_buf));
-aml_append(method, aml_return(dsm_out_buf));
+aml_append(method, aml_return(aml_name("OBUF")));
+
 aml_append(dev, method);
 }
 
diff --git a/tests/qtest/bios-tables-test-allowed-diff.h 
b/tests/qtest/bios-tables-test-allowed-diff.h
index dfb8523c8b..eb8bae1407 100644
--- a/tests/qtest/bios-tables-test-allowed-diff.h
+++ b/tests/qtest/bios-tables-test-allowed-diff.h
@@ -1 +1,3 @@
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/pc/SSDT.dimmpxm",
+"tests/data/acpi/q35/SSDT.dimmpxm",
-- 
2.17.1

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

On Fri, Jan 17, 2020 at 05:05:07PM +, Peter Maydell wrote:
> On Fri, 17 Jan 2020 at 16:46, Guenter Roeck  wrote:
> >
> > On Fri, Jan 17, 2020 at 01:23:46PM +, Peter Maydell wrote:
> > > Won't this now do all the work of constructing the hexdump strings,
> > > even if tracing is disabled ?
> > >
> > That is correct. Can I check
> > if (trace_event_get_state(TRACE_PL330_HEXDUMP) &&
> > qemu_loglevel_mask(LOG_TRACE)) {
> > directly in pl330_hexdump(), or is there some other means to handle
> > this kind of situation ?
> 
> It's not something I've had to do before.
> docs/devel/tracing.txt says "just use the TRACE_FOO_ENABLED
> macro", but looking at what it does that doesn't seem to check
> the runtime state of the trace event, so maybe those docs are out
> of date. Stefan, what's the current best way to guard expensive
> computations used only for trace output ?
> 
trace_event_get_state_backends(TRACE_PL330_HEXDUMP), maybe ?

Thanks,
Guenter

Re: [for-5.0 PATCH 00/11] Support for reverse debugging with GDB

2020-01-17 Thread Alex Bennée



Pavel Dovgalyuk  writes:

>> From: Alex Bennée [mailto:alex.ben...@linaro.org]
>> Pavel Dovgalyuk  writes:
>> 
>> > GDB remote protocol supports reverse debugging of the targets.
>> > It includes 'reverse step' and 'reverse continue' operations.
>> > The first one finds the previous step of the execution,
>> > and the second one is intended to stop at the last breakpoint that
>> > would happen when the program is executed normally.
>> >
>> > Reverse debugging is possible in the replay mode, when at least
>> > one snapshot was created at the record or replay phase.
>> > QEMU can use these snapshots for travelling back in time with GDB.
>> >
>> > Running the execution in replay mode allows using GDB reverse debugging
>> > commands:
>> >  - reverse-stepi (or rsi): Steps one instruction to the past.
>> >QEMU loads on of the prior snapshots and proceeds to the desired
>> >instruction forward. When that step is reaches, execution stops.
>> >  - reverse-continue (or rc): Runs execution "backwards".
>> >QEMU tries to find breakpoint or watchpoint by loaded prior snapshot
>> >and replaying the execution. Then QEMU loads snapshots again and
>> >replays to the latest breakpoint. When there are no breakpoints in
>> >the examined section of the execution, QEMU finds one more snapshot
>> >and tries again. After the first snapshot is processed, execution
>> >stops at this snapshot.
>> >
>> > The set of patches include the following modifications:
>> >  - gdbstub update for reverse debugging support
>> >  - functions that automatically perform reverse step and reverse
>> >continue operations
>> >  - hmp/qmp commands for manipulating the replay process
>> >  - improvement of the snapshotting for saving the execution step
>> >in the snapshot parameters
>> >
>> > The patches are available in the repository:
>> > https://github.com/ispras/qemu/tree/rr-191223
>> 
>> So I tried with your additional patch. Launching QEMU as:
>> 
>>   ./aarch64-softmmu//qemu-system-aarch64 -monitor none \
>>  -display none -M virt -cpu max -display none \
>>  -semihosting-config enable=on \
>>  -kernel ./tests/tcg/aarch64-softmmu/memory \
>>  -icount shift=5,rr=replay,rrfile=record.bin \
>>  -s -S -d trace:gdbstub\*
>> 
>> And gdb:
>> 
>>   gdb tests/tcg/aarch64-softmmu/memory \
>> -ex "target remote localhost:1234"
>> 
>> I get the following log:
>> 
>>   (gdb) x/3i $pc
>>   => 0x400037b0 <__start>:adr x0, 0x40003000 
>>  0x400037b4 <__start+4>:  msr vbar_el1, x0
>>  0x400037b8 <__start+8>:  adrpx0, 0x4020
>>   (gdb) p/x $x0
>>   $1 = 0x0
>>   (gdb) si
>>   92  msr vbar_el1, x0
>>   (gdb) p/x $x0
>>   $2 = 0x40003000
>>   (gdb) rsi
>>   warning: Remote failure reply: E14
>> 
>>   Program stopped.
>>   __start () at 
>> /home/alex.bennee/lsrc/qemu.git/tests/tcg/aarch64/system/boot.S:92
>>   92  msr vbar_el1, x0
>>   (gdb) p/x $x0
>>   $3 = 0x40003000
>> 
>> So it doesn't seem to be working.
>
> That's ok, you'll need at least one VM snapshot available to recover the 
> initial VM state.
> Try changing the command lines in the following way:
>
> First, create empty.qcow2 which will be used for saving the snapshots.
> Then record with initial snapshot and attached empty.qcow2:
>
>./aarch64-softmmu//qemu-system-aarch64 -monitor none \
>   -display none -M virt -cpu max \
>   -kernel ./tests/tcg/aarch64-softmmu/memory \
>   -icount shift=5,rr=record,rrfile=record.bin,rrsnapshot=init \
>   -drive file=empty.qcow2

./aarch64-softmmu//qemu-system-aarch64 -monitor none -display none -M virt -cpu 
max -display none -semihosting-config enable=on -kernel 
./tests/tcg/aarch64-softmmu/memory -icount 
shift=5,rr=record,rrfile=record.bin,rrsnapshot=init -drive file=empty.qcow2
qemu-system-aarch64: invalid accelerator kvm
qemu-system-aarch64: falling back to tcg
qemu-system-aarch64: The qcow format used by node '#block163' does not support 
live migration
qemu-system-aarch64: Could not create snapshot for icount record

I don't know if this is down to the current state of the tree but I
haven't been able to get it working.

>
> After that you can replay the execution with the support of reverse debugging:
>
>./aarch64-softmmu//qemu-system-aarch64 -monitor none \
>   -display none -M virt -cpu max \
>   -kernel ./tests/tcg/aarch64-softmmu/memory \
>   -icount shift=5,rr=replay,rrfile=record.bin,rrsnapshot=init \
>   -drive file=empty.qcow2
>
> I removed "-semihosting-config enable=on", because I'm not sure what is it 
> for.
> It may break the replay, because there is no implementation of
> semihosting input record/replay.

For this testcase semihosting in just a convenient output device (in
lieu of a serial device). We probably need to come up with a strategy on
how we handle all these devices otherwise we will end up with a random
selection of hardware combinations which work.

>
>

Re: [PATCH v2 66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup

2020-01-17 Thread BALATON Zoltan


On Fri, 17 Jan 2020, Igor Mammedov wrote:

On Fri, 17 Jan 2020 16:38:02 +0100
Philippe Mathieu-Daudé  wrote:


On 1/17/20 11:46 AM, Igor Mammedov wrote:

On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
BALATON Zoltan  wrote:


[...]

v2 basically works the same as original code, except of that
instead of fixing up ram_size, it ask user to fix CLI to
the same value.


but I've not yet given up to keep current functionality
(also for other boards where this series would drop it). If you don't like
a generic callback then how about letting the board optionally specify in
the machine class next to default_ram_size some constraints such as
min_ram_size, max_ram_size and an array of valid_ram_sizes (like
ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation
could check and clamp sizes to the nearest valid one emitting a warning
and only bail out if constraints are not given. This would cover most
cases expect the e500 where it has an alignment without max so
representing it as a list of valid values is probably not practical (could
also have ram_alignment but then a generic callback may be simpler). Would
that way work for you?


I agree that we need to consolidate ram_size checks,
that would allow to clean up boards that practically do
the mostly the same. In the end it would allow to introspect
supported ram_sizes which would be good for upper layers.

I even started something in that direction but dropped
all of that patches as not directly related to this
series purpose.
Phillipe is also trying to something like this for raspi
board.

As for clamping, I still think that it's not good idea
to accept invalid value and fix it up.
Users should get what they asked for or get a clear error
(it's double true if user is management software as it
won't read any sort of warnings, well not all human users
would head warnings either as far as QEMU continues to run)

Anyways, most maintainers agreed to dropping fixups
and generalizing checks or generalizing fix up for convenience
of users frameworks are out of scope of this series.

Someone can work on these refactorings later on
(assuming someone is enough interested in it)

PS:
If you insist on fixing ram_size up, it would still be possible
to do so locally within board. One can take all RAM that user
provided and then create clamped memory region alias.
It's rather ugly but will do the job without affecting generic
code. (though I wouldn't volunteer to write up such a hack)


I think from the virtualization point of view, you don't want to
allocate unused resources. Is that why you say it is "ugly"?

 From the emulation point of view, it is easier to stay as close as
possible to the hardware, where sometimes resources are wasted to
simplify the design (or reduce production delays).
If it matches hardware, clamping doesn't look "ugly" to me.



gist of discussion in this case is that:

bamboo HW supports 4 banks of RAM that could be populated by
256/128/64/32/16/8Mb DIMMs


I care more about sam460ex but limitations are similar (with different 
list of supported DIMMs). This comes from how the SoC's memory controller 
stores memory size (that's similar to how SPD data encodes it) so we can't 
represent different size.



so -m XXX must be multiple of that, otherwise we are not
able distribute all -m specified size between supported banks.

Current patch then would error out with
 "Max 4 banks of 256, 128, 64, 32, 16, 8 MB DIMM/bank supported"
 "Possible valid RAM size: XXX"
where XXX is clamped value original code uses to after fixup

Argument was that it's not convenient for users do what they
would do on real hw (i.e. calculate desired size manually
based on available banks/DIMMs).


On real hardware one cannot have invalid RAM size because you can only 
plug DIMMs of above sizes so the problem does not happen. The argument was 
more that it worked before and now users may find their previously working 
options to error out. Current version of this patch is better as it gives 
valid option so the user who does not know about SoC limitations can just 
use the printed value and doesn't have to calculate valid value.



I gave an attempt to dumping all possible sizes and it results
in not practically useful unique ~80 entries list, so that idea
also goes down the drain.

I agree that it's not convenient, but I think that human user
can easily adapt to error message and either fix CLI to use
suggested value (i.e. what they are getting now) or pick desired
DIMMs number/size and do the math.


This version is acceptable and I can live with it. If every other board is 
happy with returning error then it's OK to do the same here as long as 
it's easy for users to fix their command line by using the printed value.


Regards,
BALATON Zoltan

qemu-x86_64, buster /sbin/ldconfig and setup_arg_pages (a mind dump)

2020-01-17 Thread Alex Bennée



Hi Richard,

While I was attempting to test the new vsyscall patches for x86 I
discovered I couldn't debootstrap an x86_64 buster image on my ARM box.
After digging further into it I discovered it was because executing
/sbin/ldconfig crashes and aborts the bootstrap.

This is helpfully reproducible on my main development system which is
also running buster:

  ./x86_64-linux-user/qemu-x86_64 /sbin/ldconfig
  setup_arg_pages: 004e
  target_set_brk: new_brk=004dfdf8
  do_brk() -> 004e (!new_brk)
  do_brk(004e11c0) -> do_brk: allocating 8192 => 7fb2dace5000
  004e (mapped_addr != -1 or brk_page)
  qemu: uncaught target signal 11 (Segmentation fault) - core dumped
  fish: Job 2, “./x86_64-linux-user/qemu-x86_64…” terminated by signal SIGSEGV 
(Address boundary error)

The failure of the second do_brk during the early setup of the binaries
TLS data area. However for some reason this isn't always the case. For
example with testthread which also uses TLS:

  ./x86_64-linux-user/qemu-x86_64 ./tests/tcg/x86_64-linux-user/testthread
  setup_arg_pages: 0040
  target_set_brk: new_brk=004c8558
  do_brk() -> 004c9000 (!new_brk)
  do_brk(004ca1c0) -> do_brk: allocating 8192 => 004c9000
  004ca1c0 (mapped_addr == brk_page)
  do_brk(004eb1c0) -> do_brk: allocating 135168 => 004cb000
  004eb1c0 (mapped_addr == brk_page)
  do_brk(004ec000) -> 004ec000 (new_brk <= brk_page)
  thread1: 0 hello1
  thread2: 0 hello2
  thread1: 1 hello1

Ultimately the failure is down to setup_arg_pages allocating too low in
the address space in the ldconfig case which leaves the second brk
unable to example it's region of memory. Turning on -d page and you can
see the region forming:

  page layout changed following target_mmap
  startend  size prot
  0040-00409000 9000 r--
  00409000-004ae000 000a5000 r-x
  004ae000-004d8000 0002a000 r--
  004d8000-004df000 7000 rw-
  004df000-004e 1000 ---
  page layout changed following target_mmap
  startend  size prot
  0040-00409000 9000 r--
  00409000-004ae000 000a5000 r-x
  004ae000-004d8000 0002a000 r--
  004d8000-0040008e1000 00809000 rw-
  setup_arg_pages: 004e
  guest_base  0x0
  page layout changed following binary load
  startend  size prot
  0040-00409000 9000 r--
  00409000-004ae000 000a5000 r-x
  004ae000-004d8000 0002a000 r--
  004d8000-004e 8000 rw-
  004e-004e1000 1000 ---
  004e1000-0040008e1000 0080 rw-
  start_brk   0x
  end_code0x004ad971
  start_code  0x00409000
  start_data  0x004d8778
  end_data0x004de510
  start_stack 0x0040008e02d0
  brk 0x004dfdf8
  entry   0x0040a370
  argv_start  0x0040008e02d8
  env_start   0x0040008e02e8
  auxv_start  0x0040008e0428
  target_set_brk: new_brk=004dfdf8
  page layout changed following target_mmap
  startend  size prot
  0040-00409000 9000 r--
  00409000-004ae000 000a5000 r-x
  004ae000-004d8000 0002a000 r--
  004d8000-004e 8000 rw-
  004e-004e1000 1000 ---
  004e1000-0040008e2000 00801000 rw-

So it looks like setup_arg_pages just creates a segment right in the
middle of a previously allocated block of storage. This is odd because
the loader basically just leaves it to mmap to pick a region:

error = target_mmap(0, size + guard, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);

AFAICT this just depends on where we have allocated last, in the
testthread case we already have a high mapping to splat:

  page layout changed following target_mmap
  startend  size prot
  0040-00401000 1000 r--
  00401000-00495000 00094000 r-x
  00495000-004bc000 00027000 r--
  004bd000-004c9000 c000 rw-
  0040-004000801000 00801000 rw-
  setup_arg_pages: 0040
  guest_base  0x0
  page layout changed following binary load
  startend  size prot
  0040-00401000 1000 r--

Proposal for handling .hx files with Sphinx

Currently our manual creation includes some .texi files which
are autogenerated from .hx files by running scripts/hxtool.
.hx files are a simple format, where where a line is either a
directive or literal text to be output:

HXCOMM
 -- comment lines, ignored
STEXI/ETEXI
 -- mark start/end of chunks of text to put into the texinfo output only
DEFHEADING/ARCHHEADING
 -- appear in the .h file output verbatim (they are defined as C macros);
for texi output they are parsed to add in header sections

For Sphinx, rather than creating a file to include, the most
natural way to handle this is to have a small custom Sphinx
extension which will read the .hx file and process it. So
instead of "makefile produces foo.texi from foo.hx, qemu-doc.texi
says '@include foo.texi'", we have "qemu-doc.rst says
'hxtool-doc:: foo.hx', the Sphinx extension for hxtool has
code that runs to handle that Sphinx directive, it reads the .hx
file and emits the appropriate documentation contents". (This is
pretty much the same way the kerneldoc extension works right now.
It also has the advantage that it should work for third-party
services like readthedocs that expect to build the docs directly
with sphinx rather than by invoking our makefiles.)

We'll need to update what the markup is to handle having rST
fragments in it. A very minimalist approach to this would
simply define a new pair of SRST/ERST directives marking the
start/end of chunks of rST text to go into the rST only.
(We might be able to do better than that later, as there's
some repetition currently going on. But we'll probably get
a better idea of how easy it is to avoid the repetition if
we start with a simple conversion.)

Here's what we do with hx files at the moment. We have four:

 hmp-commands.hx
   -- defines monitor commands used by monitor.c; generates
  qemu-monitor.texi, used by qemu-doc.texi
 hmp-commands-info.hx
   -- ditto, for the "info" command's subcommand;
  generates qemu-monitor-info.texi, used by qemu-doc.texi

These two use only the "put this in the texi or in the .h file"
functionality, alternating "raw C code defining an entry for the
monitor command array" with "lump of raw texi for the docs".

 qemu-img-cmds.hx
   -- defines options for qemu-img, used by qemu-img.texi

This uses the STEXI/ETEXI directives to alternate C and texi.
In the for-the-h-file section the only content is always a DEF()
macro invocation defining the option; in the texi is only the
synopsis of the command. This means there's a lot of repetition,
as the DEF macro includes an argument giving the text of the
option synopsis, and then the texi also has that synopsis with
some extra markup. Finally the main qemu-img.texi repeats the
marked-up synopsis later on when it has the actual main documentation
of each command.

 qemu-options.hx
   -- options for qemu proper, used by qemu-doc.texi

This uses only the DEF, DEFHEADING, ARCHHEADING macros
in the for-the-h-file sections (and the DEFHEADING/ARCHHEADING
are read also for texi generation). This also repeats the
synopsis in the DEF macro and in the texi fragment.

So I think my current view is that we should do the very
simple "add SRST/ERST directives" to start with:
 * scripts/hxtool needs to recognize them and just ignore
   the text inside them
 * write the hxtool sphinx extension (shouldn't be too hard)
 * conversion of any particular .hx file then involves
   replacing the STEXI ...texi stuff... ETEXI sections with
   SRST ...rst stuff... ERST. There's no need for any
   particular .hx file to support both texi and rst output
   at the same time

I would initially start with qemu-img-cmds.hx, since that's
pulled in by qemu-img.texi, which we can convert in the
same way I've been doing qemu-nbd and other standalone-ish
manpages. The others are part of the big fat qemu-doc.texi,
which is probably going to be the very last thing we convert...

thanks
-- PMM

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

On 1/17/20 6:13 AM, Philippe Mathieu-Daudé wrote:
>>> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
>>> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
> 
> 
> Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.
> 
> Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?

The "UL" part is also being added by a->size.  This code was written this way
in preparation for the 64-bit ldc, and the bug was not noticable because we
don't have that yet.

r~

Re: [PULL 0/4] qtests and gitlab-CI

On Fri, 17 Jan 2020 at 11:08, Thomas Huth  wrote:
>
>  Hi Peter,
>
> the following changes since commit fee0ec1fd11a6fb960517e18201ed8a686a0d7e8:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/block-pull-request' 
> into staging (2020-01-14 14:11:34 +)
>
> are available in the Git repository at:
>
>   https://gitlab.com/huth/qemu.git tags/pull-request-2020-01-17
>
> for you to fetch changes up to c82b95489f8c33318101d18e643f7b50259d8e4e:
>
>   gitlab-ci.yml: Run tcg test with tci (2020-01-16 17:01:39 +0100)
>
> 
> * Various fixes for qtests
> * Enable TCG tests with TCI in the gitlab CI
> 
>
> Laurent Vivier (1):
>   migration-test: ppc64: fix FORTH test program
>
> Shameer Kolothum (1):
>   tests: acpi: update path in rebuild-expected-aml
>
> Thomas Huth (2):
>   tests/qtest/vhost-user-test: Fix memory leaks
>   gitlab-ci.yml: Run tcg test with tci


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.0
for any user-visible changes.

-- PMM

Re: [RFC v4 PATCH 00/49] Initial support of multi-process qemu - status update

* John G Johnson (john.g.john...@oracle.com) wrote:



> DMA
> 
>   This is one place where I might diverge from VFIO.  It uses an
> ioctl to tell the kernel driver what areas of guest memory the device
> can address.  The driver then pins that memory so it can be programmed
> into a HW IOMMU.  We could avoid pinning of guest memory by adopting
> the vhost-user idea of sending the file descriptors used by QEMU to
> create guest memory to the emulation process, and having it mmap() the
> guest itself.  IOMMUs are handled by having the emulation process
> request device DMA to guest PA translations from QEMU.

The interface in vhost-user to pass these memory fd's is a bit hairy;
so it would be great if there was something better for multi-process.

Some things to think about:
  a) vhost-user filters it so that areas of memory not backed by an fd
aren't passed to the client; this filters out some of the device
specific RAM blocks that aren't really normal RAM.

  b) Hugepages are tricky; especially on a PC where the 0-1MB area is
broken up into chunks and you're trying to mmap 2MB chunks into the
client.

  c) Postcopy with vhost-user was pretty tricky as well; there needs
to be some coordination with the qemu to handle pages that are missing.

  d) Some RAM mappings can change; mostly not the ones sent to the
client; but just watch out that these can happen at unexpected times.

Dave


> 
> 
> > If implementations can use the kernel uapi vfio header files then we're
> > on track for compatibility with VFIO.
> > 
> >>> This is just a more elaborate explanation for the "the cat is out of the
> >>> bag" comments that have already been made on licensing.  Does anyone
> >>> still disagree or want to discuss further?
> >>> 
> >>> If there is agreement that a stable API is okay then I think the
> >>> practical way to do this is to first merge a cleaned-up version of
> >>> multi-process QEMU as an unstable experimental API.  Once it's being
> >>> tested and used we can write a protocol specification and publish it as
> >>> a stable interface when the spec has addressed most use cases.
> >>> 
> >>> Does this sound good?
> >> 
> >> In that case, wouldn't it be preferable to revive our proposal from
> >> Edinburgh (KVM Forum 2018)? Our prototypes moved more of the Qemu VFIO
> >> code to "common" and added a "user" backend underneath it, similar to
> >> how vhost-user-scsi moved some of vhost-scsi to vhost-scsi-common and
> >> added vhost-user-scsi. It was centric on PCI, but it doesn't have to
> >> be. The other side can be implemented in libmuser for facilitating things.
> > 
> > That sounds good.
> > 
> 
>The emulation program API could be based on the current
> libmuser API or the libvfio-user API.  The protocol itself wouldn’t
> care which is chosen.  Our multi-processQEMU project would have to
> change how devices are specified from the QEMU command line to the
> emulation process command line.
> 
>   JJ
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

On Fri, 17 Jan 2020 at 16:46, Guenter Roeck  wrote:
>
> On Fri, Jan 17, 2020 at 01:23:46PM +, Peter Maydell wrote:
> > Won't this now do all the work of constructing the hexdump strings,
> > even if tracing is disabled ?
> >
> That is correct. Can I check
> if (trace_event_get_state(TRACE_PL330_HEXDUMP) &&
> qemu_loglevel_mask(LOG_TRACE)) {
> directly in pl330_hexdump(), or is there some other means to handle
> this kind of situation ?

It's not something I've had to do before.
docs/devel/tracing.txt says "just use the TRACE_FOO_ENABLED
macro", but looking at what it does that doesn't seem to check
the runtime state of the trace event, so maybe those docs are out
of date. Stefan, what's the current best way to guard expensive
computations used only for trace output ?

thanks
-- PMM

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4

2020-01-17 Thread Helge Deller

On 17.01.20 17:13, Philippe Mathieu-Daudé wrote:
> On 1/17/20 4:49 PM, Helge Deller wrote:
>> On 17.01.20 02:53, Richard Henderson wrote:
>>> The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
>>> or the operation is undefined.  However, real hardware only generates
>>> an unaligned access trap for unaligned mod 4.
>>>
>>> Match real hardware, but diagnose with GUEST_ERROR a violation of the
>>> specification.
>>>
>>> Reported-by: Helge Deller 
>>> Suggested-by: John David Anglin 
>>> Signed-off-by: Richard Henderson 
>>>
>>> Helge, can you please test this against your failing kernel?
>>> You will of course want to add -D logfile -d guest_errors to
>>> you qemu command-line.
>>
>> Yes, works as expected.
>> Thanks!
>>
>> Please add:
>> Tested-by: Helge Deller 
>>
>>
>> [deller]$ tail -f logfile
>> Undefined ldc to address unaligned mod 16: 0504fa6c7848
>> Undefined ldc to address unaligned mod 16: 0504fa6c7a48
>> Undefined ldc to address unaligned mod 16: 0506f9434848
>> Undefined ldc to address unaligned mod 16: 0506f9434a48
>> Undefined ldc to address unaligned mod 16: 0508fa036848
>> Undefined ldc to address unaligned mod 16: 0508fa036a48
>> Undefined ldc to address unaligned mod 16: 050afa8c4848
>> Undefined ldc to address unaligned mod 16: 050afa8c4a48
>> Undefined ldc to address unaligned mod 16: 050cf94d1848
>> Undefined ldc to address unaligned mod 16: 050cf94d1a48
>> 
>>
>>
>>
>>>
>>>
>>> r~
>>>
>>> ---
>>>   target/hppa/helper.h    | 2 ++
>>>   target/hppa/op_helper.c | 9 +
>>>   target/hppa/translate.c | 6 +-
>>>   3 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/target/hppa/helper.h b/target/hppa/helper.h
>>> index 38d834ef6b..2d483aab58 100644
>>> --- a/target/hppa/helper.h
>>> +++ b/target/hppa/helper.h
>>> @@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, 
>>> env, tl, tr)
>>>   DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
>>>   DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)
>>>
>>> +DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
>>> +
>>>   DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)
>>>
>>>   DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
>>> diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
>>> index f0516e81f1..345cef2c08 100644
>>> --- a/target/hppa/op_helper.c
>>> +++ b/target/hppa/op_helper.c
>>> @@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, 
>>> target_ulong addr,
>>>   do_stby_e(env, addr, val, true, GETPC());
>>>   }
>>>
>>> +void HELPER(ldc_check)(target_ulong addr)
>>> +{
>>> +    if (unlikely(addr & 0xf)) {
>>> +    qemu_log_mask(LOG_GUEST_ERROR,
>>> +  "Undefined ldc to address unaligned mod 16: "
>
> "to unaligned address mod 16"?
>
>>> +  TARGET_FMT_lx "\n", addr);
>>> +    }
>>> +}
>>> +
>>>   target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
>>>     uint32_t level, uint32_t want)
>>>   {
>>> diff --git a/target/hppa/translate.c b/target/hppa/translate.c
>>> index 2f8d407a82..669381dc1d 100644
>>> --- a/target/hppa/translate.c
>>> +++ b/target/hppa/translate.c
>>> @@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)
>>>
>>>   static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>>>   {
>>> -    MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
>>> +    MemOp mop = MO_TE | MO_ALIGN | a->size;
>
>
> Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.
>
> Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?

>>>   TCGv_reg zero, dest, ofs;
>>>   TCGv_tl addr;
>>>
>>> @@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
>>>
>>>   form_gva(ctx, , , a->b, a->x, a->scale ? a->size : 0,
>>>    a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
>>> +
>>> +    gen_helper_ldc_check(addr);

Actually, for 64-bit the address is allowed to be 4-byte aligned, as long as the
"co" completer is given, e.g. this would be OK:  "ldcw,co  0(addr),%reg".
Maybe adding something like (if TARGET_32BIT...) now would make sense, so we 
don't get it
wrong when 64bit gets added?

Helge



>>>   zero = tcg_const_reg(0);
>>>   tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
>>> +    tcg_temp_free(zero);
>>> +
>>>   if (a->m) {
>>>   save_gpr(ctx, a->b, ofs);
>>>   }
>>>
>>
>>
>

[PATCH v3 2/2] hw/timer/m48t59: Convert debug printf()s to trace events

Signed-off-by: Philippe Mathieu-Daudé 
---
 hw/rtc/m48t59-internal.h |  5 -
 hw/rtc/m48t59.c  | 11 +--
 hw/rtc/trace-events  |  6 ++
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/hw/rtc/m48t59-internal.h b/hw/rtc/m48t59-internal.h
index 4d4f2a6fed..cd648241e9 100644
--- a/hw/rtc/m48t59-internal.h
+++ b/hw/rtc/m48t59-internal.h
@@ -26,11 +26,6 @@
 #ifndef HW_M48T59_INTERNAL_H
 #define HW_M48T59_INTERNAL_H
 
-#define M48T59_DEBUG 0
-
-#define NVRAM_PRINTF(fmt, ...) do { \
-if (M48T59_DEBUG) { printf(fmt , ## __VA_ARGS__); } } while (0)
-
 /*
  * The M48T02, M48T08 and M48T59 chips are very similar. The newer '59 has
  * alarm and a watchdog timer and related control registers. In the
diff --git a/hw/rtc/m48t59.c b/hw/rtc/m48t59.c
index fc592b9fb1..ecc92ca476 100644
--- a/hw/rtc/m48t59.c
+++ b/hw/rtc/m48t59.c
@@ -35,6 +35,7 @@
 #include "exec/address-spaces.h"
 #include "qemu/bcd.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 #include "m48t59-internal.h"
 #include "migration/vmstate.h"
@@ -192,8 +193,7 @@ void m48t59_write(M48t59State *NVRAM, uint32_t addr, 
uint32_t val)
 struct tm tm;
 int tmp;
 
-if (addr > 0x1FF8 && addr < 0x2000)
-   NVRAM_PRINTF("%s: 0x%08x => 0x%08x\n", __func__, addr, val);
+trace_m48txx_nvram_mem_write(addr, val);
 
 /* check for NVRAM access */
 if ((NVRAM->model == 2 && addr < 0x7f8) ||
@@ -450,8 +450,7 @@ uint32_t m48t59_read(M48t59State *NVRAM, uint32_t addr)
}
 break;
 }
-if (addr > 0x1FF9 && addr < 0x2000)
-   NVRAM_PRINTF("%s: 0x%08x <= 0x%08x\n", __func__, addr, retval);
+trace_m48txx_nvram_mem_read(addr, retval);
 
 return retval;
 }
@@ -462,7 +461,7 @@ static void NVRAM_writeb(void *opaque, hwaddr addr, 
uint64_t val,
 {
 M48t59State *NVRAM = opaque;
 
-NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" => 0x%"PRIx64"\n", __func__, addr, val);
+trace_m48txx_nvram_io_write(addr, val);
 switch (addr) {
 case 0:
 NVRAM->addr &= ~0x00FF;
@@ -494,7 +493,7 @@ static uint64_t NVRAM_readb(void *opaque, hwaddr addr, 
unsigned size)
 retval = -1;
 break;
 }
-NVRAM_PRINTF("%s: 0x%"HWADDR_PRIx" <= 0x%08x\n", __func__, addr, retval);
+trace_m48txx_nvram_io_read(addr, retval);
 
 return retval;
 }
diff --git a/hw/rtc/trace-events b/hw/rtc/trace-events
index d6749f4616..52c1566198 100644
--- a/hw/rtc/trace-events
+++ b/hw/rtc/trace-events
@@ -17,3 +17,9 @@ pl031_set_alarm(uint32_t ticks) "alarm set for %u ticks"
 # aspeed-rtc.c
 aspeed_rtc_read(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 
0x%08" PRIx64
 aspeed_rtc_write(uint64_t addr, uint64_t value) "addr 0x%02" PRIx64 " value 
0x%08" PRIx64
+
+# m48t59.c
+m48txx_nvram_io_read(uint64_t addr, uint64_t value) "io read addr:0x%04" 
PRIx64 " value:0x%02" PRIx64
+m48txx_nvram_io_write(uint64_t addr, uint64_t value) "io write addr:0x%04" 
PRIx64 " value:0x%02" PRIx64
+m48txx_nvram_mem_read(uint32_t addr, uint32_t value) "mem read addr:0x%04x 
value:0x%02x"
+m48txx_nvram_mem_write(uint32_t addr, uint32_t value) "mem write addr:0x%04x 
value:0x%02x"
-- 
2.21.1

[PATCH v3 1/2] MAINTAINERS: Add missing m48t59 files to the PReP section

Signed-off-by: Philippe Mathieu-Daudé 
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 4b425d030d..c162145bd0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1084,6 +1084,8 @@ F: hw/pci-host/prep.[hc]
 F: hw/isa/i82378.c
 F: hw/isa/pc87312.c
 F: hw/dma/i82374.c
+F: hw/rtc/m48t59.c
+F: hw/rtc/m48t59-internal.h
 F: hw/rtc/m48t59-isa.c
 F: include/hw/isa/pc87312.h
 F: include/hw/rtc/m48t59.h
-- 
2.21.1

[PATCH v3 0/2] hw/timer/m48t59: Convert to trace events

Another trivial cleanup series.

v2: Rebased on 4.2

Philippe Mathieu-Daudé (2):
  MAINTAINERS: Add missing m48t59 files to the PReP section
  hw/timer/m48t59: Convert debug printf()s to trace events

 hw/rtc/m48t59-internal.h |  5 -
 hw/rtc/m48t59.c  | 11 +--
 MAINTAINERS  |  2 ++
 hw/rtc/trace-events  |  6 ++
 4 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.21.1

Re: [RFC PATCH] pcie: Defer hot unplug until migration is complete

* Julia Suvorova (jus...@redhat.com) wrote:
> On Mon, Jan 13, 2020 at 3:04 PM Yury Kotov  wrote:
> >
> > Devices hot-plug during migration is not allowed and disabled in
> > corresponding QMP-commands (device_add, device_del).
> > But guest still can unplug device by powering it off
> > (Example: echo 0 > /sys/bus/pci/slots/XXX/power).
> 
> You don't want to unplug device due to powering the slot off in the
> first place. Instead, you can hide the device (see f3a8505656), and
> make it visible again when the slot is powered on. Thus, the guest
> will not be able to unplug the device and your problem will disappear.
> 
> Best regards, Julia Suvorova.

I don't really understand how hidden a hidden device is;
will it still get it's migration data migrated?
  - if so what state is it in? What gets migrated?
is the behaviour after reset/reboot consistent with what we want
in the existing unplug behaviour?
Say it's a disk device; will it release the lock on the disk backend?

It feels like the semantics need to be tied down a bit.

Dave

> > Fix it by deferring unplugging until the migration is complete.
> >
> > Signed-off-by: Yury Kotov 
> > ---
> >  hw/pci-bridge/gen_pcie_root_port.c |  7 
> >  hw/pci-bridge/ioh3420.c|  7 
> >  hw/pci-bridge/xio3130_downstream.c |  7 
> >  hw/pci/pcie.c  | 54 +++---
> >  hw/pci/pcie_port.c | 47 ++
> >  include/hw/pci/pcie.h  |  1 +
> >  include/hw/pci/pcie_port.h | 20 +++
> >  7 files changed, 130 insertions(+), 13 deletions(-)
> >
> > diff --git a/hw/pci-bridge/gen_pcie_root_port.c 
> > b/hw/pci-bridge/gen_pcie_root_port.c
> > index 9eaefebca8..5b3c202341 100644
> > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > @@ -100,6 +100,9 @@ static void gen_rp_realize(DeviceState *dev, Error 
> > **errp)
> >  }
> >  }
> >
> > +static const VMStateDescription vmstate_rp_deffered_unplug =
> > +VMSTATE_DEFFERED_UNPLUG("pcie-root-port");
> > +
> >  static const VMStateDescription vmstate_rp_dev = {
> >  .name = "pcie-root-port",
> >  .priority = MIG_PRI_PCI_BUS,
> > @@ -114,6 +117,10 @@ static const VMStateDescription vmstate_rp_dev = {
> >GenPCIERootPort,
> >gen_rp_test_migrate_msix),
> >  VMSTATE_END_OF_LIST()
> > +},
> > +.subsections = (const VMStateDescription * []) {
> > +_rp_deffered_unplug,
> > +NULL
> >  }
> >  };
> >
> > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> > index f1e16135a3..2399a9a87f 100644
> > --- a/hw/pci-bridge/ioh3420.c
> > +++ b/hw/pci-bridge/ioh3420.c
> > @@ -82,6 +82,9 @@ static void ioh3420_interrupts_uninit(PCIDevice *d)
> >  msi_uninit(d);
> >  }
> >
> > +static const VMStateDescription vmstate_ioh3420_deffered_unplug =
> > +VMSTATE_DEFFERED_UNPLUG("ioh-3240-express-root-port");
> > +
> >  static const VMStateDescription vmstate_ioh3420 = {
> >  .name = "ioh-3240-express-root-port",
> >  .priority = MIG_PRI_PCI_BUS,
> > @@ -93,6 +96,10 @@ static const VMStateDescription vmstate_ioh3420 = {
> >  VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log,
> > PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog),
> >  VMSTATE_END_OF_LIST()
> > +},
> > +.subsections = (const VMStateDescription * []) {
> > +_ioh3420_deffered_unplug,
> > +NULL
> >  }
> >  };
> >
> > diff --git a/hw/pci-bridge/xio3130_downstream.c 
> > b/hw/pci-bridge/xio3130_downstream.c
> > index a9f084b863..a5b4fe08ee 100644
> > --- a/hw/pci-bridge/xio3130_downstream.c
> > +++ b/hw/pci-bridge/xio3130_downstream.c
> > @@ -139,6 +139,9 @@ static Property xio3130_downstream_props[] = {
> >  DEFINE_PROP_END_OF_LIST()
> >  };
> >
> > +static const VMStateDescription vmstate_xio3130_downstream_deffered_unplug 
> > =
> > +VMSTATE_DEFFERED_UNPLUG("xio3130-express-downstream-port");
> > +
> >  static const VMStateDescription vmstate_xio3130_downstream = {
> >  .name = "xio3130-express-downstream-port",
> >  .priority = MIG_PRI_PCI_BUS,
> > @@ -150,6 +153,10 @@ static const VMStateDescription 
> > vmstate_xio3130_downstream = {
> >  VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log,
> > PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog),
> >  VMSTATE_END_OF_LIST()
> > +},
> > +.subsections = (const VMStateDescription * []) {
> > +_xio3130_downstream_deffered_unplug,
> > +NULL
> >  }
> >  };
> >
> > diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> > index 08718188bb..29f0e5c05b 100644
> > --- a/hw/pci/pcie.c
> > +++ b/hw/pci/pcie.c
> > @@ -28,6 +28,8 @@
> >  #include "hw/pci/pcie_regs.h"
> >  #include "hw/pci/pcie_port.h"
> >  #include "qemu/range.h"
> > +#include "sysemu/sysemu.h"
> > +#include "migration/misc.h"
> >
> >  //#define

Re: [PATCH v2 0/6] Fix more GCC9 -O3 warnings


Cc'ing qemu-trivial as this series is fully reviewed.

On 12/18/19 8:25 PM, Philippe Mathieu-Daudé wrote:

Fix some trivial warnings when building with -O3.

v2:
- addressed Thomas and Aleksandar comments
- dropped 'hw/scsi/megasas: Silent GCC9 duplicated-cond warning'
- dropped 'qemu-io-cmds: Silent GCC9 format-overflow warning'

Philippe Mathieu-Daudé (6):
   audio/audio: Add missing fall through comment
   hw/display/tcx: Add missing fall through comments
   hw/timer/aspeed_timer: Add a fall through comment
   hw/net/imx_fec: Rewrite fall through comments
   hw/net/imx_fec: Remove unuseful FALLTHROUGH comments
   hw/pci-host/designware: Remove unuseful FALLTHROUGH comment

  audio/audio.c| 1 +
  hw/display/tcx.c | 2 ++
  hw/net/imx_fec.c | 7 ---
  hw/pci-host/designware.c | 2 +-
  hw/timer/aspeed_timer.c  | 2 +-
  5 files changed, 9 insertions(+), 5 deletions(-)

Cc: Aleksandar Markovic 
Cc: Andrew Jeffery 
Cc: Andrey Smirnov 
Cc: "Cédric Le Goater" 
Cc: Gerd Hoffmann 
Cc: Jason Wang 
Cc: Joel Stanley 
Cc: "Kővágó, Zoltán" 
Cc: Mark Cave-Ayland 
Cc: Markus Armbruster 
Cc: Olivier Danet 
Cc: Peter Chubb 
Cc: Peter Maydell 
Cc: qemu-...@nongnu.org

Re: [PATCH] mailmap: Add more entries to sanitize 'git log' output

ping?

On Wed, Dec 18, 2019 at 7:57 PM Philippe Mathieu-Daudé
 wrote:
>
> Most of these developers have the Signed-off-by tag properly
> written, but not the author/commiter name. Fix this.
> Also we incorrectly wrote Arei Gonglei name, update and reorder.
>
> git-log does not use this file by default until you specify the
> --use-mailmap flag:
>
>   $ git log --use-mailmap
>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  .mailmap | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)
>
> diff --git a/.mailmap b/.mailmap
> index 3816e4effe..1a859d9e65 100644
> --- a/.mailmap
> +++ b/.mailmap
> @@ -56,6 +56,10 @@ Aaron Lindsay 
>  Alexey Gerasimenko 
>  Alex Ivanov 
>  Andreas Färber 
> +Andreas Färber 
> +Andreas Färber  
> +Arei Gonglei 
> +Arei Gonglei  
>  Bandan Das 
>  Benjamin MARSILI 
>  Benoît Canet 
> @@ -67,27 +71,36 @@ Brad Smith 
>  Brijesh Singh 
>  Brilly Wu 
>  Cédric Vincent 
> +Chai Wen 
> +Chaojian Hu 
>  CheneyLin 
> +Chen Fan 
>  Chen Gang 
>  Chen Gang 
>  Chen Gang 
>  Chen Wei-Ren 
>  Christophe Lyon 
> +Christoph Hellwig 
>  Collin L. Walling 
>  Daniel P. Berrangé 
> +Disheng Su 
> +Dunrong Huang 
>  Eduardo Otubo 
>  Fabrice Desclaux 
>  Fernando Luis Vázquez Cao 
>  Fernando Luis Vázquez Cao 
>  Gautham R. Shenoy 
>  Gautham R. Shenoy 
> -Gonglei (Arei) 
>  Guang Wang 
>  Hailiang Zhang 
>  Hervé Poussineau 
> +Hiroyuki Obinata 
>  Jakub Jermář 
>  Jakub Jermář 
> +Jay Zhou 
>  Jean-Christophe Dubois 
> +Jean-Christophe Dubois  
> +Jia Lina 
>  Jindřich Makovička 
>  John Arbuckle 
>  Juha Riihimäki 
> @@ -96,9 +109,11 @@ Jun Li 
>  Laurent Vivier 
>  Leandro Lupori 
>  Li Guang 
> +Lili Huang 
>  Liming Wang 
>  linzhecheng 
>  Liran Schour 
> +Li Tianqing 
>  Liu Yu 
>  Liu Yu 
>  Li Zhang 
> @@ -112,11 +127,14 @@ Marc Marí 
>  Marc Marí 
>  Michael Avdienko 
>  Michael S. Tsirkin 
> +Michael S. Tsirkin  
> +Michael Tokarev  
>  Munkyu Im 
>  Nicholas Bellinger 
>  Nicholas Thomas 
>  Nikunj A Dadhania 
>  Orit Wasserman 
> +Pan Nengyuan 
>  Paolo Bonzini 
>  Pavel Dovgaluk 
>  Pavel Dovgaluk 
> @@ -124,15 +142,23 @@ Pavel Dovgaluk 
>  Peter Crosthwaite 
>  Peter Crosthwaite 
>  Peter Crosthwaite 
> +Peter Maydell  
>  Prasad J Pandit 
>  Prasad J Pandit 
>  Qiao Nuohan 
>  Reimar Döffinger 
>  Remy Noel 
> +Richard Henderson  
>  Roger Pau Monné 
>  Shin'ichiro Kawasaki 
>  Shin'ichiro Kawasaki 
> +Siwei Zhuang 
> +Sochin Jiang 
>  Sochin Jiang 
> +Stefan Berger   stef...@linux.vnet.ibm.com>
> +Stefan Weil  
> +Stefan Weil  
> +Stefan Weil  
>  Takashi Yoshii 
>  Thomas Huth 
>  Thomas Knych 
> @@ -150,6 +176,7 @@ Wenshuang Ma 
>  Xiaoqiang Zhao 
>  Xinhua Cao 
>  Xiong Zhang 
> +Ying Fang 
>  Yin Yin 
>  yuchenlin 
>  YunQiang Su 
> --
> 2.21.0
>

Re: [PATCH v2 00/86] refactor main RAM allocation to use hostmem backend

On 1/17/20 5:40 PM, Igor Mammedov wrote:

On Fri, 17 Jan 2020 17:19:26 +0100
Philippe Mathieu-Daudé wrote:

On 1/17/20 5:03 PM, Igor Mammedov wrote:

On Thu, 16 Jan 2020 16:43:07 +0100
Philippe Mathieu-Daudé wrote:

On 1/15/20 10:01 PM, no-re...@patchew.org wrote:

Patchew URL:
https://patchew.org/QEMU/1579100861-73692-1-git-send-email-imamm...@redhat.com/

Hi,

This series failed the docker-quick@centos7 build test. Please find the testing
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1

it doesn't work on my host, since it tries to use /tmp for building an image
and fails with no space.
is there any way to point it to some other place?

What is failing? Building the docker image, or building QEMU withing the
docker container?

docker image

Odd. I thought image was built in default storage dir: /var/lib/docker

# docker system info | fgrep /
Docker Root Dir: /var/lib/docker

The default might be distrib dependent, my workstation is on Fedora.

Maybe while building a tmpfs is mount as /tmp within the temp container,
and your ram is limited so it gets full while downloading packages to
install? Sincerely I have no clue. Docker is not your friend :/

Here they say you can change the docker dir with '-g'
https://forums.docker.com/t/how-do-i-change-the-docker-image-installation-directory/1169/4

Can you paste the error context?

Re: [PULL 0/4] qtests and gitlab-CI

2020-01-17 Thread no-reply

Patchew URL: https://patchew.org/QEMU/20200117110758.1995-1-th...@redhat.com/



Hi,

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

Message-id: 20200117110758.1995-1-th...@redhat.com
Type: series
Subject: [PULL 0/4] qtests and gitlab-CI

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

From https://github.com/patchew-project/qemu
 * [new tag] patchew/20200117110758.1995-1-th...@redhat.com -> 
patchew/20200117110758.1995-1-th...@redhat.com
Switched to a new branch 'test'
7fbbaf7 gitlab-ci.yml: Run tcg test with tci
1c4d104 tests/qtest/vhost-user-test: Fix memory leaks
2e1728c migration-test: ppc64: fix FORTH test program
247a6a0 tests: acpi: update path in rebuild-expected-aml

=== OUTPUT BEGIN ===
1/4 Checking commit 247a6a033937 (tests: acpi: update path in 
rebuild-expected-aml)
ERROR: line over 90 characters
#44: FILE: tests/data/acpi/rebuild-expected-aml.sh:34:
+echo '/* List of comma-separated changed AML files to ignore */' > 
${SRC_PATH}/tests/qtest/bios-tables-test-allowed-diff.h

total: 1 errors, 0 warnings, 21 lines checked

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

2/4 Checking commit 2e1728c34f2b (migration-test: ppc64: fix FORTH test program)
3/4 Checking commit 1c4d10497e69 (tests/qtest/vhost-user-test: Fix memory leaks)
4/4 Checking commit 7fbbaf7a0df6 (gitlab-ci.yml: Run tcg test with tci)
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH v3 02/11] 9pfs: require msize >= 4096

2020-01-17 Thread Christian Schoenebeck

On Freitag, 17. Januar 2020 16:15:37 CET Greg Kurz wrote:
> > > Hmm... this patch does a sanity check on 'count', not on 'msize'...
> > 
> > Yes ... :)
> > 
> > > I mean no matter what msize is, clipping count to msize - 11 gives a
> > > chance to stop processing the entries before overflowing the transport
> > > buffer.
> > 
> > ... and no, this cannot happen if minimum msize of 4096 is forced already
> > by Tversion. Maybe you now get my point -> It is about avoiding exactly
> > such kind
> I'm not sure to see how setting a minimum msize of 4096 at Tversion would
> prevent the client to pass a higher 'count' argument and lure the server
> into generating a bigger than msize response since it does not check
> count < msize - 11 without patch 3.

That's correct, it requires patch 3 as well to prevent that. Without patch 3, 
if a (i.e. bad) client sends a 'count' parameter >> msize then the Treaddir 
request is processed by server to full extent according to 'count' and finally 
aborted by a transport error since server's response would exceed msize.

> > of issues in the first place. Most file systems have a name limit of 255
> > bytes:
> > 
> > https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
> > 
> > So by forcing a minimum 'msize' of 4096 you avoid having to deal with this
> > issue (and similar ones) on Treaddir request level (and other request type
> > handlers), including ReiserFS BTW because 4032+35 < 4096.
> 
> Good to know for ReiserFS.
> 
> > If you would allow smaller 'msize' values by Tversion, then you would need
> > to suffer some kind of death when handling Treaddir with certain high
> > file name length. Either a transport error (with an error message that a
> > normal user would not be able to understand at all) or by returning an
> > incomplete Treaddir response sequence with { Rreaddir count=0 }, or ...
> > any other kind of death.
> Ahh I now understand at last your argument about Rreaddir loosing data.
> We may end up sending { Rreaddir count=0 } because the next entry is too
> large... and thus end the readdir sequence.

Yep.

> Mentioning this explicitly
> from the start would have been more clear for me ;-)

Sorry for that. :) I thought I made it clear with the directory entries 
example. I try to be more clear next time.

> This looks like yet another bug to me. It looks wrong to return this
> special response if we have more entries to go. Also this could be the
> client's _fault_ if it provides a ridiculously small value for count.
> The current code will return count=0 all the same.
> 
> In any case, I think v9fs_do_readdir() should only return 0 if there
> are no more entries to read. It should error out otherwise, but I'm
> not sure how...

Patience please. I have to limit the scope of this patch series somewhere. I 
am aware about these issues, but if I add fixes for more and more edge cases 
(which already exist) as part of this patch series, it will become a never 
ending story.

I just added those particular fixes to this series, because they were directly 
related to things I've changed here for the actual purpose of this patch set, 
which was and is: readdir latency optimization.

> > > My point is that we're not going to check msize in Tversion in
> > > order to to avoid multiple checks everywhere. We're going to do
> > > it there because it is the only place where it makes sense to
> > > do it.
> > 
> > Also yes and no. Of course it just makes sense to handle it already at
> > Tversion. But no, you could theoretically also allow much smaller minimum
> > 'msize' value << 4096 (i.e. somewhere closely >7 as we discussed), then
> > you
> > would indeed still need to add msize checks at other places of the code
> > base as you just found out now. So forcing a minimum 'msize' which is
> > high enough, avoids having to add such individual checks and having to
> > deal with them in some kind of unpleasant way.
> 
> We still don't understand each other I'm afraid... we actually have
> implicit 'msize' checks already for every single thing we write on
> the wire: v9fs_packunpack() which detects when we're trying to write
> passed the buffer. When this happens, it is propagated to the transport
> which then disconnects, which is the painful thing you've been
> experiencing with your readdir experiments. In the case of Rreaddir, it
> really does make sense to try to avoid the disconnection like you do in
> patch 3 because the readdir sequence allows _partial_ reads. Same goes
> for Rread. But that's it. No other message in the protocol allows that,
> so I've never thought of adding individual 'msize' checks anywhere else.
> What would they do better than v9fs_packunpack() already does ?

Right, but you realized that a min. msize of 4096 (in combination with
patch 3) prevents the readdir data loss issue we discussed here (provided we 
have a "good" client sending count=msize-11), right?

If so, I suggest I "try" to address your concerns you came up with here in the

Re: [PATCH v2 00/86] refactor main RAM allocation to use hostmem backend

2020-01-17 Thread Igor Mammedov

On Fri, 17 Jan 2020 17:19:26 +0100
Philippe Mathieu-Daudé  wrote:

> On 1/17/20 5:03 PM, Igor Mammedov wrote:
> > On Thu, 16 Jan 2020 16:43:07 +0100
> > Philippe Mathieu-Daudé  wrote:
> >   
> >> On 1/15/20 10:01 PM, no-re...@patchew.org wrote:  
> >>> Patchew URL: 
> >>> https://patchew.org/QEMU/1579100861-73692-1-git-send-email-imamm...@redhat.com/
> >>>
> >>>
> >>>
> >>> Hi,
> >>>
> >>> This series failed the docker-quick@centos7 build test. Please find the 
> >>> testing commands and
> >>> their output below. If you have Docker installed, you can probably 
> >>> reproduce it
> >>> locally.
> >>>
> >>> === TEST SCRIPT BEGIN ===
> >>> #!/bin/bash
> >>> make docker-image-centos7 V=1 NETWORK=1
> >>> time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1  
> > 
> > it doesn't work on my host, since it tries to use /tmp for building an image
> > and fails with no space.
> > is there any way to point it to some other place?  
> 
> What is failing? Building the docker image, or building QEMU withing the 
> docker container?

docker image

Re: [PATCH 1/6] dma/pl330: Convert to support tracing

On Fri, Jan 17, 2020 at 01:23:46PM +, Peter Maydell wrote:
> On Fri, 10 Jan 2020 at 20:39, Guenter Roeck  wrote:
> >
> > Replace debug logging code with tracing.
> >
> > Signed-off-by: Guenter Roeck 
> > ---
> >  hw/dma/pl330.c  | 88 +++--
> >  hw/dma/trace-events | 24 +
> 
> > +static void pl330_hexdump(uint8_t *buf, size_t size)
> > +{
> > +unsigned int b, i, len;
> > +char tmpbuf[80];
> > +
> > +for (b = 0; b < size; b += 16) {
> > +len = size - b;
> > +if (len > 16) {
> > +len = 16;
> > +}
> > +tmpbuf[0] = '\0';
> > +for (i = 0; i < len; i++) {
> > +if ((i % 4) == 0) {
> > +strcat(tmpbuf, " ");
> > +}
> > +sprintf(tmpbuf + strlen(tmpbuf), " %02x", buf[b + i]);
> > +}
> > +trace_pl330_hexdump(b, tmpbuf);
> > +}
> > +}
> >
> 
> > @@ -1175,11 +1186,8 @@ static int pl330_exec_cycle(PL330Chan *channel)
> >  int len = q->len - (q->addr & (q->len - 1));
> >
> >  dma_memory_read(_space_memory, q->addr, buf, len);
> > -if (PL330_ERR_DEBUG > 1) {
> > -DB_PRINT("PL330 read from memory @%08" PRIx32 " (size = 
> > %08x):\n",
> > -  q->addr, len);
> > -qemu_hexdump((char *)buf, stderr, "", len);
> > -}
> > +trace_pl330_exec_cycle(q->addr, len);
> > +pl330_hexdump(buf, len);
> 
> Won't this now do all the work of constructing the hexdump strings,
> even if tracing is disabled ?
> 
That is correct. Can I check
if (trace_event_get_state(TRACE_PL330_HEXDUMP) &&
qemu_loglevel_mask(LOG_TRACE)) {
directly in pl330_hexdump(), or is there some other means to handle
this kind of situation ?

Thanks,
Guenter

Re: [PATCH v3 3/5] migration: Don't wait in semaphore for thread we know has finished

* Juan Quintela (quint...@redhat.com) wrote:
> If p->quit is true for any channel, we know that it has finished for
> any reason.  So don't wait for it, just continue.
> 
> Signed-off-by: Juan Quintela 
> 
> ---
> 
> I could be convinced that the right thing to do in that case is to
> just do a break instead of a continue.  Each option has its own
> advantages/disadvantanges.
> ---
>  migration/ram.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/migration/ram.c b/migration/ram.c
> index 44ca56e1ea..bc918ef28d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1118,6 +1118,12 @@ static void multifd_send_sync_main(RAMState *rs)
>  MultiFDSendParams *p = _send_state->params[i];
>  
>  trace_multifd_send_sync_main_wait(p->id);
> +qemu_mutex_lock(>mutex);
> +if (p->quit) {
> +qemu_mutex_unlock(>mutex);
> +continue;
> +}
> +qemu_mutex_unlock(>mutex);

Why is this needed/helps?
You can't depend on the p->quit happening before the 
sem_wait, so the main thread still has to do a post on sem_sync before
the join, even with the addition of the check for p->quit.

Dave

>  qemu_sem_wait(>sem_sync);
>  }
>  trace_multifd_send_sync_main(multifd_send_state->packet_num);
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v2 66/86] ppc/{ppc440_bamboo,sam460x}: drop RAM size fixup

2020-01-17 Thread Igor Mammedov

On Fri, 17 Jan 2020 16:38:02 +0100
Philippe Mathieu-Daudé  wrote:

> On 1/17/20 11:46 AM, Igor Mammedov wrote:
> > On Wed, 15 Jan 2020 22:33:46 +0100 (CET)
> > BALATON Zoltan  wrote:
> >   
> [...]
> > v2 basically works the same as original code, except of that
> > instead of fixing up ram_size, it ask user to fix CLI to
> > the same value.
> >   
> >> but I've not yet given up to keep current functionality
> >> (also for other boards where this series would drop it). If you don't like
> >> a generic callback then how about letting the board optionally specify in
> >> the machine class next to default_ram_size some constraints such as
> >> min_ram_size, max_ram_size and an array of valid_ram_sizes (like
> >> ppc460ex_sdram_bank_sizes[]) that the generic code doing the allocation
> >> could check and clamp sizes to the nearest valid one emitting a warning
> >> and only bail out if constraints are not given. This would cover most
> >> cases expect the e500 where it has an alignment without max so
> >> representing it as a list of valid values is probably not practical (could
> >> also have ram_alignment but then a generic callback may be simpler). Would
> >> that way work for you?  
> > 
> > I agree that we need to consolidate ram_size checks,
> > that would allow to clean up boards that practically do
> > the mostly the same. In the end it would allow to introspect
> > supported ram_sizes which would be good for upper layers.
> > 
> > I even started something in that direction but dropped
> > all of that patches as not directly related to this
> > series purpose.
> > Phillipe is also trying to something like this for raspi
> > board.
> > 
> > As for clamping, I still think that it's not good idea
> > to accept invalid value and fix it up.
> > Users should get what they asked for or get a clear error
> > (it's double true if user is management software as it
> > won't read any sort of warnings, well not all human users
> > would head warnings either as far as QEMU continues to run)
> > 
> > Anyways, most maintainers agreed to dropping fixups
> > and generalizing checks or generalizing fix up for convenience
> > of users frameworks are out of scope of this series.
> > 
> > Someone can work on these refactorings later on
> > (assuming someone is enough interested in it)
> > 
> > PS:
> > If you insist on fixing ram_size up, it would still be possible
> > to do so locally within board. One can take all RAM that user
> > provided and then create clamped memory region alias.
> > It's rather ugly but will do the job without affecting generic
> > code. (though I wouldn't volunteer to write up such a hack)  
> 
> I think from the virtualization point of view, you don't want to 
> allocate unused resources. Is that why you say it is "ugly"?
> 
>  From the emulation point of view, it is easier to stay as close as 
> possible to the hardware, where sometimes resources are wasted to 
> simplify the design (or reduce production delays).
> If it matches hardware, clamping doesn't look "ugly" to me.
> 

gist of discussion in this case is that:

bamboo HW supports 4 banks of RAM that could be populated by
256/128/64/32/16/8Mb DIMMs

so -m XXX must be multiple of that, otherwise we are not
able distribute all -m specified size between supported banks.

Current patch then would error out with
  "Max 4 banks of 256, 128, 64, 32, 16, 8 MB DIMM/bank supported"
  "Possible valid RAM size: XXX"
where XXX is clamped value original code uses to after fixup

Argument was that it's not convenient for users do what they
would do on real hw (i.e. calculate desired size manually
based on available banks/DIMMs).

I gave an attempt to dumping all possible sizes and it results
in not practically useful unique ~80 entries list, so that idea
also goes down the drain.

I agree that it's not convenient, but I think that human user
can easily adapt to error message and either fix CLI to use
suggested value (i.e. what they are getting now) or pick desired
DIMMs number/size and do the math.

Re: [PATCH v3 05/11] tests/virtio-9p: added readdir test

2020-01-17 Thread Christian Schoenebeck

On Freitag, 17. Januar 2020 16:51:29 CET Greg Kurz wrote:
> On Tue, 14 Jan 2020 00:08:51 +0100
> 
> Christian Schoenebeck  wrote:
> > The first readdir test simply checks the amount of directory
> > entries returned by 9pfs server, according to the created amount
> > of virtual files on 9pfs synth driver side. Then the subsequent
> > readdir test also checks whether all directory entries have the
> > expected file names (as created on 9pfs synth driver side),
> > ignoring their precise order in result list though.
> > 
> > Signed-off-by: Christian Schoenebeck 
> > ---
> 
> LGTM but I'm afraid this needs rebasing because virtio-9p-test.c got moved
> to tests/qtest/ by this commit (merged earlier this week):
> 
> commit 1e8a1fae7464ef79c9e50aa0f807d2c511be3c8e
> Author: Thomas Huth 
> Date:   Mon Sep 9 12:04:01 2019 +0200
> 
> test: Move qtests to a separate directory
> 
> The tests directory itself is pretty overcrowded, and it's hard to
> see which test belongs to which test subsystem (unit, qtest, ...).
> Let's move the qtests to a separate folder for more clarity.
> 
> Message-Id: <20191218103059.11729-6-th...@redhat.com>
> Reviewed-by: Paolo Bonzini 
> Signed-off-by: Thomas Huth 

Sure, I'll take care of that, and like previously discussed ...

> >  tests/virtio-9p-test.c | 149 +
> >  1 file changed, 149 insertions(+)
> > 
> > diff --git a/tests/virtio-9p-test.c b/tests/virtio-9p-test.c
> > index 06263edb53..721f13c1fb 100644
> > --- a/tests/virtio-9p-test.c
> > +++ b/tests/virtio-9p-test.c
> > @@ -68,6 +68,11 @@ static void v9fs_memread(P9Req *req, void *addr, size_t
> > len)> 
> >  req->r_off += len;
> >  
> >  }
> > 
> > +static void v9fs_uint8_read(P9Req *req, uint8_t *val)
> > +{
> > +v9fs_memread(req, val, 1);
> > +}
> > +
> > 
> >  static void v9fs_uint16_write(P9Req *req, uint16_t val)
> >  {
> >  
> >  uint16_t le_val = cpu_to_le16(val);
> > 
> > @@ -101,6 +106,12 @@ static void v9fs_uint32_read(P9Req *req, uint32_t
> > *val)> 
> >  le32_to_cpus(val);
> >  
> >  }
> > 
> > +static void v9fs_uint64_read(P9Req *req, uint64_t *val)
> > +{
> > +v9fs_memread(req, val, 8);
> > +le64_to_cpus(val);
> > +}
> > +
> > 
> >  /* len[2] string[len] */
> >  static uint16_t v9fs_string_size(const char *string)
> >  {
> > 
> > @@ -191,6 +202,7 @@ static const char *rmessage_name(uint8_t id)
> > 
> >  id == P9_RLOPEN ? "RLOPEN" :
> >  id == P9_RWRITE ? "RWRITE" :
> > 
> >  id == P9_RFLUSH ? "RFLUSH" :
> > +id == P9_RREADDIR ? "READDIR" :
> >  "";
> >  
> >  }
> > 
> > @@ -348,6 +360,82 @@ static void v9fs_rwalk(P9Req *req, uint16_t *nwqid,
> > v9fs_qid **wqid)> 
> >  v9fs_req_free(req);
> >  
> >  }
> > 
> > +/* size[4] Treaddir tag[2] fid[4] offset[8] count[4] */
> > +static P9Req *v9fs_treaddir(QVirtio9P *v9p, uint32_t fid, uint64_t
> > offset,
> > +uint32_t count, uint16_t tag)
> > +{
> > +P9Req *req;
> > +
> > +req = v9fs_req_init(v9p, 4 + 8 + 4, P9_TREADDIR, tag);
> > +v9fs_uint32_write(req, fid);
> > +v9fs_uint64_write(req, offset);
> > +v9fs_uint32_write(req, count);
> > +v9fs_req_send(req);
> > +return req;
> > +}
> > +
> > +struct V9fsDirent {
> > +v9fs_qid qid;
> > +uint64_t offset;
> > +uint8_t type;
> > +char *name;
> > +struct V9fsDirent *next;
> > +};
> > +
> > +/* size[4] Rreaddir tag[2] count[4] data[count] */
> > +static void v9fs_rreaddir(P9Req *req, uint32_t *count, uint32_t
> > *nentries,
> > +  struct V9fsDirent **entries)
> > +{
> > +uint32_t local_count;
> > +struct V9fsDirent *e = NULL;
> > +uint16_t slen;
> > +uint32_t n = 0;
> > +
> > +v9fs_req_recv(req, P9_RREADDIR);
> > +v9fs_uint32_read(req, _count);
> > +
> > +if (count) {
> > +*count = local_count;
> > +}
> > +
> > +for (int32_t togo = (int32_t)local_count;
> > + togo >= 13 + 8 + 1 + 2;
> > + togo -= 13 + 8 + 1 + 2 + slen, ++n)
> > +{
> > +if (!e) {
> > +e = g_malloc(sizeof(struct V9fsDirent));
> > +if (entries) {
> > +*entries = e;
> > +}
> > +} else {
> > +e = e->next = g_malloc(sizeof(struct V9fsDirent));
> > +}
> > +e->next = NULL;
> > +/* qid[13] offset[8] type[1] name[s] */
> > +v9fs_memread(req, >qid, 13);
> > +v9fs_uint64_read(req, >offset);
> > +v9fs_uint8_read(req, >type);
> > +v9fs_string_read(req, , >name);
> > +}
> > +
> > +if (nentries) {
> > +*nentries = n;
> > +}
> > +
> > +v9fs_req_free(req);
> > +}
> > +
> > +static void v9fs_free_dirents(struct V9fsDirent *e)
> > +{
> > +struct V9fsDirent *next = NULL;
> > +
> > +for (; e; e = next) {
> > +next = e->next;
> > +g_free(e->name);
> > +g_free(e);
> > +}
> > +}
> > +
> > 
> >  /*

Re: [PATCH v3 2/5] migration: Create MigrationState active field

* Juan Quintela (quint...@redhat.com) wrote:
> Right now, there is no easy way to dectect if we have already
> cancelled/finished/failed a migration.  This field is setup to true
> when we start a migration, and it is set to false as soon as we stop
> it.
> 
> It fixes a real bug, in ram_save_iterate() we call functions that
> wrote to the channel even if we know that migration has stopped for
> any reason.  This gives problems with multifd because we need to
> synchronize various semoaphores that we don't want to take.
> 
> Signed-off-by: Juan Quintela 

Why can't you use migration_is_active() in the ram.c case?
My preference would be just to stick with something derived
from the state rather than tacking another state bit on.

> ---
>  migration/migration.c | 5 +
>  migration/migration.h | 5 +
>  migration/ram.c   | 2 +-
>  migration/savevm.c| 2 ++
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 990bff00c0..60bc8710b6 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1583,6 +1583,8 @@ static void migrate_fd_cancel(MigrationState *s)
>  QEMUFile *f = migrate_get_current()->to_dst_file;
>  trace_migrate_fd_cancel();
>  
> +s->active = false;
> +
>  if (s->rp_state.from_dst_file) {
>  /* shutdown the rp socket, so causing the rp thread to shutdown */
>  qemu_file_shutdown(s->rp_state.from_dst_file);
> @@ -2834,6 +2836,7 @@ static void migration_completion(MigrationState *s)
>  }
>  
>  if (!migrate_colo_enabled()) {
> +s->active = false;
>  migrate_set_state(>state, current_active_state,
>MIGRATION_STATUS_COMPLETED);

You've not always got these two the same way around - i.e. do you change
the state first or do you set the active state first?  I think it needs
to be consistent.

>  }
> @@ -2859,6 +2862,7 @@ fail_invalidate:
>  }
>  
>  fail:
> +s->active = false;
>  migrate_set_state(>state, current_active_state,
>MIGRATION_STATUS_FAILED);
>  }
> @@ -3289,6 +3293,7 @@ static void *migration_thread(void *opaque)
>  }
>  
>  qemu_savevm_state_setup(s->to_dst_file);
> +s->active = true;
>  
>  if (qemu_savevm_nr_failover_devices()) {
>  migrate_set_state(>state, MIGRATION_STATUS_SETUP,
> diff --git a/migration/migration.h b/migration/migration.h
> index aa9ff6f27b..e0386efe95 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -169,6 +169,11 @@ struct MigrationState
>  
>  int state;
>  
> +/* Is the migration channel still open.  When migration finish,
> + * gets an error or is cancelled this becomes false.
> + */
> +
> +bool active;
>  /* State related to return path */
>  struct {
>  QEMUFile *from_dst_file;
> diff --git a/migration/ram.c b/migration/ram.c
> index 8f9f3bba5b..44ca56e1ea 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -3521,7 +3521,7 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>  ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>  
>  out:
> -if (ret >= 0) {
> +if (ret >= 0 && migrate_get_current()->active) {
>  multifd_send_sync_main(rs);
>  qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>  qemu_fflush(f);
> diff --git a/migration/savevm.c b/migration/savevm.c
> index adfdca26ac..3efde5b3dd 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -1550,6 +1550,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  qemu_mutex_unlock_iothread();
>  qemu_savevm_state_header(f);
>  qemu_savevm_state_setup(f);
> +ms->active = true;
>  qemu_mutex_lock_iothread();
>  
>  while (qemu_file_get_error(f) == 0) {
> @@ -1574,6 +1575,7 @@ static int qemu_savevm_state(QEMUFile *f, Error **errp)
>  status = MIGRATION_STATUS_COMPLETED;
>  }
>  migrate_set_state(>state, MIGRATION_STATUS_SETUP, status);
> +ms->active = false;
>  
>  /* f is outer parameter, it should not stay in global migration state 
> after
>   * this function finished */
> -- 
> 2.24.1
> 
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK

Re: [PATCH v2 00/86] refactor main RAM allocation to use hostmem backend


On 1/17/20 5:03 PM, Igor Mammedov wrote:

On Thu, 16 Jan 2020 16:43:07 +0100
Philippe Mathieu-Daudé  wrote:


On 1/15/20 10:01 PM, no-re...@patchew.org wrote:

Patchew URL: 
https://patchew.org/QEMU/1579100861-73692-1-git-send-email-imamm...@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing 
commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1


it doesn't work on my host, since it tries to use /tmp for building an image
and fails with no space.
is there any way to point it to some other place?


What is failing? Building the docker image, or building QEMU withing the 
docker container?

Re: [PATCH] qemu-img: Add --target-is-zero to convert

2020-01-17 Thread no-reply

Patchew URL: 
https://patchew.org/QEMU/20200117104156.1364069-1-david.edmond...@oracle.com/



Hi,

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

Message-id: 20200117104156.1364069-1-david.edmond...@oracle.com
Type: series
Subject: [PATCH] qemu-img: Add --target-is-zero to convert

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

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
c181366 qemu-img: Add --target-is-zero to convert

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

total: 1 errors, 0 warnings, 62 lines checked

Commit c181366f92d4 (qemu-img: Add --target-is-zero to convert) has style 
problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


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

Re: [PATCH] target/hppa: Allow, but diagnose, LDCW aligned only mod 4


On 1/17/20 4:49 PM, Helge Deller wrote:

On 17.01.20 02:53, Richard Henderson wrote:

The PA-RISC 1.1 specification says that LDCW must be aligned mod 16
or the operation is undefined.  However, real hardware only generates
an unaligned access trap for unaligned mod 4.

Match real hardware, but diagnose with GUEST_ERROR a violation of the
specification.

Reported-by: Helge Deller 
Suggested-by: John David Anglin 
Signed-off-by: Richard Henderson 

Helge, can you please test this against your failing kernel?
You will of course want to add -D logfile -d guest_errors to
you qemu command-line.


Yes, works as expected.
Thanks!

Please add:
Tested-by: Helge Deller 


[deller]$ tail -f logfile
Undefined ldc to address unaligned mod 16: 0504fa6c7848
Undefined ldc to address unaligned mod 16: 0504fa6c7a48
Undefined ldc to address unaligned mod 16: 0506f9434848
Undefined ldc to address unaligned mod 16: 0506f9434a48
Undefined ldc to address unaligned mod 16: 0508fa036848
Undefined ldc to address unaligned mod 16: 0508fa036a48
Undefined ldc to address unaligned mod 16: 050afa8c4848
Undefined ldc to address unaligned mod 16: 050afa8c4a48
Undefined ldc to address unaligned mod 16: 050cf94d1848
Undefined ldc to address unaligned mod 16: 050cf94d1a48







r~

---
  target/hppa/helper.h| 2 ++
  target/hppa/op_helper.c | 9 +
  target/hppa/translate.c | 6 +-
  3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/target/hppa/helper.h b/target/hppa/helper.h
index 38d834ef6b..2d483aab58 100644
--- a/target/hppa/helper.h
+++ b/target/hppa/helper.h
@@ -17,6 +17,8 @@ DEF_HELPER_FLAGS_3(stby_b_parallel, TCG_CALL_NO_WG, void, 
env, tl, tr)
  DEF_HELPER_FLAGS_3(stby_e, TCG_CALL_NO_WG, void, env, tl, tr)
  DEF_HELPER_FLAGS_3(stby_e_parallel, TCG_CALL_NO_WG, void, env, tl, tr)

+DEF_HELPER_FLAGS_1(ldc_check, TCG_CALL_NO_RWG, void, tl)
+
  DEF_HELPER_FLAGS_4(probe, TCG_CALL_NO_WG, tr, env, tl, i32, i32)

  DEF_HELPER_FLAGS_1(loaded_fr0, TCG_CALL_NO_RWG, void, env)
diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
index f0516e81f1..345cef2c08 100644
--- a/target/hppa/op_helper.c
+++ b/target/hppa/op_helper.c
@@ -153,6 +153,15 @@ void HELPER(stby_e_parallel)(CPUHPPAState *env, 
target_ulong addr,
  do_stby_e(env, addr, val, true, GETPC());
  }

+void HELPER(ldc_check)(target_ulong addr)
+{
+if (unlikely(addr & 0xf)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+  "Undefined ldc to address unaligned mod 16: "


"to unaligned address mod 16"?


+  TARGET_FMT_lx "\n", addr);
+}
+}
+
  target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
uint32_t level, uint32_t want)
  {
diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 2f8d407a82..669381dc1d 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2942,7 +2942,7 @@ static bool trans_st(DisasContext *ctx, arg_ldst *a)

  static bool trans_ldc(DisasContext *ctx, arg_ldst *a)
  {
-MemOp mop = MO_TEUL | MO_ALIGN_16 | a->size;
+MemOp mop = MO_TE | MO_ALIGN | a->size;



Hmmm you changed MO_TEUL -> MO_TE, so from MO_32 to MO_8.

Per your description, shouldn't this be MO_TEUL | MO_ALIGN_4?


  TCGv_reg zero, dest, ofs;
  TCGv_tl addr;

@@ -2958,8 +2958,12 @@ static bool trans_ldc(DisasContext *ctx, arg_ldst *a)

  form_gva(ctx, , , a->b, a->x, a->scale ? a->size : 0,
   a->disp, a->sp, a->m, ctx->mmu_idx == MMU_PHYS_IDX);
+
+gen_helper_ldc_check(addr);
  zero = tcg_const_reg(0);
  tcg_gen_atomic_xchg_reg(dest, addr, zero, ctx->mmu_idx, mop);
+tcg_temp_free(zero);
+
  if (a->m) {
  save_gpr(ctx, a->b, ofs);
  }

Re: [PATCH v3 1/3] qcow2: introduce Qcow2Metadata structure

2020-01-17 Thread Vladimir Sementsov-Ogievskiy

14.01.2020 11:22, Andrey Shinkevich wrote:
> The preliminary patch to provide an extendable structure for dumping
> QCOW2 metadata allocations in image. The command line optional key is
> introduced in the patch that follows.
> 
> Suggested-by: Vladimir Sementsov-Ogievskiy 
> Signed-off-by: Andrey Shinkevich 
> ---
>   qapi/block-core.json | 209 
> ++-
>   1 file changed, 208 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ff5e5e..fab7435 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -176,6 +176,209 @@
>  '*backing-image': 'ImageInfo',
>  '*format-specific': 'ImageInfoSpecific' } }
>   
> +
> +##
> +# @Qcow2Metadata:
> +#
> +# Encapsulates QCOW2 metadata information
> +#
> +# @qcow2-header: QCOW2 header info
> +#
> +# @active-l1: L1 active table info
> +#
> +# @refcount-table: refcount table info
> +#
> +# @crypt-header: encryption header info
> +#
> +# @bitmaps: bitmap tables info
> +#
> +# @snapshot-table: snapshot tables info
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Metadata',
> +  'data': { 'qcow2-header': 'Qcow2Header',
> +'refcount-table': 'Qcow2RefcountTable',
> +'active-l1': 'Qcow2L1Table',
> +'*crypt-header': 'Qcow2EncryptionHeader',
> +'*bitmaps': 'Qcow2Bitmaps',
> +'*snapshot-table': 'Qcow2SnapshotsTable' } }
> +
> +##
> +# @Qcow2Header:
> +#
> +# QCOW2 header information
> +#
> +# @version: version number
> +#
> +# @location: header offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Header',
> +  'data': {'version': 'int',
> +   'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2EncryptionHeader:
> +#
> +# QCOW2 encryption header information
> +#
> +# @location: header offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2EncryptionHeader',
> +  'data': {'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2RefcountTable:
> +#
> +# QCOW2 refcount table information
> +#
> +# @location: refcount table offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2RefcountTable',
> +  'data': {'location': 'Qcow2Allocation' } }
> +
> +##
> +# @Qcow2L1Table:
> +#
> +# L1 table information
> +#
> +# @l2-list: list of L2 table locations
> +#
> +# @location: L1 table offset and size in image
> +#
> +# @name: entity name the table relates to
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2L1Table',
> +  'data': {'l2-list': ['Qcow2Allocation'],

If we list only allocated l2, we lost information about their
idexes.

I think we need instead

'entries': ['Qcow2L1TableEntry'],


> +   'location': 'Qcow2Allocation',
> +   'name': 'str'} }

L1 table doesn't has name. Name is a property of snapshot.
I'd prefer to follow object model defined in docs/interop/qcow2.txt as
close as possible, with the only exception for "location" property, which
may be addeded to data structures like Qcow2L1Table.. (and I don't think
we need to add location into each Qcow2L1TableEntry), and dropping
corresponding offset and size fields, which are mirrored to the location
object.

Generic location object is good to automatically parse resulting json, to
make a view of it.

> +
> +##
> +# @Qcow2Allocation:
> +#
> +# QCOW2 data location in image
> +#
> +# @offset: data offset
> +#
> +# @size: data size
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Allocation',
> +  'data': {'offset': 'uint64', 'size': 'uint64' } }
> +
> +##
> +# @Qcow2Bitmaps:

This may be called Qcow2BitmapsExtension, to follow qcow2.txt

> +#
> +# QCOW2 bitmaps information
> +#
> +# @nb-bitmaps: the number of bitmaps contained in the image
> +#
> +# @bitmap-dir: bitmap directory information
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2Bitmaps',
> +  'data': {'nb-bitmaps': 'int',
> +   'bitmap-dir': 'Qcow2BitmapDir' } }

Hmm I don't like these abbreviations. Qcow2BitmapDirectory will
interfere with existing type, I understand but let's at least keep
full field names, like bitmap-directory.

Also, wouldn't existence of types Qcow2BitmapDir and Qcow2BitmapDirectory
be confusing? I think it will.

Maybe, for qapi, we need QapiQcow2BitmapDirectory, to make it obvious?

[upd, after looking ahead], it should be called Qcow2BitmapDirectoryInfo.

> +
> +##
> +# @Qcow2BitmapDir:
> +#
> +# QCOW2 bitmap directory information
> +#
> +# @dir-entries: list of bitmap directory entries
> +#
> +# @location: bitmap directory offset and size in image
> +#
> +# Since: 5.0
> +##
> +{ 'struct': 'Qcow2BitmapDir',
> +  'data': {'dir-entries': ['Qcow2BitmapDirectoryEntry'],
> +   'location': 'Qcow2Allocation' } }

I'd call them just 'entries'. It inconsistent with locations,
and I didn't see somewhere in qapi prefixing fields by structure
name abbreviation.

> +
> +##
> +# @Qcow2BitmapDirectoryEntry:
> +#
> +# QCOW2 bitmap directory entry information
> +#
> +# @bitmap-table:

Re: [PATCH v2 00/86] refactor main RAM allocation to use hostmem backend

2020-01-17 Thread Igor Mammedov

On Thu, 16 Jan 2020 16:43:07 +0100
Philippe Mathieu-Daudé  wrote:

> On 1/15/20 10:01 PM, no-re...@patchew.org wrote:
> > Patchew URL: 
> > https://patchew.org/QEMU/1579100861-73692-1-git-send-email-imamm...@redhat.com/
> > 
> > 
> > 
> > Hi,
> > 
> > This series failed the docker-quick@centos7 build test. Please find the 
> > testing commands and
> > their output below. If you have Docker installed, you can probably 
> > reproduce it
> > locally.
> > 
> > === TEST SCRIPT BEGIN ===
> > #!/bin/bash
> > make docker-image-centos7 V=1 NETWORK=1
> > time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1

it doesn't work on my host, since it tries to use /tmp for building an image
and fails with no space.
is there any way to point it to some other place?

> > === TEST SCRIPT END ===
> > 
> >TESTiotest-qcow2: 217
> > socket_accept failed: Resource temporarily unavailable
> > **
> > ERROR:/tmp/qemu-test/src/tests/qtest/libqtest.c:272:qtest_init_without_qmp_handshake:
> >  assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
> > /tmp/qemu-test/src/tests/qtest/libqtest.c:140: kill_qemu() tried to 
> > terminate QEMU process but encountered exit status 1 (expected 0)
> > ERROR - Bail out! 
> > ERROR:/tmp/qemu-test/src/tests/qtest/libqtest.c:272:qtest_init_without_qmp_handshake:
> >  assertion failed: (s->fd >= 0 && s->qmp_fd >= 0)
> > make: *** [check-qtest-x86_64] Error 1  
> 
> Is this something we should care about?


#217 passes on my host, albeit it takes time to complete

> 
> > make: *** Waiting for unfinished jobs
> >TESTcheck-qtest-aarch64: tests/qtest/test-hmp
> >TESTiotest-qcow2: 220
> > ---
> >  raise CalledProcessError(retcode, cmd)
> > subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', 
> > '--label', 'com.qemu.instance.uuid=755fcbfd171f40b1b16d6aac34e7c09b', '-u', 
> > '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 
> > 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', 
> > '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', 
> > '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', 
> > '/var/tmp/patchew-tester-tmp-qd5wtatm/src/docker-src.2020-01-15-15.50.14.28652:/var/tmp/qemu:z,ro',
> >  'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit 
> > status 2.
> > filter=--filter=label=com.qemu.instance.uuid=755fcbfd171f40b1b16d6aac34e7c09b
> > make[1]: *** [docker-run] Error 1
> > make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-qd5wtatm/src'
> > make: *** [docker-run-test-quick@centos7] Error 2
> > 
> > real11m0.219s
> > user0m8.267s
> > 
> > 
> > The full log is available at
> > http://patchew.org/logs/1579100861-73692-1-git-send-email-imamm...@redhat.com/testing.docker-quick@centos7/?type=message.
> > ---
> > Email generated automatically by Patchew [https://patchew.org/].
> > Please send your feedback to patchew-de...@redhat.com
> >   
> 
>

Re: qemu-4.0.1: vhost_region_add_section:Section rounded to 0 prior to previous a0000