Re: [Qemu-devel] [PATCH 1/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Michael Roth]

Quoting Markus Armbruster (2017-05-31 12:05:16)
> Markus Armbruster  writes:
> 
> > "Dr. David Alan Gilbert"  writes:
> >
> >> I notice this pair of patches doesn't seem to have gone anywhere.
> >> WHile it's labelled as a monitor fix, it's all QOM stuff, so I don't
> >> think it should be going via me.
> >
> > It's somewhere between QOM and QemuOpts.  Andreas, please have a look.
> > Feel free to ask me to take the patch through my tree.
> 
> I considered applying the series to qapi-next, and realized I'm not sure
> what the latest version is.  I could peruse the archives, but that's
> work.  Mike, would you mind resending your latest version?

This version (v4) is the latest, but I've rebased/re-tested on master
and re-submitted those as v5. Thanks!

[Qemu-devel] [PATCH v5 2/2] monitor: fix object_del for command-line-created objects

[Michael Roth]

Currently objects specified on the command-line are only partially
cleaned up when 'object_del' is issued in either HMP or QMP: the
object itself is fully finalized, but the QemuOpts are not removed.
This results in the following behavior:

  x86_64-softmmu/qemu-system-x86_64 -monitor stdio \
-object memory-backend-ram,id=ram1,size=256M

  QEMU 2.7.91 monitor - type 'help' for more information
  (qemu) object_del ram1
  (qemu) object_del ram1
  object 'ram1' not found
  (qemu) object_add memory-backend-ram,id=ram1,size=256M
  Duplicate ID 'ram1' for object
  Try "help object_add" for more information

which can be an issue for use-cases like memory hotplug.

This happens on the HMP side because hmp_object_add() attempts to
create a temporary QemuOpts entry with ID 'ram1', which ends up
conflicting with the command-line-created entry, since it was never
cleaned up during the previous hmp_object_del() call.

We address this by adding a check in user_creatable_del(), which
is called by both qmp_object_del() and hmp_object_del() to handle
the actual object cleanup, to determine whether an option group entry
matching the object's ID is present and removing it if it is.

Note that qmp_object_add() never attempts to create a temporary
QemuOpts entry, so it does not encounter the duplicate ID error,
which is why this isn't generally visible in libvirt.

Cc: "Dr. David Alan Gilbert" 
Cc: Markus Armbruster 
Cc: Eric Blake 
Cc: Daniel Berrange 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth 
Reviewed-by: Daniel P. Berrange 
Reviewed-by: Markus Armbruster 
---
 qom/object_interfaces.c| 9 +
 tests/check-qom-proplist.c | 4 ++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index d4253a8..ff27e06 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -4,6 +4,7 @@
 #include "qemu/module.h"
 #include "qapi-visit.h"
 #include "qapi/opts-visitor.h"
+#include "qemu/config-file.h"
 
 void user_creatable_complete(Object *obj, Error **errp)
 {
@@ -181,6 +182,14 @@ void user_creatable_del(const char *id, Error **errp)
 error_setg(errp, "object '%s' is in use, can not be deleted", id);
 return;
 }
+
+/*
+ * if object was defined on the command-line, remove its corresponding
+ * option group entry
+ */
+qemu_opts_del(qemu_opts_find(qemu_find_opts_err("object", _abort),
+ id));
+
 object_unparent(obj);
 }
 
diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index e3f56ca..ddec574 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -437,9 +437,9 @@ static void test_dummy_createcmdl(void)
  * check for this in user_creatable_del() and remove the QemuOpts if
  * it is present.
  *
- * FIXME: add an assert to verify that the QemuOpts is cleaned up
- * once the corresponding cleanup code is added.
+ * The below check ensures this works as expected.
  */
+g_assert_null(qemu_opts_find(_object_opts, "dev0"));
 }
 
 static void test_dummy_badenum(void)
-- 
2.7.4

[Qemu-devel] [PATCH v5 1/2] tests: check-qom-proplist: add checks for cmdline-created objects

[Michael Roth]

check-qom-proplist originally added tests for verifying that
object-creation helpers object_new_with_{props,propv} behaved in
similar fashion to the "traditional" method involving setting each
individual property separately after object creation rather than
via a single call.

Another similar "helper" for creating Objects exists in the form of
objects specified via -object command-line parameters. By that
rationale, we extend check-qom-proplist to include similar checks
for command-line-created objects by employing the same
qemu_opts_parse()-based parsing the vl.c employs.

This parser has a side-effect of parsing the object's options into
a QemuOpt structure and registering this in the global QemuOptsList
using the Object's ID. This can conflict with future Object instances
that attempt to use the same ID if we don't ensure this is cleaned
up as part of Object finalization, so we include a FIXME stub to test
for this case, which will then be resolved in a subsequent patch.

Suggested-by: Daniel Berrange 
Cc: "Dr. David Alan Gilbert" 
Cc: Markus Armbruster 
Cc: Eric Blake 
Cc: Daniel Berrange 
Cc: qemu-sta...@nongnu.org
Signed-off-by: Michael Roth 
Reviewed-by: Markus Armbruster 
---
 tests/check-qom-proplist.c | 55 ++
 1 file changed, 55 insertions(+)

diff --git a/tests/check-qom-proplist.c b/tests/check-qom-proplist.c
index a16cefc..e3f56ca 100644
--- a/tests/check-qom-proplist.c
+++ b/tests/check-qom-proplist.c
@@ -23,6 +23,9 @@
 #include "qapi/error.h"
 #include "qom/object.h"
 #include "qemu/module.h"
+#include "qemu/option.h"
+#include "qemu/config-file.h"
+#include "qom/object_interfaces.h"
 
 
 #define TYPE_DUMMY "qemu-dummy"
@@ -162,6 +165,10 @@ static const TypeInfo dummy_info = {
 .instance_finalize = dummy_finalize,
 .class_size = sizeof(DummyObjectClass),
 .class_init = dummy_class_init,
+.interfaces = (InterfaceInfo[]) {
+{ TYPE_USER_CREATABLE },
+{ }
+}
 };
 
 
@@ -320,6 +327,14 @@ static const TypeInfo dummy_backend_info = {
 .class_size = sizeof(DummyBackendClass),
 };
 
+static QemuOptsList qemu_object_opts = {
+.name = "object",
+.implied_opt_name = "qom-type",
+.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
+.desc = {
+{ }
+},
+};
 
 
 static void test_dummy_createv(void)
@@ -388,6 +403,45 @@ static void test_dummy_createlist(void)
 object_unparent(OBJECT(dobj));
 }
 
+static void test_dummy_createcmdl(void)
+{
+QemuOpts *opts;
+DummyObject *dobj;
+Error *err = NULL;
+const char *params = TYPE_DUMMY \
+ ",id=dev0," \
+ "bv=yes,sv=Hiss hiss hiss,av=platypus";
+
+qemu_add_opts(_object_opts);
+opts = qemu_opts_parse(_object_opts, params, true, );
+g_assert(err == NULL);
+g_assert(opts);
+
+dobj = DUMMY_OBJECT(user_creatable_add_opts(opts, ));
+g_assert(err == NULL);
+g_assert(dobj);
+g_assert_cmpstr(dobj->sv, ==, "Hiss hiss hiss");
+g_assert(dobj->bv == true);
+g_assert(dobj->av == DUMMY_PLATYPUS);
+
+user_creatable_del("dev0", );
+g_assert(err == NULL);
+error_free(err);
+
+/* cmdline-parsing via qemu_opts_parse() results in a QemuOpts entry
+ * corresponding to the Object's ID to be added to the QemuOptsList
+ * for objects. To avoid having this entry conflict with future
+ * Objects using the same ID (which can happen in cases where
+ * qemu_opts_parse() is used to parse the object params, such as
+ * with hmp_object_add() at the time of this comment), we need to
+ * check for this in user_creatable_del() and remove the QemuOpts if
+ * it is present.
+ *
+ * FIXME: add an assert to verify that the QemuOpts is cleaned up
+ * once the corresponding cleanup code is added.
+ */
+}
+
 static void test_dummy_badenum(void)
 {
 Error *err = NULL;
@@ -525,6 +579,7 @@ int main(int argc, char **argv)
 
 g_test_add_func("/qom/proplist/createlist", test_dummy_createlist);
 g_test_add_func("/qom/proplist/createv", test_dummy_createv);
+g_test_add_func("/qom/proplist/createcmdline", test_dummy_createcmdl);
 g_test_add_func("/qom/proplist/badenum", test_dummy_badenum);
 g_test_add_func("/qom/proplist/getenum", test_dummy_getenum);
 g_test_add_func("/qom/proplist/iterator", test_dummy_iterator);
-- 
2.7.4

[Qemu-devel] [PATCH v5 0/2] Fixes/tests for hmp_object_del()

[Michael Roth]

hmp_object_del() followed by a subsequent hmp_object_add() can trigger a
duplicate ID error if the previous object shared the same ID and was added
via the command-line. Please see patch 2/2 for more details.

This patchset fixes the issue in question and adds some general unit tests
for object created via -object, which we later extend to verify the fix in
question.

Changes since v4:

  - Rebased on master and re-tested

Changes since v3:

  - Fixed up comment formating (Markus)
  - Instead of segfaulting, use _abort if assumptions about
'object' property group existence change (Markus)
  - Use g_assert_null in place of g_assert(... == NULL) (Markus)

Changes since v2:

  - Moved the generic unit tests ahead of the fix patch, with a FIXME
in place of the actual check for the failure addressed in patch
2/2 (Daniel/Markus)
  - Dropped check for existence of objects' QemuOptsList (Markus)
  - Dropped unintended whitespace removal in PATCH 1/2
  - Slight rewording of commit messages to reflect the changes and fix
minor grammar errors.

Changes since v1:

  - Moved QemuOpt cleanup out of {qmp,hmp}_object_del() and into common
user_creatable_del() path (Daniel, David)
  - Added corresponding test case in check-qom-proplist

Re: [Qemu-devel] [PATCH 1/5] spapr: Introduce DRC subclasses

[Michael Roth]

Quoting David Gibson (2017-06-02 02:29:48)
> Currently we only have a single QOM type for all DRCs, but lots of
> places where we switch behaviour based on the DRC's PAPR defined type.
> This is a poor use of our existing type system.
> 
> So, instead create QOM subclasses for each PAPR defined DRC type.  We
> also introduce intermediate subclasses for physical and logical DRCs,
> a division which will be useful later on.
> 
> Instead of being stored in the DRC object itself, the PAPR type is now
> stored in the class structure.  There are still many places where we
> switch directly on the PAPR type value, but this at least provides the
> basis to start to remove those.
> 
> Signed-off-by: David Gibson 
> ---
>  hw/ppc/spapr.c |   5 +-
>  hw/ppc/spapr_drc.c | 120 
> +
>  hw/ppc/spapr_pci.c |   3 +-
>  include/hw/ppc/spapr_drc.h |  47 --
>  4 files changed, 136 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 5d10366..456f9e7 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1911,7 +1911,7 @@ static void 
> spapr_create_lmb_dr_connectors(sPAPRMachineState *spapr)
>  uint64_t addr;
> 
>  addr = i * lmb_size + spapr->hotplug_memory.base;
> -drc = spapr_dr_connector_new(OBJECT(spapr), 
> SPAPR_DR_CONNECTOR_TYPE_LMB,
> +drc = spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_LMB,
>   addr/lmb_size);
>  qemu_register_reset(spapr_drc_reset, drc);
>  }
> @@ -2008,8 +2008,7 @@ static void spapr_init_cpus(sPAPRMachineState *spapr)
> 
>  if (mc->has_hotpluggable_cpus) {
>  sPAPRDRConnector *drc =
> -spapr_dr_connector_new(OBJECT(spapr),
> -   SPAPR_DR_CONNECTOR_TYPE_CPU,
> +spapr_dr_connector_new(OBJECT(spapr), TYPE_SPAPR_DRC_CPU,
> (core_id / smp_threads) * smt);
> 
>  qemu_register_reset(spapr_drc_reset, drc);
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index a35314e..690b41f 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -70,14 +70,23 @@ static sPAPRDRConnectorTypeShift 
> get_type_shift(sPAPRDRConnectorType type)
>  return shift;
>  }
> 
> +sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> +{
> +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
> +return 1 << drck->typeshift;
> +}
> +
>  uint32_t spapr_drc_index(sPAPRDRConnector *drc)
>  {
> +sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> +
>  /* no set format for a drc index: it only needs to be globally
>   * unique. this is how we encode the DRC type on bare-metal
>   * however, so might as well do that here
>   */
> -return (get_type_shift(drc->type) << DRC_INDEX_TYPE_SHIFT) |
> -(drc->id & DRC_INDEX_ID_MASK);
> +return (drck->typeshift << DRC_INDEX_TYPE_SHIFT)
> +| (drc->id & DRC_INDEX_ID_MASK);
>  }
> 
>  static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> @@ -107,7 +116,7 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
>   * If the LMB being removed doesn't belong to a DIMM device that is
>   * actually being unplugged, fail the isolation request here.
>   */
> -if (drc->type == SPAPR_DR_CONNECTOR_TYPE_LMB) {
> +if (spapr_drc_type(drc) == SPAPR_DR_CONNECTOR_TYPE_LMB) {
>  if ((state == SPAPR_DR_ISOLATION_STATE_ISOLATED) &&
>   !drc->awaiting_release) {
>  return RTAS_OUT_HW_ERROR;
> @@ -177,7 +186,7 @@ static uint32_t set_allocation_state(sPAPRDRConnector 
> *drc,
>  }
>  }
> 
> -if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI) {
> +if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI) {
>  drc->allocation_state = state;
>  if (drc->awaiting_release &&
>  drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
> @@ -191,11 +200,6 @@ static uint32_t set_allocation_state(sPAPRDRConnector 
> *drc,
>  return RTAS_OUT_SUCCESS;
>  }
> 
> -sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
> -{
> -return drc->type;
> -}
> -
>  static const char *get_name(sPAPRDRConnector *drc)
>  {
>  return drc->name;
> @@ -217,7 +221,7 @@ static void set_signalled(sPAPRDRConnector *drc)
>  static uint32_t entity_sense(sPAPRDRConnector *drc, sPAPRDREntitySense 
> *state)
>  {
>  if (drc->dev) {
> -if (drc->type != SPAPR_DR_CONNECTOR_TYPE_PCI &&
> +if (spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PCI &&
>  drc->allocation_state == SPAPR_DR_ALLOCATION_STATE_UNUSABLE) {
>  /* for logical DR, we return a state of UNUSABLE
>   * iff the allocation state UNUSABLE.
> @@ -235,7 +239,7 @@ static uint32_t entity_sense(sPAPRDRConnector *drc, 
> sPAPRDREntitySense 

Re: [Qemu-devel] [PATCH 2/5] spapr: Clean up spapr_dr_connector_by_*()

[Michael Roth]

Quoting David Gibson (2017-06-02 02:29:49)
>  * Change names to something less ludicrously verbose
>  * Now that we have QOM subclasses for the different DRC types, use a QOM
>typename instead of a PAPR type value parameter
> 
> The latter allows removal of the get_type_shift() helper.
> 
> Signed-off-by: David Gibson 

Reviewed-by: Michael Roth 

> ---
>  hw/ppc/spapr.c | 28 ++--
>  hw/ppc/spapr_drc.c | 34 ++
>  hw/ppc/spapr_events.c  |  2 +-
>  hw/ppc/spapr_pci.c |  6 ++
>  include/hw/ppc/spapr_drc.h |  5 ++---
>  5 files changed, 29 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 456f9e7..7aac3b9 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -460,7 +460,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>  uint32_t radix_AP_encodings[PPC_PAGE_SIZES_MAX_SZ];
>  int i;
> 
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index);
>  if (drc) {
>  drc_index = spapr_drc_index(drc);
>  _FDT((fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index)));
> @@ -653,7 +653,7 @@ static int spapr_populate_drconf_memory(sPAPRMachineState 
> *spapr, void *fdt)
>  if (i >= hotplug_lmb_start) {
>  sPAPRDRConnector *drc;
> 
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, i);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB, i);
>  g_assert(drc);
> 
>  dynamic_memory[0] = cpu_to_be32(addr >> 32);
> @@ -2528,8 +2528,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
>  uint64_t addr = addr_start;
> 
>  for (i = 0; i < nr_lmbs; i++) {
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -addr/SPAPR_MEMORY_BLOCK_SIZE);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> +  addr / SPAPR_MEMORY_BLOCK_SIZE);
>  g_assert(drc);
> 
>  fdt = create_device_tree(_size);
> @@ -2550,8 +2550,8 @@ static void spapr_add_lmbs(DeviceState *dev, uint64_t 
> addr_start, uint64_t size,
>   */
>  if (dev->hotplugged) {
>  if (dedicated_hp_event_source) {
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> +  addr_start / SPAPR_MEMORY_BLOCK_SIZE);
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  
> spapr_hotplug_req_add_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
> nr_lmbs,
> @@ -2668,8 +2668,8 @@ static sPAPRDIMMState 
> *spapr_recover_pending_dimm_state(sPAPRMachineState *ms,
> 
>  addr = addr_start;
>  for (i = 0; i < nr_lmbs; i++) {
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -   addr / SPAPR_MEMORY_BLOCK_SIZE);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> +  addr / SPAPR_MEMORY_BLOCK_SIZE);
>  g_assert(drc);
>  if (drc->indicator_state != SPAPR_DR_INDICATOR_STATE_INACTIVE) {
>  avail_lmbs++;
> @@ -2752,8 +2752,8 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
> 
>  addr = addr_start;
>  for (i = 0; i < nr_lmbs; i++) {
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -addr / SPAPR_MEMORY_BLOCK_SIZE);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> +  addr / SPAPR_MEMORY_BLOCK_SIZE);
>  g_assert(drc);
> 
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -2761,8 +2761,8 @@ static void spapr_memory_unplug_request(HotplugHandler 
> *hotplug_dev,
>  addr += SPAPR_MEMORY_BLOCK_SIZE;
>  }
> 
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB,
> -   addr_start / SPAPR_MEMORY_BLOCK_SIZE);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_LMB,
> +  addr_start / SPAPR_MEMORY_BLOCK_SIZE);
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
>  spapr_hotplug_req_remove_by_count_indexed(SPAPR_DR_CONNECTOR_TYPE_LMB,
>nr_lmbs, spapr_drc_index(drc));
> @@ -2833,7 +2833,7 @@ void spapr_core_unplug_request(HotplugHandler 
> *hotplug_dev, DeviceState *dev,
>  return;
>  }
> 
> -drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, index * smt);
> +drc = spapr_drc_by_id(TYPE_SPAPR_DRC_CPU, index * smt);
>  g_assert(drc);
> 
>  drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -2868,7 +2868,7 @@ static void spapr_core_plug(HotplugHandler 
> 

Re: [Qemu-devel] [PATCH 4/5] spapr: Eliminate spapr_drc_get_type_str()

[Michael Roth]

Quoting David Gibson (2017-06-02 02:29:51)
> This function was used in generating the device tree.  However, now that
> we have different QOM types for different DRC types we can easily store
> the information we need in the class structure and avoid this specialized
> lookup function.
> 
> Signed-off-by: David Gibson 

Reviewed-by: Michael Roth 

> ---
>  hw/ppc/spapr_drc.c | 31 ---
>  include/hw/ppc/spapr_drc.h |  1 +
>  2 files changed, 5 insertions(+), 27 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 27d4bd3..d43c9cd 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -716,6 +716,7 @@ static void spapr_drc_cpu_class_init(ObjectClass *k, void 
> *data)
>  sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> 
>  drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_CPU;
> +drck->typename = "CPU";
>  }
> 
>  static void spapr_drc_pci_class_init(ObjectClass *k, void *data)
> @@ -723,6 +724,7 @@ static void spapr_drc_pci_class_init(ObjectClass *k, void 
> *data)
>  sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> 
>  drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_PCI;
> +drck->typename = "28";
>  }
> 
>  static void spapr_drc_lmb_class_init(ObjectClass *k, void *data)
> @@ -730,6 +732,7 @@ static void spapr_drc_lmb_class_init(ObjectClass *k, void 
> *data)
>  sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_CLASS(k);
> 
>  drck->typeshift = SPAPR_DR_CONNECTOR_TYPE_SHIFT_LMB;
> +drck->typename = "MEM";
>  }
> 
>  static const TypeInfo spapr_dr_connector_info = {
> @@ -796,31 +799,6 @@ sPAPRDRConnector *spapr_drc_by_id(const char *type, 
> uint32_t id)
>| (id & DRC_INDEX_ID_MASK));
>  }
> 
> -/* generate a string the describes the DRC to encode into the
> - * device tree.
> - *
> - * as documented by PAPR+ v2.7, 13.5.2.6 and C.6.1
> - */
> -static const char *spapr_drc_get_type_str(sPAPRDRConnectorType type)
> -{
> -switch (type) {
> -case SPAPR_DR_CONNECTOR_TYPE_CPU:
> -return "CPU";
> -case SPAPR_DR_CONNECTOR_TYPE_PHB:
> -return "PHB";
> -case SPAPR_DR_CONNECTOR_TYPE_VIO:
> -return "SLOT";
> -case SPAPR_DR_CONNECTOR_TYPE_PCI:
> -return "28";
> -case SPAPR_DR_CONNECTOR_TYPE_LMB:
> -return "MEM";
> -default:
> -g_assert(false);
> -}
> -
> -return NULL;
> -}
> -
>  /**
>   * spapr_drc_populate_dt
>   *
> @@ -902,8 +880,7 @@ int spapr_drc_populate_dt(void *fdt, int fdt_offset, 
> Object *owner,
>  drc_names = g_string_insert_len(drc_names, -1, "\0", 1);
> 
>  /* ibm,drc-types */
> -drc_types = g_string_append(drc_types,
> -
> spapr_drc_get_type_str(spapr_drc_type(drc)));
> +drc_types = g_string_append(drc_types, drck->typename);
>  drc_types = g_string_insert_len(drc_types, -1, "\0", 1);
>  }
> 
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 53b0f8b..8a4889a 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -212,6 +212,7 @@ typedef struct sPAPRDRConnectorClass {
> 
>  /*< public >*/
>  sPAPRDRConnectorTypeShift typeshift;
> +const char *typename; /* used in device tree, PAPR 13.5.2.6 & C.6.1 */
> 
>  /* accessors for guest-visible (generally via RTAS) DR state */
>  uint32_t (*set_isolation_state)(sPAPRDRConnector *drc,
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [PATCH 5/5] spapr: Remove some non-useful properties on DRC objects

[Michael Roth]

Quoting David Gibson (2017-06-02 02:29:52)
>  * 'connector_type' is easily derived from the 'index' property, so there's
>no point to it (it's also implicit in the QOM type of the DRC)
>  * 'isolation-state', 'indicator-state' and 'allocation-state' are
>part of the transaction between qemu and guest during PAPR hotplug
>operations, and outside tools really have no business looking at it
>(especially not changing, and these were RW properties)
>  * 'entity-sense' is basically just a weird PAPR encoding of whether there
>is a device connected to this DRC
> 
> Strictly speaking removing these properties is breaking the qemu interface.
> However, I'm pretty sure no management tools have ever used these.  For
> debugging there are better alternatives.  Therefore, I think removing these
> broken interfaces is the better option.

Debugging was indeed the primary motivation behind these. I agree these
don't really serve any useful purpose now and probably should've just
been traces from the start.

> 
> Signed-off-by: David Gibson 

Reviewed-by: Michael Roth 

> ---
>  hw/ppc/spapr_drc.c | 29 -
>  1 file changed, 29 deletions(-)
> 
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index d43c9cd..4dd26a8 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -228,14 +228,6 @@ static void prop_get_index(Object *obj, Visitor *v, 
> const char *name,
>  visit_type_uint32(v, name, , errp);
>  }
> 
> -static void prop_get_type(Object *obj, Visitor *v, const char *name,
> -  void *opaque, Error **errp)
> -{
> -sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> -uint32_t value = (uint32_t)spapr_drc_type(drc);
> -visit_type_uint32(v, name, , errp);
> -}
> -
>  static char *prop_get_name(Object *obj, Error **errp)
>  {
>  sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> @@ -243,17 +235,6 @@ static char *prop_get_name(Object *obj, Error **errp)
>  return g_strdup(drck->get_name(drc));
>  }
> 
> -static void prop_get_entity_sense(Object *obj, Visitor *v, const char *name,
> -  void *opaque, Error **errp)
> -{
> -sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> -sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> -uint32_t value;
> -
> -drck->entity_sense(drc, );
> -visit_type_uint32(v, name, , errp);
> -}
> -
>  static void prop_get_fdt(Object *obj, Visitor *v, const char *name,
>   void *opaque, Error **errp)
>  {
> @@ -670,20 +651,10 @@ static void spapr_dr_connector_instance_init(Object 
> *obj)
>  {
>  sPAPRDRConnector *drc = SPAPR_DR_CONNECTOR(obj);
> 
> -object_property_add_uint32_ptr(obj, "isolation-state",
> -   >isolation_state, NULL);
> -object_property_add_uint32_ptr(obj, "indicator-state",
> -   >indicator_state, NULL);
> -object_property_add_uint32_ptr(obj, "allocation-state",
> -   >allocation_state, NULL);
>  object_property_add_uint32_ptr(obj, "id", >id, NULL);
>  object_property_add(obj, "index", "uint32", prop_get_index,
>  NULL, NULL, NULL, NULL);
> -object_property_add(obj, "connector_type", "uint32", prop_get_type,
> -NULL, NULL, NULL, NULL);
>  object_property_add_str(obj, "name", prop_get_name, NULL, NULL);
> -object_property_add(obj, "entity-sense", "uint32", prop_get_entity_sense,
> -NULL, NULL, NULL, NULL);
>  object_property_add(obj, "fdt", "struct", prop_get_fdt,
>  NULL, NULL, NULL, NULL);
>  }
> -- 
> 2.9.4
> 

Re: [Qemu-devel] [PATCH 3/5] spapr: Move configure-connector state into DRC

[Michael Roth]

Quoting David Gibson (2017-06-02 02:29:50)
> Currently the sPAPRMachineState contains a of sPAPRConfigureConnector

"contains a list"?

> structures which store intermediate state for the ibm,configure-connector
> RTAS call.
> 
> This was an attempt to separate this state from the core of the DRC state.
> However the configure connector process is intimately tied to the DRC
> model, so there's really no point trying to have two levels of interface
> here.
> 
> Moving the configure-connector state into its corresponding DRC allows
> removal of a number of helpers for maintaining the anciliary list.
> 
> Signed-off-by: David Gibson 

Reviewed-by: Michael Roth 

> ---
>  hw/ppc/spapr.c |  4 ---
>  hw/ppc/spapr_drc.c | 73 
> --
>  include/hw/ppc/spapr.h | 14 -
>  include/hw/ppc/spapr_drc.h |  7 +
>  4 files changed, 25 insertions(+), 73 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7aac3b9..6234dbd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2339,10 +2339,6 @@ static void ppc_spapr_init(MachineState *machine)
>  register_savevm_live(NULL, "spapr/htab", -1, 1,
>   _htab_handlers, spapr);
> 
> -/* used by RTAS */
> -QTAILQ_INIT(>ccs_list);
> -qemu_register_reset(spapr_ccs_reset_hook, spapr);
> -
>  qemu_register_boot_set(spapr_boot_set, spapr);
> 
>  if (kvm_enabled()) {
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 2514f87..27d4bd3 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -27,34 +27,6 @@
>  #define DRC_INDEX_TYPE_SHIFT 28
>  #define DRC_INDEX_ID_MASK ((1ULL << DRC_INDEX_TYPE_SHIFT) - 1)
> 
> -static sPAPRConfigureConnectorState *spapr_ccs_find(sPAPRMachineState *spapr,
> -uint32_t drc_index)
> -{
> -sPAPRConfigureConnectorState *ccs = NULL;
> -
> -QTAILQ_FOREACH(ccs, >ccs_list, next) {
> -if (ccs->drc_index == drc_index) {
> -break;
> -}
> -}
> -
> -return ccs;
> -}
> -
> -static void spapr_ccs_add(sPAPRMachineState *spapr,
> -  sPAPRConfigureConnectorState *ccs)
> -{
> -g_assert(!spapr_ccs_find(spapr, ccs->drc_index));
> -QTAILQ_INSERT_HEAD(>ccs_list, ccs, next);
> -}
> -
> -static void spapr_ccs_remove(sPAPRMachineState *spapr,
> - sPAPRConfigureConnectorState *ccs)
> -{
> -QTAILQ_REMOVE(>ccs_list, ccs, next);
> -g_free(ccs);
> -}
> -
>  sPAPRDRConnectorType spapr_drc_type(sPAPRDRConnector *drc)
>  {
>  sPAPRDRConnectorClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -81,6 +53,16 @@ static uint32_t set_isolation_state(sPAPRDRConnector *drc,
> 
>  trace_spapr_drc_set_isolation_state(spapr_drc_index(drc), state);
> 
> +/* if the guest is configuring a device attached to this DRC, we
> + * should reset the configuration state at this point since it may
> + * no longer be reliable (guest released device and needs to start
> + * over, or unplug occurred so the FDT is no longer valid)
> + */
> +if (state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> +g_free(drc->ccs);
> +drc->ccs = NULL;
> +}
> +
>  if (state == SPAPR_DR_ISOLATION_STATE_UNISOLATED) {
>  /* cannot unisolate a non-existent resource, and, or resources
>   * which are in an 'UNUSABLE' allocation state. (PAPR 2.7, 13.5.3.5)
> @@ -485,6 +467,10 @@ static void reset(DeviceState *d)
>  sPAPRDREntitySense state;
> 
>  trace_spapr_drc_reset(spapr_drc_index(drc));
> +
> +g_free(drc->ccs);
> +drc->ccs = NULL;
> +
>  /* immediately upon reset we can safely assume DRCs whose devices
>   * are pending removal can be safely removed, and that they will
>   * subsequently be left in an ISOLATED state. move the DRC to this
> @@ -1020,19 +1006,6 @@ static void rtas_set_indicator(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
> 
>  switch (sensor_type) {
>  case RTAS_SENSOR_TYPE_ISOLATION_STATE:
> -/* if the guest is configuring a device attached to this
> - * DRC, we should reset the configuration state at this
> - * point since it may no longer be reliable (guest released
> - * device and needs to start over, or unplug occurred so
> - * the FDT is no longer valid)
> - */
> -if (sensor_state == SPAPR_DR_ISOLATION_STATE_ISOLATED) {
> -sPAPRConfigureConnectorState *ccs = spapr_ccs_find(spapr,
> -   sensor_index);
> -if (ccs) {
> -spapr_ccs_remove(spapr, ccs);
> -}
> -}
>  ret = drck->set_isolation_state(drc, sensor_state);
>  break;
>  case RTAS_SENSOR_TYPE_DR:
> @@ -1116,16 +1089,6 @@ static void configure_connector_st(target_ulong addr, 
> 

Re: [Qemu-devel] [PATCH 17/19] nbd/common: nbd_tls_handshake: use error_reportf_err instead of TRACE

[Eric Blake]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use error_reportf_err instead of TRACE in case of fail.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/common.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/nbd/common.c b/nbd/common.c
> index 88e0297fb2..574a551abe 100644
> --- a/nbd/common.c
> +++ b/nbd/common.c
> @@ -102,7 +102,7 @@ void nbd_tls_handshake(QIOTask *task,
>  struct NBDTLSHandshakeData *data = opaque;
>  
>  if (qio_task_propagate_error(task, >error)) {
> -TRACE("TLS failed %s", error_get_pretty(data->error));
> +error_reportf_err(data->error, "TLS failed");

I don't think this is right. You already populated >error(), which
means you have the error message available to the caller, and should let
the caller handle the message rather than blindly reporting it here
yourself (especially since if the caller also reports it, you've now
doubled up error messages).  Converting this TRACE() into a proper
tracepoint may be okay, but it may also be sufficient to just delete
this TRACE() since the caller should already be handling the failure.

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 16/19] nbd/server: add errp to nbd_send_reply()

[Eric Blake]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/server.c | 17 +
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 

>  
> -if (nbd_co_send_reply(req, , reply_data_len) < 0) {
> -error_setg(_err, "Failed to send reply");
> +if (nbd_co_send_reply(req, , reply_data_len, _err) < 0) {
> +error_prepend(_err, "Failed to send reply");

In the previous patch, you had a lot of conversions from LOG("message")
to error_prepend(..., "message: ") (note the addition of the ': '
suffix).  Is this addition of error_prepend going to come out right?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH 15/19] nbd/server: use errp instead of LOG

[Eric Blake]

On 05/30/2017 09:30 AM, Vladimir Sementsov-Ogievskiy wrote:
> Move to modern errp scheme from just LOGging errors.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  nbd/server.c | 257 
> ++-
>  1 file changed, 150 insertions(+), 107 deletions(-)
> 

> @@ -143,30 +143,30 @@ static int nbd_negotiate_send_rep_len(QIOChannel *ioc, 
> uint32_t type,
>type, opt, len);
>  
>  magic = cpu_to_be64(NBD_REP_MAGIC);
> -ret = write_sync(ioc, , sizeof(magic), NULL);
> +ret = write_sync(ioc, , sizeof(magic), errp);
>  if (ret < 0) {
> -LOG("write failed (rep magic)");
> +error_prepend(errp, "write failed (rep magic): ");
>  return ret;
>  }

I'm wondering how much we really want to use error_prepend(), or if we
could just get away with using the original error message unchanged.
I'm not saying to rewrite the patch now that you've done the work, so
much as asking aloud whether the additional information in the error
messages will prove useful.

There are definitely some ripple effects from your v2 posting of the
first half of the series that will require you to rebase this, but the
overall idea is sound.

>  /* Send an error reply.
>   * Return -errno on error, 0 on success. */
> -static int GCC_FMT_ATTR(4, 5)
> +static int GCC_FMT_ATTR(5, 6)
>  nbd_negotiate_send_rep_err(QIOChannel *ioc, uint32_t type,
> -   uint32_t opt, const char *fmt, ...)
> +   uint32_t opt, Error **errp, const char *fmt, ...)

Having errp not be the last argument is unusual, but I don't see how you
can do any differently with the var-args nature of the call.

> @@ -505,26 +515,33 @@ static int nbd_negotiate_options(NBDClient *client)
>   * disconnecting, but that we must also tolerate
>   * guests that don't wait for our reply. */
>  ret = nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
> - clientflags);
> -return ret < 0 && ret != -EPIPE ? ret : 1;
> + clientflags, _err);
> +
> +if (ret < 0 && ret != -EPIPE) {
> +error_propagate(errp, local_err);
> +return ret;
> +}
> +
> +error_free(local_err);
> +return 1;

Of course, you'll have to simplify this portion.  This is probably the
one place where you actually DO want to use:

nbd_negotiate_send_rep(client->ioc, NBD_REP_ACK,
   clientflags, NULL)

because you truly do not care whether you had an error.

> @@ -1090,18 +1113,18 @@ static int nbd_co_receive_request(NBDRequestData 
> *req, NBDRequest *request)
>  
>  /* Sanity checks, part 2. */
>  if (request->from + request->len > client->exp->size) {
> -LOG("operation past EOF; From: %" PRIu64 ", Len: %" PRIu32
> -", Size: %" PRIu64, request->from, request->len,
> -(uint64_t)client->exp->size);
> +error_setg(errp, "operation past EOF; From: %" PRIu64 ", Len: %" 
> PRIu32
> +   ", Size: %" PRIu64, request->from, request->len,
> +   (uint64_t)client->exp->size);
>  return request->type == NBD_CMD_WRITE ? -ENOSPC : -EINVAL;

Question - now that we are consistently setting a decent errp, do we
have any callers that care about specific -errno return values, or could
we further simplify the functions by just returning -1 (instead of
worrying about -errno) on failures?


> @@ -1244,21 +1268,33 @@ static coroutine_fn void nbd_trip(void *opaque)

>  
>  reply:
> +if (local_err) {
> +error_report_err(local_err);
> +local_err = NULL;
> +}

This says that after we detect an error, we report it,

> +
> +if (nbd_co_send_reply(req, , reply_data_len) < 0) {
> +error_setg(_err, "Failed to send reply");
> +goto disconnect;
> +}

...but then blindly try to send the reply anyways, forgetting that we
ever detected the original error.  Is that going to bite us?

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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v20 10/30] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Sementsov-Ogievskiy Vladimir]

On 03.06.2017 00:02, John Snow wrote:


On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:

It will be needed in following commits for persistent bitmaps.
If bitmap is loaded from read-only storage (and we can't mark it
"in use" in this storage) corresponding BdrvDirtyBitmap should be
read-only.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block/dirty-bitmap.c | 32 
  block/io.c   |  8 
  blockdev.c   |  6 ++
  include/block/dirty-bitmap.h |  4 
  4 files changed, 50 insertions(+)

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index f25428868c..1c9ffb292a 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -45,6 +45,12 @@ struct BdrvDirtyBitmap {
  bool disabled;  /* Bitmap is disabled. It skips all writes to
 the device */
  int active_iterators;   /* How many iterators are active */
+bool readonly;  /* Bitmap is read-only and may be changed only
+   by deserialize* functions. This field blocks

In what way do the deserialize functions change the bitmaps, again?


Hmm, I mean loading bitmap from file, but actually, bitmap is set 
readonly after loading, so this sentence half can be dropped and asserts 
added to deserialize* functions too (however, deserialize should never 
be applied to the bitmap already loaded, or created from qmp, 
deserialize should be used with newly created bitmap, by the code like 
bitmap-loading or bitmap-incoming-migration, so such asserts may be a 
bit strange)





+   any changing operations on owning image
+   (writes and discards), if bitmap is readonly
+   such operations must fail and not change
+   image or this bitmap */
  QLIST_ENTRY(BdrvDirtyBitmap) list;
  };
  
@@ -437,6 +443,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,

 int64_t cur_sector, int64_t nr_sectors)
  {
  assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));

I still feel as if bdrv_dirty_bitmap_enabled() can return false if
bdrv_dirty_bitmap_readonly is true, and you wouldn't have to edit these
parts, but I don't care enough to press the issue.


  hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
  }
  
@@ -444,12 +451,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,

   int64_t cur_sector, int64_t nr_sectors)
  {
  assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
  hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
  }
  
  void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)

  {
  assert(bdrv_dirty_bitmap_enabled(bitmap));
+assert(!bdrv_dirty_bitmap_readonly(bitmap));
  if (!out) {
  hbitmap_reset_all(bitmap->bitmap);
  } else {
@@ -520,6 +529,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
cur_sector,
  if (!bdrv_dirty_bitmap_enabled(bitmap)) {
  continue;
  }
+assert(!bdrv_dirty_bitmap_readonly(bitmap));

Highlighting the difference in strictness between "disabled" and "readonly."


this case also show that we cant include !readonly-check into 
bdrv_dirty_bitmap_enabled()





  hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
  }
  }
@@ -541,3 +551,25 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
  {
  return hbitmap_count(bitmap->meta);
  }
+
+bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
+{
+return bitmap->readonly;
+}
+
+void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value)
+{
+bitmap->readonly = value;
+}
+
+bool bdrv_has_readonly_bitmaps(BlockDriverState *bs)
+{
+BdrvDirtyBitmap *bm;
+QLIST_FOREACH(bm, >dirty_bitmaps, list) {
+if (bm->readonly) {
+return true;
+}
+}
+
+return false;
+}
diff --git a/block/io.c b/block/io.c
index fdd7485c22..0e28a1f595 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1349,6 +1349,10 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild 
*child,
  uint64_t bytes_remaining = bytes;
  int max_transfer;
  
+if (bdrv_has_readonly_bitmaps(bs)) {

+return -EPERM;
+}
+

Should this be a dynamic error, or an assertion? We should probably
endeavor to never actually hit this circumstance (we should not have
readonly bitmaps on a RW node.)


if on reopening rw write of 'in_use=1' flag failed, we are stay with 
readonly bitmaps in rw image. So this is possible.


two notes:
1. we can't fail bdrv_reopen_commit if we failed writing 'in_use=1' flag 
into image
2. may be this is not bad: user can remove bitmaps to unblock write, or 
he can retry operation leading to reopening rw 

Re: [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Sementsov-Ogievskiy Vladimir]

On 02.06.2017 21:46, John Snow wrote:


On 06/02/2017 05:45 AM, Vladimir Sementsov-Ogievskiy wrote:

02.06.2017 12:01, Vladimir Sementsov-Ogievskiy wrote:

02.06.2017 11:56, Vladimir Sementsov-Ogievskiy wrote:

02.06.2017 02:25, John Snow wrote:

On 06/01/2017 03:30 AM, Sementsov-Ogievskiy Vladimir wrote:

Hi John!

Look at our discussion about this in v18 thread.

Shortly: readonly is not the same as disabled. disabled= bitmap just

ignores all writes. readonly= writes are not allowed at all.

And I think, I'll try to go through way 2: "dirty" field instead of
"readonly" (look at v18 discussion), as it a bit more flexible.


Not sure which I prefer...

Method 1 is attractive in that it is fairly simple, and enforces fairly
loudly the inability to write to devices with RO bitmaps. It's a
natural
extension of your current approach.

For now I decided to realize this one, I think I'll publish it today.
Also, I'm going to rename s/readonly/in_use - to avoid the confuse
with disabled. So let this field just be mirror of IN_USE in the
image and just say "persistent storage knows, that bitmap is in use
and may be dirty".

Finally it would be readonly. in_use is bad for created (not loaded)
bitmaps. I'll add more descriptive comments for disabled and readonly.


Makes sense. It sounds like "readonly" is simply a stricter superset of
"disabled," where "disabled" doesn't care if the bitmap gets out of sync
with the data, but "readonly" attempts to preserve that semantic
relationship.


should we add separate "READONLY" bitmap status for qapi? I'm sure that 
I don't want to call them "disabled" as it is not the same. "active" is 
better (as when image is RW they are active, when image is RO, they are 
active in RO image - not bad too I think. So, may be such addition to 
qapi would be redundant.





Also, optimization with 'dirty' flag may be added later.

Yes, I agree.


And, also, I don't want to influence this "first write", on which we
will set "IN_USE" in all bitmaps (for way (2). Reopening rw is less
performance-demanding place than write.


"And, also," -- I think you've been reading my emails too much, you're
picking up my 'isms ;)

Thanks,
--John


--
Best regards,
Vladimir.

Re: [Qemu-devel] [PATCH v20 13/30] block: new bdrv_reopen_bitmaps_rw interface

[Sementsov-Ogievskiy Vladimir]

On 03.06.2017 01:17, John Snow wrote:


On 06/02/2017 07:21 AM, Vladimir Sementsov-Ogievskiy wrote:

Add format driver handler, which should mark loaded read-only
bitmaps as 'IN_USE' in the image and unset read_only field in
corresponding BdrvDirtyBitmap's.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c   | 17 +
  include/block/block_int.h |  7 +++
  2 files changed, 24 insertions(+)

diff --git a/block.c b/block.c
index 04af7697dc..161db9e32a 100644
--- a/block.c
+++ b/block.c
@@ -2946,12 +2946,16 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
  {
  BlockDriver *drv;
  BlockDriverState *bs;
+bool old_can_write, new_can_write;
  
  assert(reopen_state != NULL);

  bs = reopen_state->bs;
  drv = bs->drv;
  assert(drv != NULL);
  
+old_can_write =

+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+
  /* If there are any driver level actions to take */
  if (drv->bdrv_reopen_commit) {
  drv->bdrv_reopen_commit(reopen_state);
@@ -2965,6 +2969,19 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
  bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
  
  bdrv_refresh_limits(bs, NULL);

+
+new_can_write =
+!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
+if (!old_can_write && new_can_write && drv->bdrv_reopen_bitmaps_rw) {
+Error *local_err = NULL;
+if (drv->bdrv_reopen_bitmaps_rw(bs, _err) < 0) {
+/* This is not fatal, bitmaps just left read-only, so all following
+ * writes will fail. User can remove read-only bitmaps to unblock
+ * writes.
+ */
+error_report_err(local_err);
+}
+}
  }
  
  /*

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8d3724cce6..1dc6f2e90d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -380,6 +380,13 @@ struct BlockDriver {
   uint64_t parent_perm, uint64_t parent_shared,
   uint64_t *nperm, uint64_t *nshared);
  
+/**

+ * Bitmaps should be marked as 'IN_USE' in the image on reopening image
+ * as rw. This handler should realize it. It also should unset readonly
+ * field of BlockDirtyBitmap's in case of success.
+ */
+int (*bdrv_reopen_bitmaps_rw)(BlockDriverState *bs, Error **errp);
+

Hmm, do we need a new top-level hook for this? We already have
.bdrv_reopen_commit and .bdrv_reopen_prepare which inform the
blockdriver that a reopen event is occurring.

Can't we just amend qcow2_reopen_prepare and qcow2_reopen_commit to do
the necessary tasks of either:

(A) Flushing the bitmap in preparation for reopening as RO, or
(B) Writing in_use and removing the RO flag in preparation for reopening
as RW


(A) is done (see patch about reopen RO)

(B) - We can't do it, as on this preparation the image is still RO, and 
I've created new handler, write 'in_use' _after_ the point when new 
flags was set.





  QLIST_ENTRY(BlockDriver) list;
  };
  



Well, design issues aside, I will at least confirm that I think this
patch should work as designed, and I will leave the design critiques to
Max and Kevin:

Reviewed-by: John Snow 



--
Best regards,
Vladimir.

Re: [Qemu-devel] [PATCH] configure: Define NCURSES_WIDECHAR if we're using curses

[Laszlo Ersek]

On 06/03/17 11:43, Kamil Rytarowski wrote:
> On 02.06.2017 23:58, Laszlo Ersek wrote:
>> On 06/02/17 16:35, Peter Maydell wrote:
>>> We want the wide character functions from the ncurses header.
>>> Unfortunately it doesn't provide them by default, but only
>>> if either:
>>>  * NCURSES_WIDECHAR is defined (for ncurses 20111030 and up)
>>>  * _XOPEN_SOURCE/_XOPEN_SOURCE_EXTENDED are suitably defined
>>>
>>> So far we have been implicitly relying on the latter, because
>>> for GNU libc when we define _GNU_SOURCE this causes libc
>>> to define the _XOPEN_SOURCE macros for us. Unfortunately
>>> this doesn't work on all libcs, because some (like OSX and
>>> musl libc) do not define _XOPEN_SOURCE when _GNU_SOURCE
>>> is defined.
>>>
>>> We can't fix this by defining _XOPEN_SOURCE ourselves, because
>>> that also means "and don't provide any functions that aren't in
>>> that standard", and not all libcs provide any way to override
>>> that to also get the non-standard functions. In particular
>>> FreeBSD has no such mechanism, and OSX's _DARWIN_C_SOURCE
>>> doesn't reenable everything (for instance getpagesize()
>>> is still not prototyped if _DARWIN_C_SOURCE and _XOPEN_SOURCE
>>> are both defined).
>>>
>>> So we have to define NCURSES_WIDECHAR. (This will only work
>>> if your ncurses is at least 20111030, as older versions
>>> don't honour this macro.)
>>>
>>> Signed-off-by: Peter Maydell 
>>> ---
>>> Testing from the people with musl libc and OSX-with-ncurses
>>> appreciated, as I don't have any systems which have the bug
>>> which this patch is attempting to fix...
>>>
>>>  configure | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/configure b/configure
>>> index 0586ec9..6aca5d1 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -3053,6 +3053,8 @@ int main(void) {
>>>  EOF
>>>IFS=:
>>>for curses_inc in $curses_inc_list; do
>>> +# Make sure we get the wide character prototypes
>>> +curses_inc="-DNCURSES_WIDECHAR $curses_inc"
>>>  IFS=:
>>>  for curses_lib in $curses_lib_list; do
>>>unset IFS
>>>
>>
>> Given that we're already consciously using non-portable functions,
>> this solution looks the least messy to me.
>>
>
> These functions are enough portable to work on NetBSD curses(3).

Sorry, that's not what I meant by "non-portable functions".

Peter wrote,

>>> We can't fix this by defining _XOPEN_SOURCE ourselves, because
>>> that also means "and don't provide any functions that aren't in
>>> that standard", and not all libcs provide any way to override
>>> that to also get the non-standard functions.

So basically, if we only used standard (SUSv2, SUSv3 or SUSv4, i.e.
_XOPEN_SOURCE=500, =600 or =700) functions, then the wide char curses
functions could also be declared just through _XOPEN_SOURCE:

  http://pubs.opengroup.org/onlinepubs/007908799/xcurses/implement.html

(

Note that X/Open Curses, Issue 4 Version 2, does provide the wide char
stuff; see for example the add_wch() function at
,
and my "/usr/include/curses.h" has:

  #ifndef NCURSES_WIDECHAR
  #if defined(_XOPEN_SOURCE_EXTENDED) || (defined(_XOPEN_SOURCE) && 
(_XOPEN_SOURCE - 0 >= 500))
  #define NCURSES_WIDECHAR 1
  #else
  #define NCURSES_WIDECHAR 0
  #endif
  #endif /* NCURSES_WIDECHAR */
  ...
  #if NCURSES_WIDECHAR
  ...
  extern NCURSES_EXPORT(int) add_wch (const cchar_t *);   /* 
generated:WIDEC */

)

Since we can't use _XOPEN_SOURCE (because of our reliance on non-std
functions), we have to go with "whatever works" (basically abandoning
any benefit that the SUS / POSIX standardization brings), and then
NCURSES_WIDECHAR looks simple enough.

Thanks
Laszlo

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Stefan Berger]

On 06/02/2017 07:20 PM, Laszlo Ersek wrote:

On 06/02/17 18:30, Michael S. Tsirkin wrote:

On Fri, Jun 02, 2017 at 05:45:21PM +0200, Laszlo Ersek wrote:

Hi,

this message is cross-posted to three lists (qemu, seabios, edk2). I'll
follow up with three patch series, one series for each project. I'll
cross-post all of the patches as well, but I'll add the project name in
the "bag of tags" in the subject lines.

The QEMU series introduces two extensions to the ALLOCATE firmware
linker/loader command.

One extension is a new allocation zone, with value 3, for allowing the
firmware to allocate the fw_cfg blobs in 64-bit address space.

Seems to make sense. I guess it's safe to do this if no
pointers to this table are 32 bit, right?

That's right. For example, the TCPA patch (6 of 7) in the QEMU series
does this, because the ACPI_BUILD_TPMLOG_FILE is only referenced by a
64-bit pointer.


Is there a chance we'll ever be able to use this on PC
assuming the need to support 32 bit guests?

Well, sticking with the TCPA example, if an ACPI table defines *only* an
8-byte pointer to some memory area, that seems to preclude support for
32-bit guests already, generally speaking, no?


I just tested this, giving 8G of memory to a VM and running i386 Fedora 
in it. The memory allocated for the TCPA log seems to be in 32bit 
memory, so not out of reach of i386 guests. I guess it's important what 
the firmware does with it, whether it strictly follows the 64bit and 
allocates memory as far up as possible or provides compatibility. 
SeaBIOS (1.10.0) seems to do the right thing.


   Stefan

Re: [Qemu-devel] [PATCH] spapr: Allow boot from vhost-*-scsi backends

[David Gibson]

On Thu, Jun 01, 2017 at 08:51:58AM +0100, Felipe Franciosi wrote:
> This makes VMs bootable on spapr when using vhost-*-scsi.

This commit message needs more information: what previously prevented
vhost-*-scsi from working?

> Signed-off-by: Felipe Franciosi 
> Signed-off-by: Mike Cui 
> ---
>  hw/ppc/spapr.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index ab3aab1..1c87886 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -57,6 +57,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/scsi/scsi.h"
>  #include "hw/virtio/virtio-scsi.h"
> +#include "hw/virtio/vhost-scsi-common.h"
>  
>  #include "exec/address-spaces.h"
>  #include "hw/usb.h"
> @@ -2388,6 +2389,7 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, 
> BusState *bus,
>  ((type *)object_dynamic_cast(OBJECT(obj), (name)))
>  SCSIDevice *d = CAST(SCSIDevice,  dev, TYPE_SCSI_DEVICE);
>  sPAPRPHBState *phb = CAST(sPAPRPHBState, dev, 
> TYPE_SPAPR_PCI_HOST_BRIDGE);
> +VHostSCSICommon *vsc = CAST(VHostSCSICommon, dev, 
> TYPE_VHOST_SCSI_COMMON);
>  
>  if (d) {
>  void *spapr = CAST(void, bus->parent, "spapr-vscsi");
> @@ -2444,6 +2446,12 @@ static char *spapr_get_fw_dev_path(FWPathProvider *p, 
> BusState *bus,
>  return g_strdup_printf("pci@%"PRIX64, phb->buid);
>  }
>  
> +if (vsc) {
> +/* Same logic as virtio above */
> +unsigned id = 0x100 | (vsc->target << 16) | vsc->lun;
> +return g_strdup_printf("disk@%"PRIX64, (uint64_t)id << 32);
> +}
> +
>  return NULL;
>  }
>  

-- 
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH v3 1/4] ACPI: Add APEI GHES Table Generation support

[Laszlo Ersek]

On 05/22/17 16:23, Laszlo Ersek wrote:
> Keeping some context:
> 
> On 05/12/17 23:00, Laszlo Ersek wrote:
>> On 04/30/17 07:35, Dongjiu Geng wrote:

> (68) In the code below, you are not taking an "OVMF header probe
> suppressor" into account.
> 
> But, we have already planned to replace that quirk with a separate,
> dedicated allocation hint or command, so I'm not going to describe what
> an "OVMF header probe suppressor" is; instead, I'll describe the
> replacement for it.
> 
> [...]

So, the NOACPI allocation hint is a no-go at the moment, based on the
discussion in the following threads:

http://mid.mail-archive.com/20170601112241.2580-1-ard.biesheuvel@linaro.org

c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com">http://mid.mail-archive.com/c76b36de-ebf9-c662-d454-0a95b43901e8@redhat.com

Therefore the header probe suppression remains necessary.

In this case, it is not hard to do, you just have to reorder the
following two ADD_POINTER additions a bit:

>>> +
>>> +bios_linker_loader_add_pointer(linker, GHES_ERRORS_FW_CFG_FILE,
>>> +sizeof(uint64_t) * i, sizeof(uint64_t),
>>> +GHES_ERRORS_FW_CFG_FILE,
>>> +MAX_ERROR_SOURCE_COUNT_V6 * 
>>> sizeof(uint64_t) +
>>> +i * MAX_RAW_DATA_LENGTH);

This one should be moved out to a separate loop, after the current loop.

>>> +bios_linker_loader_add_pointer(linker, ACPI_BUILD_TABLE_FILE,
>>> +address_registers_offset
>>> ++ i * sizeof(AcpiGenericHardwareErrorSource),
>>> +sizeof(uint32_t), GHES_ERRORS_FW_CFG_FILE,
>>> +i * sizeof(uint64_t));

This one should be kept in the first (i.e., current) loop.

The idea is, when you first point the HEST/GHES_n entries in
ACPI_BUILD_TABLE_FILE to the "address registers" in
GHES_ERRORS_FW_CFG_FILE, all those address registers will still be
zero-filled. This will fail the ACPI table header probe in
OvmfPkg/AcpiPlatformDxe, which is what we want.

After this is done, the address registers in GHES_ERRORS_FW_CFG_FILE
should be pointed to the error status data blocks in the same fw_cfg
blob, in a separate loop. (Those error status data blocks will again be
zero-filled, so no ACPI table headers will be mistakenly recognized in
them.)

Thanks
Laszlo

Re: [Qemu-devel] [PATCH v4 6/8] vmdk: New functions to assist allocating multiple clusters

[Ashijeet Acharya]

On Thu, Jun 1, 2017 at 7:27 PM, Fam Zheng  wrote:
> On Sat, 04/22 10:43, Ashijeet Acharya wrote:
>> Introduce two new helper functions handle_alloc() and
>> vmdk_alloc_cluster_offset(). handle_alloc() helps to allocate multiple
>> clusters at once starting from a given offset on disk and performs COW
>> if necessary for first and last allocated clusters.
>> vmdk_alloc_cluster_offset() helps to return the offset of the first of
>> the many newly allocated clusters. Also, provide proper documentation
>> for both.
>>
>> Signed-off-by: Ashijeet Acharya 
>> ---
>>  block/vmdk.c | 192 
>> +++
>>  1 file changed, 182 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/vmdk.c b/block/vmdk.c
>> index 7862791..8d34cd9 100644
>> --- a/block/vmdk.c
>> +++ b/block/vmdk.c
>> @@ -136,6 +136,7 @@ typedef struct VmdkMetaData {
>>  unsigned int l2_offset;
>>  int valid;
>>  uint32_t *l2_cache_entry;
>> +uint32_t nb_clusters;
>>  } VmdkMetaData;
>>
>>  typedef struct VmdkGrainMarker {
>> @@ -1242,6 +1243,174 @@ static int get_cluster_table(VmdkExtent *extent, 
>> uint64_t offset,
>>  return VMDK_OK;
>>  }
>>
>> +/*
>> + * handle_alloc
>> + *
>> + * Allocate new clusters for an area that either is yet unallocated or 
>> needs a
>> + * copy on write. If *cluster_offset is non_zero, clusters are only 
>> allocated if
>> + * the new allocation can match the specified host offset.
>> + *
>> + * Returns:
>> + *   VMDK_OK:   if new clusters were allocated, *bytes may be decreased 
>> if
>> + *  the new allocation doesn't cover all of the requested 
>> area.
>> + *  *cluster_offset is updated to contain the offset of the
>> + *  first newly allocated cluster.
>> + *
>> + *   VMDK_UNALLOC:  if no clusters could be allocated. *cluster_offset is 
>> left
>> + *  unchanged.
>> + *
>> + *   VMDK_ERROR:in error cases
>> + */
>> +static int handle_alloc(BlockDriverState *bs, VmdkExtent *extent,
>> +uint64_t offset, uint64_t *cluster_offset,
>> +int64_t *bytes, VmdkMetaData *m_data,
>> +bool allocate, uint32_t *total_alloc_clusters)
>
> Not super important but personally I always prefer to stick to a "vmdk_" 
> prefix
> when naming local identifiers, so that ctags and git grep can take it easy.

Done.

>
>> +{
>> +int l1_index, l2_offset, l2_index;
>> +uint32_t *l2_table;
>> +uint32_t cluster_sector;
>> +uint32_t nb_clusters;
>> +bool zeroed = false;
>> +uint64_t skip_start_bytes, skip_end_bytes;
>> +int ret;
>> +
>> +ret = get_cluster_table(extent, offset, _index, _offset,
>> +_index, _table);
>> +if (ret < 0) {
>> +return ret;
>> +}
>> +
>> +cluster_sector = le32_to_cpu(l2_table[l2_index]);
>> +
>> +skip_start_bytes = vmdk_find_offset_in_cluster(extent, offset);
>> +/* Calculate the number of clusters to look for. Here we truncate the 
>> last
>> + * cluster, i.e. 1 less than the actual value calculated as we may need 
>> to
>> + * perform COW for the last one. */
>> +nb_clusters = DIV_ROUND_UP(skip_start_bytes + *bytes,
>> +extent->cluster_sectors << BDRV_SECTOR_BITS) - 
>> 1;
>
> Alignment could be improved: here ^

Done.

>
>> +
>> +nb_clusters = MIN(nb_clusters, extent->l2_size - l2_index);
>> +assert(nb_clusters <= INT_MAX);
>> +
>> +/* update bytes according to final nb_clusters value */
>> +if (nb_clusters != 0) {
>> +*bytes = ((nb_clusters * extent->cluster_sectors) << 9)
>
> Better use BDRV_SECTOR_BITS instead of 9.

Done.

>
>> +- skip_start_bytes;
>> +} else {
>> +nb_clusters = 1;
>> +}
>> +*total_alloc_clusters += nb_clusters;
>
> It is weird that you increment *total_alloc_clusters instead of simply 
> assigning
> to it, because it's not clear why before reading the caller code.
>
I am incrementing it because we allocate clusters in every iteration
and *total_alloc_clusters contains number of clusters allocated in
total and not just in one iteration. So basically it is a sum of all
the m_data->nb_clusters present in every element of the linked list I
prepare.

> It's better if you just return nb_clusters from this function (either as a
> return value, or assign to *total_alloc_clusters), then do the accumulation in
> vmdk_pwritev by adding m_data->nb_clusters, which is simpler.

If I understood it correctly, you mean to say that I should add all
the m_data->nb_clusters present in my linked list inside
vmdk_pwritev() using a loop? If yes, I think that's what I have been
doing earlier by incrementing *total_alloc_clusters there itself and
is better, no?
Also, please correct me if I am wrong :-)

Ashijeet

Re: [Qemu-devel] [PATCH] configure: Define NCURSES_WIDECHAR if we're using curses

[Kamil Rytarowski]

On 03.06.2017 12:13, Rainer Müller wrote:
> On 2017-06-02 16:35, Peter Maydell wrote:
>> diff --git a/configure b/configure
>> index 0586ec9..6aca5d1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3053,6 +3053,8 @@ int main(void) {
>>  EOF
>>IFS=:
>>for curses_inc in $curses_inc_list; do
>> +# Make sure we get the wide character prototypes
>> +curses_inc="-DNCURSES_WIDECHAR $curses_inc"
>>  IFS=:
>>  for curses_lib in $curses_lib_list; do
>>unset IFS
>>
> 
> Thank you for getting back to this. I can confirm that this patch fixes
> --enable-curses for me on Mac OS X.
> 
> Although this already works as is, I would use -DNCURSES_WIDECHAR=1 as
> ncurses.h uses #if and not #ifdef to check for this.
> 

-DNCURSES_WIDECHAR evaluates to 1 for #if

> Rainer
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH] configure: Define NCURSES_WIDECHAR if we're using curses

[Rainer Müller]

On 2017-06-02 16:35, Peter Maydell wrote:
> diff --git a/configure b/configure
> index 0586ec9..6aca5d1 100755
> --- a/configure
> +++ b/configure
> @@ -3053,6 +3053,8 @@ int main(void) {
>  EOF
>IFS=:
>for curses_inc in $curses_inc_list; do
> +# Make sure we get the wide character prototypes
> +curses_inc="-DNCURSES_WIDECHAR $curses_inc"
>  IFS=:
>  for curses_lib in $curses_lib_list; do
>unset IFS
> 

Thank you for getting back to this. I can confirm that this patch fixes
--enable-curses for me on Mac OS X.

Although this already works as is, I would use -DNCURSES_WIDECHAR=1 as
ncurses.h uses #if and not #ifdef to check for this.

Rainer

Re: [Qemu-devel] [PATCH] configure: Define NCURSES_WIDECHAR if we're using curses

[Kamil Rytarowski]

On 02.06.2017 23:58, Laszlo Ersek wrote:
> On 06/02/17 16:35, Peter Maydell wrote:
>> We want the wide character functions from the ncurses header.
>> Unfortunately it doesn't provide them by default, but only
>> if either:
>>  * NCURSES_WIDECHAR is defined (for ncurses 20111030 and up)
>>  * _XOPEN_SOURCE/_XOPEN_SOURCE_EXTENDED are suitably defined
>>
>> So far we have been implicitly relying on the latter, because
>> for GNU libc when we define _GNU_SOURCE this causes libc
>> to define the _XOPEN_SOURCE macros for us. Unfortunately
>> this doesn't work on all libcs, because some (like OSX and
>> musl libc) do not define _XOPEN_SOURCE when _GNU_SOURCE
>> is defined.
>>
>> We can't fix this by defining _XOPEN_SOURCE ourselves, because
>> that also means "and don't provide any functions that aren't in
>> that standard", and not all libcs provide any way to override
>> that to also get the non-standard functions. In particular
>> FreeBSD has no such mechanism, and OSX's _DARWIN_C_SOURCE
>> doesn't reenable everything (for instance getpagesize()
>> is still not prototyped if _DARWIN_C_SOURCE and _XOPEN_SOURCE
>> are both defined).
>>
>> So we have to define NCURSES_WIDECHAR. (This will only work
>> if your ncurses is at least 20111030, as older versions
>> don't honour this macro.)
>>
>> Signed-off-by: Peter Maydell 
>> ---
>> Testing from the people with musl libc and OSX-with-ncurses
>> appreciated, as I don't have any systems which have the bug
>> which this patch is attempting to fix...
>>
>>  configure | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/configure b/configure
>> index 0586ec9..6aca5d1 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3053,6 +3053,8 @@ int main(void) {
>>  EOF
>>IFS=:
>>for curses_inc in $curses_inc_list; do
>> +# Make sure we get the wide character prototypes
>> +curses_inc="-DNCURSES_WIDECHAR $curses_inc"
>>  IFS=:
>>  for curses_lib in $curses_lib_list; do
>>unset IFS
>>
> 
> Given that we're already consciously using non-portable functions, this
> solution looks the least messy to me.
> 

These functions are enough portable to work on NetBSD curses(3).

pkgsrc had an equivalent patch to define NCURSES_WIDECHAR=1 to fix build
on Darwin.

> Acked-by: Laszlo Ersek 
> 
> Thanks
> Laszlo
> 




signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [PATCH] target/xtensa: handle unknown registers in gdbstub

[Max Filippov]

Xtensa cores may have registers of types/sizes not supported by the
gdbstub accessors. Ignore writes to such registers and return zero on
read, but always return correct register size, so that gdb on the other
side is able to access all registers in the packet holding unsupported
registers in the middle. This fixes gdb interaction with cores that have
vector/custom TIE registers.

Cc: qemu-sta...@nongnu.org
Signed-off-by: Max Filippov 
---
 target/xtensa/gdbstub.c | 13 ++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c
index fa5469a..da131ae 100644
--- a/target/xtensa/gdbstub.c
+++ b/target/xtensa/gdbstub.c
@@ -58,7 +58,10 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, uint8_t 
*mem_buf, int n)
 case 8:
 return gdb_get_reg64(mem_buf, float64_val(env->fregs[i].f64));
 default:
-return 0;
+qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported size %d\n",
+  __func__, n, reg->size);
+memset(mem_buf, 0, reg->size);
+return reg->size;
 }
 
 case 8: /*a*/
@@ -67,6 +70,8 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, uint8_t 
*mem_buf, int n)
 default:
 qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
   __func__, n, reg->type);
+memset(mem_buf, 0, reg->size);
+return reg->size;
 return 0;
 }
 }
@@ -111,7 +116,9 @@ int xtensa_cpu_gdb_write_register(CPUState *cs, uint8_t 
*mem_buf, int n)
 env->fregs[reg->targno & 0x0f].f64 = make_float64(tmp);
 return 8;
 default:
-return 0;
+qemu_log_mask(LOG_UNIMP, "%s to reg %d of unsupported size %d\n",
+  __func__, n, reg->size);
+return reg->size;
 }
 
 case 8: /*a*/
@@ -121,7 +128,7 @@ int xtensa_cpu_gdb_write_register(CPUState *cs, uint8_t 
*mem_buf, int n)
 default:
 qemu_log_mask(LOG_UNIMP, "%s to reg %d of unsupported type %d\n",
   __func__, n, reg->type);
-return 0;
+return reg->size;
 }
 
 return 4;
-- 
2.1.4

Re: [Qemu-devel] allocation zone extensions for the firmware linker/loader

[Laszlo Ersek]

On 06/02/17 17:45, Laszlo Ersek wrote:

> The patches can cause linker/loader breakage when old firmware is booted
> on new QEMU. However, that's no problem (it's nothing new), the next
> release of QEMU should bundle the new firmware binaries as always.

Dave made a good point (which I should have realized myself, really!),
namely if you launch old fw on old qemu, then migrate the guest to a new
qemu and then reboot the guest on the target host, within the migrated
VM, things will break.

So that makes this approach dead in the water.

Possible mitigations I could think of:
- Make it machine type dependent. Complicated (we don't usually bind
ACPI generation to machine types) and wouldn't help existing devices.
- Let the firmware negotiate these extensions. Very complicated (new
fw-cfg files are needed for negotiation) and wouldn't help existing devices.

So I guess I'll do what Igor and Gerd suggested: record in advance
whether any pointer field narrower than 8 bytes points into a given
blob, and if so, forbid allocating that blob from 64-bit address space.
This should solve Ard's needs purely within the firmware.

Regarding the NOACPI hint, I guess I'm dropping that. I only meant
NOACPI for addressing Igor's long-standing dislike for the "ACPI SDT
header probe suppression" in VMGENID (and future similar devices). But,
there's no actual *technical* need to eliminate that (unlike the
technical need for 64-bit blob allocations which should really be
solved), so I guess it's OK to postpone NOACPI indefinitely.

Self-nack for this set of sets.

Thanks for the feedback,
Laszlo