[Qemu-devel] [PATCH v6 0/7] KASLR kernel dump support

[Qemu-devel] [PATCH v6 0/7] KASLR kernel dump support

[Marc-André Lureau]

Recent linux kernels enable KASLR to randomize phys/virt memory
addresses. This series aims to provide enough information in qemu
dumps so that crash utility can work with randomized kernel too (it
hasn't been tested on other archs than x86 though, help welcome).

The previous design to provide qemu with debug details (using qemu-ga
and a dedicated vmcoreinfo ACPI device) failed to satisfy the
requirements during previous iterations.

In particular, the previous proposed vmcoreinfo ACPI device had the
following issues:
- hazardous memory handling with no explicit synchronization
- occupy 2 fw-cfg entries (for memory and pointer)
- occupy 4k of guest memory (this could have been tweaked)
- did not provide ACPI methods (this could have been added)
- may be difficult to maintain compatibility (according to Michael)

This is a new proposal, that leverage fw-cfg device instead of adding
a new device. A "etc/vmcoreinfo" entry is added, where the guest,
during boot or later, can write the addr/size location of an ELF note
to be appended in the qemu dump.

Note: only guest kernel is expected to write to a fw-cfg entry.  This
method is not meant for general qemu/user-space communication. There
are more appropriate devices for this purpose, and the guest kernel
should not expose this facility.

This is quite easier to implement, and uses less of the limited fw-cfg
slots, and guest memory. It also solves the synchronization issue, and
may be easier to discover or to maintain compatibility.

The Linux ELF note is expected to be the VMCOREINFO note, which will
have a special handling in qemu in this case helping kaslr-kernel
debugging. But it could be any valid ELF note.

Crash 7.1.9 will parse the "phys_base" value from the VMCOREINFO note,
and thus will work with KASLR-dump produced by this series.

The series implements the note addition in qemu ELF/kdump,
as well as the python scripts/dump-guest-memory.py.

To test:

Using kernel from https://github.com/elmarco/linux fw-cfg branch,
Compile and run guest kernel with CONFIG_RANDOMIZE_BASE=y & CONFIG_FW_CFG_SYSFS=y.

Run qemu with -device vmcoreinfo

Produce an ELF dump:
{ "execute": "dump-guest-memory", "arguments": { "protocol": "file:dump", "paging": false } }

Produce a kdump:
{ "execute": "dump-guest-memory", "arguments": { "protocol": "file:dump", "paging": false, "format": "kdump-zlib" } }

Or with (gdb) dump-guest-memory, with scripts/dump-guest-memory.py script.

Analyze with crash >= 7.1.9 (or the git version for 4.13 fixes..):

$ crash vmlinux dump

v6: after Michael Tsirkin review
- rebased
- modify fw_cfg write callback, called for all write
- back to a seperate -device vmcoreinfo
- add host_format/guest_format fields
- clear/reset fw_cfg entry values on reset
- write 0 as guest format to disable device

v5:
- removed x-write-pointer-available patch from this series
- drop vmcoreinfo device
- add write callback to fw_cfg entries
- add a writable fw_cfg "vmcoreinfo" entry
- split phys_base update from VMCOREINFO note in a seperate patch
- most patches had non-trivial changes, dropping reviewed-by tags

v4: from Laszlo review
- switch to warn_report*()
- update test to follow vmgenid and use boot-sector infrastructure
- fix range checks in the python script
- add vmcoreinfo_get() stub

v3: from Laszlo review
- change vmcoreinfo offset to 36
- reset err to null after report
- use PRIu32
- change name_size and desc_size against MAX_VMCOREINFO_SIZE
- python code simplification
- check boundaries of blocks in phys_memory_read()
- fix some vmgi vs vmci names
- add more comments in code
- fix comment indentation
- add r-b tags

v2: from Laszlo review
- vmci: fix guest endianess handling
- vmci: fix wrong sizeof()
- vmci: add back reset logic from vmgenid
- dump: have 1MB size limit for vmcoreinfo
- dump: fix potential off-by-1 buffer manipulation
- dump: use temporary variable for qemu_strtou64
- dump: fixed VMCOREINFO duplication in kdump
- update gdb script to not call into qemu process
- update MAINTAINERS with some new files

Marc-André Lureau (7):
  fw_cfg: add write callback
  hw/misc: add vmcoreinfo device
  dump: add guest ELF note
  dump: update phys_base header field based on VMCOREINFO content
  kdump: set vmcoreinfo location
  scripts/dump-guest-memory.py: add vmcoreinfo
  MAINTAINERS: add Dump maintainers

 scripts/dump-guest-memory.py |  61 +++++++++++++++
 include/hw/misc/vmcoreinfo.h |  46 +++++++++++
 include/hw/nvram/fw_cfg.h    |   3 +
 include/sysemu/dump.h        |   2 +
 dump.c                       | 183 +++++++++++++++++++++++++++++++++++++++++++
 hw/acpi/vmgenid.c            |   2 +-
 hw/core/loader.c             |   2 +-
 hw/i386/acpi-build.c         |   2 +-
 hw/isa/lpc_ich9.c            |   4 +-
 hw/misc/vmcoreinfo.c         |  96 +++++++++++++++++++++++
 hw/nvram/fw_cfg.c            |  14 +++-
 MAINTAINERS                  |  11 +++
 docs/specs/vmcoreinfo.txt    |  49 ++++++++++++
 hw/misc/Makefile.objs        |   1 +
 14 files changed, 467 insertions(+), 9 deletions(-)
 create mode 100644 include/hw/misc/vmcoreinfo.h
 create mode 100644 hw/misc/vmcoreinfo.c
 create mode 100644 docs/specs/vmcoreinfo.txt

-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 1/7] fw_cfg: add write callback

[Marc-André Lureau]

Reintroduce the write callback that was removed when write support was
removed in commit 023e3148567ac898c7258138f8e86c3c2bb40d07.

Contrary to the previous callback implementation, the write_cb
callback is called whenever a write happened, so handlers must be
ready to handle partial write as necessary.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/nvram/fw_cfg.h |  3 +++
 hw/acpi/vmgenid.c         |  2 +-
 hw/core/loader.c          |  2 +-
 hw/i386/acpi-build.c      |  2 +-
 hw/isa/lpc_ich9.c         |  4 ++--
 hw/nvram/fw_cfg.c         | 14 ++++++++++----
 6 files changed, 18 insertions(+), 9 deletions(-)

diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f50d068563..7ccbae5fba 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -45,6 +45,7 @@ typedef struct FWCfgDmaAccess {
 } QEMU_PACKED FWCfgDmaAccess;
 
 typedef void (*FWCfgCallback)(void *opaque);
+typedef void (*FWCfgWriteCallback)(void *opaque, off_t start, size_t len);
 
 struct FWCfgState {
     /*< private >*/
@@ -183,6 +184,7 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
  * @s: fw_cfg device being modified
  * @filename: name of new fw_cfg file item
  * @select_cb: callback function when selecting
+ * @write_cb: callback function after a write
  * @callback_opaque: argument to be passed into callback function
  * @data: pointer to start of item data
  * @len: size of item data
@@ -202,6 +204,7 @@ void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
  */
 void fw_cfg_add_file_callback(FWCfgState *s, const char *filename,
                               FWCfgCallback select_cb,
+                              FWCfgWriteCallback write_cb,
                               void *callback_opaque,
                               void *data, size_t len, bool read_only);
 
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index 2876d8a639..105044f666 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -124,7 +124,7 @@ void vmgenid_add_fw_cfg(VmGenIdState *vms, FWCfgState *s, GArray *guid)
     fw_cfg_add_file(s, VMGENID_GUID_FW_CFG_FILE, guid->data,
                     VMGENID_FW_CFG_SIZE);
     /* Create a read-write fw_cfg file for Address */
-    fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL,
+    fw_cfg_add_file_callback(s, VMGENID_ADDR_FW_CFG_FILE, NULL, NULL, NULL,
                              vms->vmgenid_addr_le,
                              ARRAY_SIZE(vms->vmgenid_addr_le), false);
 }
diff --git a/hw/core/loader.c b/hw/core/loader.c
index 4593061445..91669d65aa 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1023,7 +1023,7 @@ MemoryRegion *rom_add_blob(const char *name, const void *blob, size_t len,
         }
 
         fw_cfg_add_file_callback(fw_cfg, fw_file_name,
-                                 fw_callback, callback_opaque,
+                                 fw_callback, NULL, callback_opaque,
                                  data, rom->datasize, read_only);
     }
     return mr;
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4d19d91e1b..702a214369 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -2890,7 +2890,7 @@ void acpi_setup(void)
 
         build_state->rsdp = g_memdup(tables.rsdp->data, rsdp_size);
         fw_cfg_add_file_callback(pcms->fw_cfg, ACPI_BUILD_RSDP_FILE,
-                                 acpi_build_update, build_state,
+                                 acpi_build_update, NULL, build_state,
                                  build_state->rsdp, rsdp_size, true);
         build_state->rsdp_mr = NULL;
     } else {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index ac8416d42b..de8fbb7260 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -402,12 +402,12 @@ void ich9_lpc_pm_init(PCIDevice *lpc_pci, bool smm_enabled)
          * just link them into fw_cfg here.
          */
         fw_cfg_add_file_callback(fw_cfg, "etc/smi/requested-features",
-                                 NULL, NULL,
+                                 NULL, NULL, NULL,
                                  lpc->smi_guest_features_le,
                                  sizeof lpc->smi_guest_features_le,
                                  false);
         fw_cfg_add_file_callback(fw_cfg, "etc/smi/features-ok",
-                                 smi_features_ok_callback, lpc,
+                                 smi_features_ok_callback, NULL, lpc,
                                  &lpc->smi_features_ok,
                                  sizeof lpc->smi_features_ok,
                                  true);
diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index e3bd626b8c..753ac0e4ea 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -56,6 +56,7 @@ struct FWCfgEntry {
     uint8_t *data;
     void *callback_opaque;
     FWCfgCallback select_cb;
+    FWCfgWriteCallback write_cb;
 };
 
 #define JPG_FILE 0
@@ -370,6 +371,8 @@ static void fw_cfg_dma_transfer(FWCfgState *s)
                     dma_memory_read(s->dma_as, dma.address,
                                     &e->data[s->cur_offset], len)) {
                     dma.control |= FW_CFG_DMA_CTL_ERROR;
+                } else if (e->write_cb) {
+                    e->write_cb(e->callback_opaque, s->cur_offset, len);
                 }
             }
 
@@ -570,6 +573,7 @@ static const VMStateDescription vmstate_fw_cfg = {
 
 static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
                                       FWCfgCallback select_cb,
+                                      FWCfgWriteCallback write_cb,
                                       void *callback_opaque,
                                       void *data, size_t len,
                                       bool read_only)
@@ -584,6 +588,7 @@ static void fw_cfg_add_bytes_callback(FWCfgState *s, uint16_t key,
     s->entries[arch][key].data = data;
     s->entries[arch][key].len = (uint32_t)len;
     s->entries[arch][key].select_cb = select_cb;
+    s->entries[arch][key].write_cb = write_cb;
     s->entries[arch][key].callback_opaque = callback_opaque;
     s->entries[arch][key].allow_write = !read_only;
 }
@@ -610,7 +615,7 @@ static void *fw_cfg_modify_bytes_read(FWCfgState *s, uint16_t key,
 
 void fw_cfg_add_bytes(FWCfgState *s, uint16_t key, void *data, size_t len)
 {
-    fw_cfg_add_bytes_callback(s, key, NULL, NULL, data, len, true);
+    fw_cfg_add_bytes_callback(s, key, NULL, NULL, NULL, data, len, true);
 }
 
 void fw_cfg_add_string(FWCfgState *s, uint16_t key, const char *value)
@@ -737,6 +742,7 @@ static int get_fw_cfg_order(FWCfgState *s, const char *name)
 
 void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
                               FWCfgCallback select_cb,
+                              FWCfgWriteCallback write_cb,
                               void *callback_opaque,
                               void *data, size_t len, bool read_only)
 {
@@ -800,7 +806,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
     }
 
     fw_cfg_add_bytes_callback(s, FW_CFG_FILE_FIRST + index,
-                              select_cb,
+                              select_cb, write_cb,
                               callback_opaque, data, len,
                               read_only);
 
@@ -815,7 +821,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
 void fw_cfg_add_file(FWCfgState *s,  const char *filename,
                      void *data, size_t len)
 {
-    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len, true);
+    fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
 }
 
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
@@ -838,7 +844,7 @@ void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
         }
     }
     /* add new one */
-    fw_cfg_add_file_callback(s, filename, NULL, NULL, data, len, true);
+    fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
     return NULL;
 }
 
-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Marc-André Lureau]

See docs/specs/vmcoreinfo.txt for details.

"etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/hw/misc/vmcoreinfo.h | 46 +++++++++++++++++++++
 hw/misc/vmcoreinfo.c         | 96 ++++++++++++++++++++++++++++++++++++++++++++
 docs/specs/vmcoreinfo.txt    | 41 +++++++++++++++++++
 hw/misc/Makefile.objs        |  1 +
 4 files changed, 184 insertions(+)
 create mode 100644 include/hw/misc/vmcoreinfo.h
 create mode 100644 hw/misc/vmcoreinfo.c
 create mode 100644 docs/specs/vmcoreinfo.txt

diff --git a/include/hw/misc/vmcoreinfo.h b/include/hw/misc/vmcoreinfo.h
new file mode 100644
index 0000000000..c3aa856545
--- /dev/null
+++ b/include/hw/misc/vmcoreinfo.h
@@ -0,0 +1,46 @@
+/*
+ * Virtual Machine coreinfo device
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * Authors: Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * 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 VMCOREINFO_H
+#define VMCOREINFO_H
+
+#include "hw/qdev.h"
+
+#define VMCOREINFO_DEVICE "vmcoreinfo"
+#define VMCOREINFO(obj) OBJECT_CHECK(VMCoreInfoState, (obj), VMCOREINFO_DEVICE)
+
+#define VMCOREINFO_FORMAT_NONE 0x0
+#define VMCOREINFO_FORMAT_ELF 0x1
+
+/* all fields are little-endian */
+typedef struct FWCfgVMCoreInfo {
+    uint16_t host_format; /* set on reset */
+    uint16_t guest_format;
+    uint32_t size;
+    uint64_t paddr;
+} QEMU_PACKED FWCfgVMCoreInfo;
+
+typedef struct VMCoreInfoState {
+    DeviceClass parent_obj;
+
+    bool has_vmcoreinfo;
+    FWCfgVMCoreInfo vmcoreinfo;
+} VMCoreInfoState;
+
+/* returns NULL unless there is exactly one device */
+static inline VMCoreInfoState *vmcoreinfo_find(void)
+{
+    Object *o = object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
+
+    return o ? VMCOREINFO(o) : NULL;
+}
+
+#endif
diff --git a/hw/misc/vmcoreinfo.c b/hw/misc/vmcoreinfo.c
new file mode 100644
index 0000000000..a618e12677
--- /dev/null
+++ b/hw/misc/vmcoreinfo.c
@@ -0,0 +1,96 @@
+/*
+ * Virtual Machine coreinfo device
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ *
+ * Authors: Marc-André Lureau <marcandre.lureau@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "hw/nvram/fw_cfg.h"
+#include "hw/misc/vmcoreinfo.h"
+
+static void fw_cfg_vmci_write(void *dev, off_t offset, size_t len)
+{
+    VMCoreInfoState *s = VMCOREINFO(dev);
+
+    s->has_vmcoreinfo = offset == 0 && len == sizeof(s->vmcoreinfo)
+        && s->vmcoreinfo.guest_format != VMCOREINFO_FORMAT_NONE;
+}
+
+static void vmcoreinfo_reset(void *dev)
+{
+    VMCoreInfoState *s = VMCOREINFO(dev);
+
+    s->has_vmcoreinfo = false;
+    memset(&s->vmcoreinfo, 0, sizeof(s->vmcoreinfo));
+    s->vmcoreinfo.host_format = cpu_to_le16(VMCOREINFO_FORMAT_ELF);
+}
+
+static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
+{
+    VMCoreInfoState *s = VMCOREINFO(dev);
+    FWCfgState *fw_cfg = fw_cfg_find();
+
+    /* Given that this function is executing, there is at least one VMCOREINFO
+     * device. Check if there are several.
+     */
+    if (!vmcoreinfo_find()) {
+        error_setg(errp, "at most one %s device is permitted",
+                   VMCOREINFO_DEVICE);
+        return;
+    }
+
+    if (!fw_cfg || !fw_cfg->dma_enabled) {
+        error_setg(errp, "%s device requires fw_cfg with DMA",
+                   VMCOREINFO_DEVICE);
+        return;
+    }
+
+    fw_cfg_add_file_callback(fw_cfg, "etc/vmcoreinfo",
+                             NULL, fw_cfg_vmci_write, s,
+                             &s->vmcoreinfo, sizeof(s->vmcoreinfo), false);
+
+    qemu_register_reset(vmcoreinfo_reset, dev);
+}
+
+static const VMStateDescription vmstate_vmcoreinfo = {
+    .name = "vmcoreinfo",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_BOOL(has_vmcoreinfo, VMCoreInfoState),
+        VMSTATE_UINT16(vmcoreinfo.host_format, VMCoreInfoState),
+        VMSTATE_UINT16(vmcoreinfo.guest_format, VMCoreInfoState),
+        VMSTATE_UINT32(vmcoreinfo.size, VMCoreInfoState),
+        VMSTATE_UINT64(vmcoreinfo.paddr, VMCoreInfoState),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void vmcoreinfo_device_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    dc->vmsd = &vmstate_vmcoreinfo;
+    dc->realize = vmcoreinfo_realize;
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo vmcoreinfo_device_info = {
+    .name          = VMCOREINFO_DEVICE,
+    .parent        = TYPE_DEVICE,
+    .instance_size = sizeof(VMCoreInfoState),
+    .class_init    = vmcoreinfo_device_class_init,
+};
+
+static void vmcoreinfo_register_types(void)
+{
+    type_register_static(&vmcoreinfo_device_info);
+}
+
+type_init(vmcoreinfo_register_types)
diff --git a/docs/specs/vmcoreinfo.txt b/docs/specs/vmcoreinfo.txt
new file mode 100644
index 0000000000..2868a77142
--- /dev/null
+++ b/docs/specs/vmcoreinfo.txt
@@ -0,0 +1,41 @@
+=================
+VMCoreInfo device
+=================
+
+The `-device vmcoreinfo` will create a fw_cfg entry for a guest to
+store dump details.
+
+etc/vmcoreinfo
+**************
+
+A guest may use this fw_cfg entry to add information details to qemu
+dumps.
+
+The entry of 16 bytes has the following layout, in little-endian::
+
+#define VMCOREINFO_FORMAT_NONE 0x0
+#define VMCOREINFO_FORMAT_ELF 0x1
+
+    struct FWCfgVMCoreInfo {
+        uint16_t host_format;  /* formats host supports */
+        uint16_t guest_format; /* format guest supplies */
+        uint32_t size;         /* size of vmcoreinfo region */
+        uint64_t paddr;        /* physical address of vmcoreinfo region */
+    };
+
+Only full write (of 16 bytes) are considered valid for further
+processing of entry values.
+
+A write of 0 in guest_format will disable further processing of
+vmcoreinfo entry values & content.
+
+Format & content
+****************
+
+As of qemu 2.11, only VMCOREINFO_FORMAT_ELF is supported.
+
+The entry gives location and size of an ELF note that is appended in
+qemu dumps.
+
+The note format/class must be of the target bitness and the size must
+be less than 1Mb.
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 29fb922cef..052982f69b 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -9,6 +9,7 @@ common-obj-$(CONFIG_PCI_TESTDEV) += pci-testdev.o
 common-obj-$(CONFIG_EDU) += edu.o
 
 common-obj-y += unimp.o
+common-obj-y += vmcoreinfo.o
 
 obj-$(CONFIG_VMPORT) += vmport.o
 
-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 3/7] dump: add guest ELF note

[Marc-André Lureau]

Read the guest ELF PT_NOTE from guest memory when fw_cfg
etc/vmcoreinfo entry provides the location, and write it as an
additional note in the dump.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/dump.h |   2 +
 dump.c                | 107 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+)

diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 2672a15f8b..df43bd0e07 100644
--- a/include/sysemu/dump.h
+++ b/include/sysemu/dump.h
@@ -192,6 +192,8 @@ typedef struct DumpState {
                                   * this could be used to calculate
                                   * how much work we have
                                   * finished. */
+    uint8_t *guest_note;         /* ELF note content */
+    size_t guest_note_size;
 } DumpState;
 
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/dump.c b/dump.c
index a79773d0f7..3cec6a8c93 100644
--- a/dump.c
+++ b/dump.c
@@ -26,6 +26,8 @@
 #include "qapi/qmp/qerror.h"
 #include "qmp-commands.h"
 #include "qapi-event.h"
+#include "qemu/error-report.h"
+#include "hw/misc/vmcoreinfo.h"
 
 #include <zlib.h>
 #ifdef CONFIG_LZO
@@ -38,6 +40,13 @@
 #define ELF_MACHINE_UNAME "Unknown"
 #endif
 
+#define MAX_GUEST_NOTE_SIZE (1 << 20) /* 1MB should be enough */
+
+#define ELF_NOTE_SIZE(hdr_size, name_size, desc_size)   \
+    ((DIV_ROUND_UP((hdr_size), 4) +                     \
+      DIV_ROUND_UP((name_size), 4) +                    \
+      DIV_ROUND_UP((desc_size), 4)) * 4)
+
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
 {
     if (s->dump_info.d_endian == ELFDATA2LSB) {
@@ -76,6 +85,8 @@ static int dump_cleanup(DumpState *s)
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
     close(s->fd);
+    g_free(s->guest_note);
+    s->guest_note = NULL;
     if (s->resume) {
         if (s->detached) {
             qemu_mutex_lock_iothread();
@@ -235,6 +246,19 @@ static inline int cpu_index(CPUState *cpu)
     return cpu->cpu_index + 1;
 }
 
+static void write_guest_note(WriteCoreDumpFunction f, DumpState *s,
+                             Error **errp)
+{
+    int ret;
+
+    if (s->guest_note) {
+        ret = f(s->guest_note, s->guest_note_size, s);
+        if (ret < 0) {
+            error_setg(errp, "dump: failed to write guest note");
+        }
+    }
+}
+
 static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
                               Error **errp)
 {
@@ -258,6 +282,8 @@ static void write_elf64_notes(WriteCoreDumpFunction f, DumpState *s,
             return;
         }
     }
+
+    write_guest_note(f, s, errp);
 }
 
 static void write_elf32_note(DumpState *s, Error **errp)
@@ -303,6 +329,8 @@ static void write_elf32_notes(WriteCoreDumpFunction f, DumpState *s,
             return;
         }
     }
+
+    write_guest_note(f, s, errp);
 }
 
 static void write_elf_section(DumpState *s, int type, Error **errp)
@@ -714,6 +742,44 @@ static int buf_write_note(const void *buf, size_t size, void *opaque)
     return 0;
 }
 
+/*
+ * This function retrieves various sizes from an elf header.
+ *
+ * @note has to be a valid ELF note. The return sizes are unmodified
+ * (not padded or rounded up to be multiple of 4).
+ */
+static void get_note_sizes(DumpState *s, const void *note,
+                           uint64_t *note_head_size,
+                           uint64_t *name_size,
+                           uint64_t *desc_size)
+{
+    uint64_t note_head_sz;
+    uint64_t name_sz;
+    uint64_t desc_sz;
+
+    if (s->dump_info.d_class == ELFCLASS64) {
+        const Elf64_Nhdr *hdr = note;
+        note_head_sz = sizeof(Elf64_Nhdr);
+        name_sz = tswap64(hdr->n_namesz);
+        desc_sz = tswap64(hdr->n_descsz);
+    } else {
+        const Elf32_Nhdr *hdr = note;
+        note_head_sz = sizeof(Elf32_Nhdr);
+        name_sz = tswap32(hdr->n_namesz);
+        desc_sz = tswap32(hdr->n_descsz);
+    }
+
+    if (note_head_size) {
+        *note_head_size = note_head_sz;
+    }
+    if (name_size) {
+        *name_size = name_sz;
+    }
+    if (desc_size) {
+        *desc_size = desc_sz;
+    }
+}
+
 /* write common header, sub header and elf note to vmcore */
 static void create_header32(DumpState *s, Error **errp)
 {
@@ -1492,6 +1558,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
                       DumpGuestMemoryFormat format, bool paging, bool has_filter,
                       int64_t begin, int64_t length, Error **errp)
 {
+    VMCoreInfoState *vmci = vmcoreinfo_find();
     CPUState *cpu;
     int nr_cpus;
     Error *err = NULL;
@@ -1563,6 +1630,46 @@ static void dump_init(DumpState *s, int fd, bool has_format,
         goto cleanup;
     }
 
+    /*
+     * The goal of this block is to copy the guest note out of
+     * the guest.  Failure to do so is not fatal for dumping.
+     */
+    if (vmci) {
+        uint64_t addr, note_head_size, name_size, desc_size;
+        uint32_t size;
+        uint16_t format;
+
+        note_head_size = s->dump_info.d_class == ELFCLASS32 ?
+            sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
+
+        format = le16_to_cpu(vmci->vmcoreinfo.guest_format);
+        size = le32_to_cpu(vmci->vmcoreinfo.size);
+        addr = le64_to_cpu(vmci->vmcoreinfo.paddr);
+        if (!vmci->has_vmcoreinfo) {
+            warn_report("guest note is not present");
+        } else if (size < note_head_size || size > MAX_GUEST_NOTE_SIZE) {
+            warn_report("guest note size is invalid: %" PRIu32, size);
+        } else if (format != VMCOREINFO_FORMAT_ELF) {
+            warn_report("guest note format is unsupported: %" PRIu16, format);
+        } else {
+            s->guest_note = g_malloc(size + 1); /* +1 for adding \0 */
+            cpu_physical_memory_read(addr, s->guest_note, size);
+
+            get_note_sizes(s, s->guest_note, NULL, &name_size, &desc_size);
+            s->guest_note_size = ELF_NOTE_SIZE(note_head_size, name_size,
+                                               desc_size);
+            if (name_size > MAX_GUEST_NOTE_SIZE ||
+                desc_size > MAX_GUEST_NOTE_SIZE ||
+                s->guest_note_size > size) {
+                warn_report("Invalid guest note header");
+                g_free(s->guest_note);
+                s->guest_note = NULL;
+            } else {
+                s->note_size += s->guest_note_size;
+            }
+        }
+    }
+
     /* get memory mapping */
     if (paging) {
         qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 4/7] dump: update phys_base header field based on VMCOREINFO content

[Marc-André Lureau]

If the guest note is VMCOREINFO, try to get phys_base from it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 dump.c                    | 56 +++++++++++++++++++++++++++++++++++++++++++++--
 docs/specs/vmcoreinfo.txt |  8 +++++++
 2 files changed, 62 insertions(+), 2 deletions(-)

diff --git a/dump.c b/dump.c
index 3cec6a8c93..0671a73108 100644
--- a/dump.c
+++ b/dump.c
@@ -780,6 +780,23 @@ static void get_note_sizes(DumpState *s, const void *note,
     }
 }
 
+static bool note_name_equal(DumpState *s,
+                            const uint8_t *note, const char *name)
+{
+    int len = strlen(name) + 1;
+    uint64_t head_size, name_size;
+
+    get_note_sizes(s, note, &head_size, &name_size, NULL);
+    head_size = ROUND_UP(head_size, 4);
+
+    if (name_size != len ||
+        memcmp(note + head_size, "VMCOREINFO", len)) {
+        return false;
+    }
+
+    return true;
+}
+
 /* write common header, sub header and elf note to vmcore */
 static void create_header32(DumpState *s, Error **errp)
 {
@@ -1554,6 +1571,39 @@ static int64_t dump_calculate_size(DumpState *s)
     return total;
 }
 
+static void vmcoreinfo_update_phys_base(DumpState *s)
+{
+    uint64_t size, note_head_size, name_size, phys_base;
+    char **lines;
+    uint8_t *vmci;
+    size_t i;
+
+    if (!note_name_equal(s, s->guest_note, "VMCOREINFO")) {
+        return;
+    }
+
+    get_note_sizes(s, s->guest_note, &note_head_size, &name_size, &size);
+    note_head_size = ROUND_UP(note_head_size, 4);
+
+    vmci = s->guest_note + note_head_size + ROUND_UP(name_size, 4);
+    *(vmci + size) = '\0';
+
+    lines = g_strsplit((char *)vmci, "\n", -1);
+    for (i = 0; lines[i]; i++) {
+        if (g_str_has_prefix(lines[i], "NUMBER(phys_base)=")) {
+            if (qemu_strtou64(lines[i] + 18, NULL, 16,
+                              &phys_base) < 0) {
+                warn_report("Failed to read NUMBER(phys_base)=");
+            } else {
+                s->dump_info.phys_base = phys_base;
+            }
+            break;
+        }
+    }
+
+    g_strfreev(lines);
+}
+
 static void dump_init(DumpState *s, int fd, bool has_format,
                       DumpGuestMemoryFormat format, bool paging, bool has_filter,
                       int64_t begin, int64_t length, Error **errp)
@@ -1631,8 +1681,9 @@ static void dump_init(DumpState *s, int fd, bool has_format,
     }
 
     /*
-     * The goal of this block is to copy the guest note out of
-     * the guest.  Failure to do so is not fatal for dumping.
+     * The goal of this block is to (a) update the previously guessed
+     * phys_base, (b) copy the guest note out of the guest.
+     * Failure to do so is not fatal for dumping.
      */
     if (vmci) {
         uint64_t addr, note_head_size, name_size, desc_size;
@@ -1665,6 +1716,7 @@ static void dump_init(DumpState *s, int fd, bool has_format,
                 g_free(s->guest_note);
                 s->guest_note = NULL;
             } else {
+                vmcoreinfo_update_phys_base(s);
                 s->note_size += s->guest_note_size;
             }
         }
diff --git a/docs/specs/vmcoreinfo.txt b/docs/specs/vmcoreinfo.txt
index 2868a77142..821261067f 100644
--- a/docs/specs/vmcoreinfo.txt
+++ b/docs/specs/vmcoreinfo.txt
@@ -39,3 +39,11 @@ qemu dumps.
 
 The note format/class must be of the target bitness and the size must
 be less than 1Mb.
+
+If the ELF note name is "VMCOREINFO", it is expected to be the Linux
+vmcoreinfo note (see Documentation/ABI/testing/sysfs-kernel-vmcoreinfo
+in Linux source). In this case, qemu dump code will read the content
+as a key=value text file, looking for "NUMBER(phys_base)" key
+value. The value is expected to be more accurate than architecture
+guess of the value. This is useful for KASLR-enabled guest with
+ancient tools not handling the VMCOREINFO note.
-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 5/7] kdump: set vmcoreinfo location

[Marc-André Lureau]

kdump header provides offset and size of the vmcoreinfo content,
append it if available (skip the ELF note header).

crash-7.1.9 was the first version that started looking in the
vmcoreinfo data for phys_base instead of in the kdump_sub_header.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 dump.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/dump.c b/dump.c
index 0671a73108..f061fc69db 100644
--- a/dump.c
+++ b/dump.c
@@ -858,6 +858,18 @@ static void create_header32(DumpState *s, Error **errp)
     kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL);
 
     offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
+    if (s->guest_note &&
+        note_name_equal(s, s->guest_note, "VMCOREINFO")) {
+        uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo;
+
+        get_note_sizes(s, s->guest_note,
+                       &hsize, &name_size, &size_vmcoreinfo_desc);
+        offset_vmcoreinfo = offset_note + s->note_size - s->guest_note_size +
+            (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4;
+        kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo);
+        kh->size_vmcoreinfo = cpu_to_dump32(s, size_vmcoreinfo_desc);
+    }
+
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump32(s, s->note_size);
 
@@ -958,6 +970,18 @@ static void create_header64(DumpState *s, Error **errp)
     kh->dump_level = cpu_to_dump32(s, DUMP_LEVEL);
 
     offset_note = DISKDUMP_HEADER_BLOCKS * block_size + size;
+    if (s->guest_note &&
+        note_name_equal(s, s->guest_note, "VMCOREINFO")) {
+        uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo;
+
+        get_note_sizes(s, s->guest_note,
+                       &hsize, &name_size, &size_vmcoreinfo_desc);
+        offset_vmcoreinfo = offset_note + s->note_size - s->guest_note_size +
+            (DIV_ROUND_UP(hsize, 4) + DIV_ROUND_UP(name_size, 4)) * 4;
+        kh->offset_vmcoreinfo = cpu_to_dump64(s, offset_vmcoreinfo);
+        kh->size_vmcoreinfo = cpu_to_dump64(s, size_vmcoreinfo_desc);
+    }
+
     kh->offset_note = cpu_to_dump64(s, offset_note);
     kh->note_size = cpu_to_dump64(s, s->note_size);
 
-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 6/7] scripts/dump-guest-memory.py: add vmcoreinfo

[Marc-André Lureau]

Add a vmcoreinfo ELF note in the dump if vmcoreinfo device has the
memory location details.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 scripts/dump-guest-memory.py | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index f7c6635f15..69dd5efadf 100644
--- a/scripts/dump-guest-memory.py
+++ b/scripts/dump-guest-memory.py
@@ -14,6 +14,7 @@ the COPYING file in the top-level directory.
 """
 
 import ctypes
+import struct
 
 UINTPTR_T = gdb.lookup_type("uintptr_t")
 
@@ -45,6 +46,17 @@ EM_S390 = 22
 EM_AARCH = 183
 EM_X86_64 = 62
 
+VMCOREINFO_FORMAT_ELF = 1
+
+def le16_to_cpu(val):
+    return struct.unpack("<H", struct.pack("=H", val))[0]
+
+def le32_to_cpu(val):
+    return struct.unpack("<I", struct.pack("=I", val))[0]
+
+def le64_to_cpu(val):
+    return struct.unpack("<Q", struct.pack("=Q", val))[0]
+
 class ELF(object):
     """Representation of a ELF file."""
 
@@ -120,6 +132,25 @@ class ELF(object):
         self.segments[0].p_filesz += ctypes.sizeof(note)
         self.segments[0].p_memsz += ctypes.sizeof(note)
 
+
+    def add_vmcoreinfo_note(self, vmcoreinfo):
+        """Adds a vmcoreinfo note to the ELF dump."""
+        # compute the header size, and copy that many bytes from the note
+        header = get_arch_note(self.endianness, 0, 0)
+        ctypes.memmove(ctypes.pointer(header),
+                       vmcoreinfo, ctypes.sizeof(header))
+        if header.n_descsz > 1 << 20:
+            print('warning: invalid vmcoreinfo size')
+            return
+        # now get the full note
+        note = get_arch_note(self.endianness,
+                             header.n_namesz - 1, header.n_descsz)
+        ctypes.memmove(ctypes.pointer(note), vmcoreinfo, ctypes.sizeof(note))
+
+        self.notes.append(note)
+        self.segments[0].p_filesz += ctypes.sizeof(note)
+        self.segments[0].p_memsz += ctypes.sizeof(note)
+
     def add_segment(self, p_type, p_paddr, p_size):
         """Adds a segment to the elf."""
 
@@ -505,6 +536,35 @@ shape and this command should mostly work."""
                 cur += chunk_size
                 left -= chunk_size
 
+    def phys_memory_read(self, addr, size):
+        qemu_core = gdb.inferiors()[0]
+        for block in self.guest_phys_blocks:
+            if block["target_start"] <= addr \
+               and addr + size <= block["target_end"]:
+                haddr = block["host_addr"] + (addr - block["target_start"])
+                return qemu_core.read_memory(haddr, size)
+        return None
+
+    def add_vmcoreinfo(self):
+        if not gdb.parse_and_eval("vmcoreinfo_find()") \
+           or not gdb.parse_and_eval("vmcoreinfo_find()->has_vmcoreinfo"):
+            return
+
+        fmt = gdb.parse_and_eval("vmcoreinfo_find()->vmcoreinfo.guest_format")
+        addr = gdb.parse_and_eval("vmcoreinfo_find()->vmcoreinfo.paddr")
+        size = gdb.parse_and_eval("vmcoreinfo_find()->vmcoreinfo.size")
+
+        fmt = le16_to_cpu(fmt)
+        addr = le64_to_cpu(addr)
+        size = le32_to_cpu(size)
+
+        if fmt != VMCOREINFO_FORMAT_ELF:
+            return
+
+        vmcoreinfo = self.phys_memory_read(addr, size)
+        if vmcoreinfo:
+            self.elf.add_vmcoreinfo_note(vmcoreinfo.tobytes())
+
     def invoke(self, args, from_tty):
         """Handles command invocation from gdb."""
 
@@ -518,6 +578,7 @@ shape and this command should mostly work."""
 
         self.elf = ELF(argv[1])
         self.guest_phys_blocks = get_guest_phys_blocks()
+        self.add_vmcoreinfo()
 
         with open(argv[0], "wb") as vmcore:
             self.dump_init(vmcore)
-- 
2.14.1.146.gd35faa819

[Qemu-devel] [PATCH v6 7/7] MAINTAINERS: add Dump maintainers

[Marc-André Lureau]

Proposing myself, since I have some familiarity with the code now.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
 MAINTAINERS | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 36eeb42d19..6e47428b33 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1288,6 +1288,17 @@ S: Maintained
 F: device_tree.c
 F: include/sysemu/device_tree.h
 
+Dump
+S: Supported
+M: Marc-André Lureau <marcandre.lureau@redhat.com>
+F: dump.c
+F: hw/misc/vmcoreinfo.c
+F: include/hw/misc/vmcoreinfo.h
+F: include/sysemu/dump-arch.h
+F: include/sysemu/dump.h
+F: scripts/dump-guest-memory.py
+F: stubs/dump.c
+
 Error reporting
 M: Markus Armbruster <armbru@redhat.com>
 S: Supported
-- 
2.14.1.146.gd35faa819

Re: [Qemu-devel] [PATCH v6 0/7] KASLR kernel dump support

[Marc-André Lureau]

ping

On Mon, Sep 11, 2017 at 6:59 PM, Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
> Recent linux kernels enable KASLR to randomize phys/virt memory
> addresses. This series aims to provide enough information in qemu
> dumps so that crash utility can work with randomized kernel too (it
> hasn't been tested on other archs than x86 though, help welcome).
>
> The previous design to provide qemu with debug details (using qemu-ga
> and a dedicated vmcoreinfo ACPI device) failed to satisfy the
> requirements during previous iterations.
>
> In particular, the previous proposed vmcoreinfo ACPI device had the
> following issues:
> - hazardous memory handling with no explicit synchronization
> - occupy 2 fw-cfg entries (for memory and pointer)
> - occupy 4k of guest memory (this could have been tweaked)
> - did not provide ACPI methods (this could have been added)
> - may be difficult to maintain compatibility (according to Michael)
>
> This is a new proposal, that leverage fw-cfg device instead of adding
> a new device. A "etc/vmcoreinfo" entry is added, where the guest,
> during boot or later, can write the addr/size location of an ELF note
> to be appended in the qemu dump.
>
> Note: only guest kernel is expected to write to a fw-cfg entry.  This
> method is not meant for general qemu/user-space communication. There
> are more appropriate devices for this purpose, and the guest kernel
> should not expose this facility.
>
> This is quite easier to implement, and uses less of the limited fw-cfg
> slots, and guest memory. It also solves the synchronization issue, and
> may be easier to discover or to maintain compatibility.
>
> The Linux ELF note is expected to be the VMCOREINFO note, which will
> have a special handling in qemu in this case helping kaslr-kernel
> debugging. But it could be any valid ELF note.
>
> Crash 7.1.9 will parse the "phys_base" value from the VMCOREINFO note,
> and thus will work with KASLR-dump produced by this series.
>
> The series implements the note addition in qemu ELF/kdump,
> as well as the python scripts/dump-guest-memory.py.
>
> To test:
>
> Using kernel from https://github.com/elmarco/linux fw-cfg branch,
> Compile and run guest kernel with CONFIG_RANDOMIZE_BASE=y & CONFIG_FW_CFG_SYSFS=y.
>
> Run qemu with -device vmcoreinfo
>
> Produce an ELF dump:
> { "execute": "dump-guest-memory", "arguments": { "protocol": "file:dump", "paging": false } }
>
> Produce a kdump:
> { "execute": "dump-guest-memory", "arguments": { "protocol": "file:dump", "paging": false, "format": "kdump-zlib" } }
>
> Or with (gdb) dump-guest-memory, with scripts/dump-guest-memory.py script.
>
> Analyze with crash >= 7.1.9 (or the git version for 4.13 fixes..):
>
> $ crash vmlinux dump
>
> v6: after Michael Tsirkin review
> - rebased
> - modify fw_cfg write callback, called for all write
> - back to a seperate -device vmcoreinfo
> - add host_format/guest_format fields
> - clear/reset fw_cfg entry values on reset
> - write 0 as guest format to disable device
>
> v5:
> - removed x-write-pointer-available patch from this series
> - drop vmcoreinfo device
> - add write callback to fw_cfg entries
> - add a writable fw_cfg "vmcoreinfo" entry
> - split phys_base update from VMCOREINFO note in a seperate patch
> - most patches had non-trivial changes, dropping reviewed-by tags
>
> v4: from Laszlo review
> - switch to warn_report*()
> - update test to follow vmgenid and use boot-sector infrastructure
> - fix range checks in the python script
> - add vmcoreinfo_get() stub
>
> v3: from Laszlo review
> - change vmcoreinfo offset to 36
> - reset err to null after report
> - use PRIu32
> - change name_size and desc_size against MAX_VMCOREINFO_SIZE
> - python code simplification
> - check boundaries of blocks in phys_memory_read()
> - fix some vmgi vs vmci names
> - add more comments in code
> - fix comment indentation
> - add r-b tags
>
> v2: from Laszlo review
> - vmci: fix guest endianess handling
> - vmci: fix wrong sizeof()
> - vmci: add back reset logic from vmgenid
> - dump: have 1MB size limit for vmcoreinfo
> - dump: fix potential off-by-1 buffer manipulation
> - dump: use temporary variable for qemu_strtou64
> - dump: fixed VMCOREINFO duplication in kdump
> - update gdb script to not call into qemu process
> - update MAINTAINERS with some new files
>
> Marc-André Lureau (7):
>   fw_cfg: add write callback
>   hw/misc: add vmcoreinfo device
>   dump: add guest ELF note
>   dump: update phys_base header field based on VMCOREINFO content
>   kdump: set vmcoreinfo location
>   scripts/dump-guest-memory.py: add vmcoreinfo
>   MAINTAINERS: add Dump maintainers
>
>  scripts/dump-guest-memory.py |  61 +++++++++++++++
>  include/hw/misc/vmcoreinfo.h |  46 +++++++++++
>  include/hw/nvram/fw_cfg.h    |   3 +
>  include/sysemu/dump.h        |   2 +
>  dump.c                       | 183 +++++++++++++++++++++++++++++++++++++++++++
>  hw/acpi/vmgenid.c            |   2 +-
>  hw/core/loader.c             |   2 +-
>  hw/i386/acpi-build.c         |   2 +-
>  hw/isa/lpc_ich9.c            |   4 +-
>  hw/misc/vmcoreinfo.c         |  96 +++++++++++++++++++++++
>  hw/nvram/fw_cfg.c            |  14 +++-
>  MAINTAINERS                  |  11 +++
>  docs/specs/vmcoreinfo.txt    |  49 ++++++++++++
>  hw/misc/Makefile.objs        |   1 +
>  14 files changed, 467 insertions(+), 9 deletions(-)
>  create mode 100644 include/hw/misc/vmcoreinfo.h
>  create mode 100644 hw/misc/vmcoreinfo.c
>  create mode 100644 docs/specs/vmcoreinfo.txt
>
> --
> 2.14.1.146.gd35faa819
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v6 0/7] KASLR kernel dump support

[Marc-André Lureau]

ping

On Mon, Sep 25, 2017 at 12:52 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> ping
>
> On Mon, Sep 11, 2017 at 6:59 PM, Marc-André Lureau
> <marcandre.lureau@redhat.com> wrote:
>> Recent linux kernels enable KASLR to randomize phys/virt memory
>> addresses. This series aims to provide enough information in qemu
>> dumps so that crash utility can work with randomized kernel too (it
>> hasn't been tested on other archs than x86 though, help welcome).
>>
>> The previous design to provide qemu with debug details (using qemu-ga
>> and a dedicated vmcoreinfo ACPI device) failed to satisfy the
>> requirements during previous iterations.
>>
>> In particular, the previous proposed vmcoreinfo ACPI device had the
>> following issues:
>> - hazardous memory handling with no explicit synchronization
>> - occupy 2 fw-cfg entries (for memory and pointer)
>> - occupy 4k of guest memory (this could have been tweaked)
>> - did not provide ACPI methods (this could have been added)
>> - may be difficult to maintain compatibility (according to Michael)
>>
>> This is a new proposal, that leverage fw-cfg device instead of adding
>> a new device. A "etc/vmcoreinfo" entry is added, where the guest,
>> during boot or later, can write the addr/size location of an ELF note
>> to be appended in the qemu dump.
>>
>> Note: only guest kernel is expected to write to a fw-cfg entry.  This
>> method is not meant for general qemu/user-space communication. There
>> are more appropriate devices for this purpose, and the guest kernel
>> should not expose this facility.
>>
>> This is quite easier to implement, and uses less of the limited fw-cfg
>> slots, and guest memory. It also solves the synchronization issue, and
>> may be easier to discover or to maintain compatibility.
>>
>> The Linux ELF note is expected to be the VMCOREINFO note, which will
>> have a special handling in qemu in this case helping kaslr-kernel
>> debugging. But it could be any valid ELF note.
>>
>> Crash 7.1.9 will parse the "phys_base" value from the VMCOREINFO note,
>> and thus will work with KASLR-dump produced by this series.
>>
>> The series implements the note addition in qemu ELF/kdump,
>> as well as the python scripts/dump-guest-memory.py.
>>
>> To test:
>>
>> Using kernel from https://github.com/elmarco/linux fw-cfg branch,
>> Compile and run guest kernel with CONFIG_RANDOMIZE_BASE=y & CONFIG_FW_CFG_SYSFS=y.
>>
>> Run qemu with -device vmcoreinfo
>>
>> Produce an ELF dump:
>> { "execute": "dump-guest-memory", "arguments": { "protocol": "file:dump", "paging": false } }
>>
>> Produce a kdump:
>> { "execute": "dump-guest-memory", "arguments": { "protocol": "file:dump", "paging": false, "format": "kdump-zlib" } }
>>
>> Or with (gdb) dump-guest-memory, with scripts/dump-guest-memory.py script.
>>
>> Analyze with crash >= 7.1.9 (or the git version for 4.13 fixes..):
>>
>> $ crash vmlinux dump
>>
>> v6: after Michael Tsirkin review
>> - rebased
>> - modify fw_cfg write callback, called for all write
>> - back to a seperate -device vmcoreinfo
>> - add host_format/guest_format fields
>> - clear/reset fw_cfg entry values on reset
>> - write 0 as guest format to disable device
>>
>> v5:
>> - removed x-write-pointer-available patch from this series
>> - drop vmcoreinfo device
>> - add write callback to fw_cfg entries
>> - add a writable fw_cfg "vmcoreinfo" entry
>> - split phys_base update from VMCOREINFO note in a seperate patch
>> - most patches had non-trivial changes, dropping reviewed-by tags
>>
>> v4: from Laszlo review
>> - switch to warn_report*()
>> - update test to follow vmgenid and use boot-sector infrastructure
>> - fix range checks in the python script
>> - add vmcoreinfo_get() stub
>>
>> v3: from Laszlo review
>> - change vmcoreinfo offset to 36
>> - reset err to null after report
>> - use PRIu32
>> - change name_size and desc_size against MAX_VMCOREINFO_SIZE
>> - python code simplification
>> - check boundaries of blocks in phys_memory_read()
>> - fix some vmgi vs vmci names
>> - add more comments in code
>> - fix comment indentation
>> - add r-b tags
>>
>> v2: from Laszlo review
>> - vmci: fix guest endianess handling
>> - vmci: fix wrong sizeof()
>> - vmci: add back reset logic from vmgenid
>> - dump: have 1MB size limit for vmcoreinfo
>> - dump: fix potential off-by-1 buffer manipulation
>> - dump: use temporary variable for qemu_strtou64
>> - dump: fixed VMCOREINFO duplication in kdump
>> - update gdb script to not call into qemu process
>> - update MAINTAINERS with some new files
>>
>> Marc-André Lureau (7):
>>   fw_cfg: add write callback
>>   hw/misc: add vmcoreinfo device
>>   dump: add guest ELF note
>>   dump: update phys_base header field based on VMCOREINFO content
>>   kdump: set vmcoreinfo location
>>   scripts/dump-guest-memory.py: add vmcoreinfo
>>   MAINTAINERS: add Dump maintainers
>>
>>  scripts/dump-guest-memory.py |  61 +++++++++++++++
>>  include/hw/misc/vmcoreinfo.h |  46 +++++++++++
>>  include/hw/nvram/fw_cfg.h    |   3 +
>>  include/sysemu/dump.h        |   2 +
>>  dump.c                       | 183 +++++++++++++++++++++++++++++++++++++++++++
>>  hw/acpi/vmgenid.c            |   2 +-
>>  hw/core/loader.c             |   2 +-
>>  hw/i386/acpi-build.c         |   2 +-
>>  hw/isa/lpc_ich9.c            |   4 +-
>>  hw/misc/vmcoreinfo.c         |  96 +++++++++++++++++++++++
>>  hw/nvram/fw_cfg.c            |  14 +++-
>>  MAINTAINERS                  |  11 +++
>>  docs/specs/vmcoreinfo.txt    |  49 ++++++++++++
>>  hw/misc/Makefile.objs        |   1 +
>>  14 files changed, 467 insertions(+), 9 deletions(-)
>>  create mode 100644 include/hw/misc/vmcoreinfo.h
>>  create mode 100644 hw/misc/vmcoreinfo.c
>>  create mode 100644 docs/specs/vmcoreinfo.txt
>>
>> --
>> 2.14.1.146.gd35faa819
>>
>>
>
>
>
> --
> Marc-André Lureau



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Daniel P. Berrange]

On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> See docs/specs/vmcoreinfo.txt for details.
> 
> "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".

I'm wondering if you considered just adding the entry to fw_cfg by
default, without requiring any -device arg ? Unless I'm misunderstanding,
this doesn't feel like a device to me - its just a well known bucket
in fw_cfg IIUC ?  Obviously its existance would need to be tied to
the latest machine type for ABI reasons though. The benefit of this
is that it would "just work" without us having to plumb it through to
all the downstream applications that use QEMU for mgmt guest (OpenStack,
oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).

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

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Marc-André Lureau]

Hi

----- Original Message -----
> On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > See docs/specs/vmcoreinfo.txt for details.
> > 
> > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> 
> I'm wondering if you considered just adding the entry to fw_cfg by
> default, without requiring any -device arg ? Unless I'm misunderstanding,
> this doesn't feel like a device to me - its just a well known bucket
> in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> the latest machine type for ABI reasons though. The benefit of this
> is that it would "just work" without us having to plumb it through to
> all the downstream applications that use QEMU for mgmt guest (OpenStack,
> oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).

v5 did that, it was using a -global fw_cfg.vmcoreinfo=on that defaulted to on.

Michael preferred to have a separate -device rather than mix it with fw_cfg, since it's not directly related.

We could make -device vmcoreinfo default on on machine type >= 2.11 instead?

thanks

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Igor Mammedov]

On Mon, 9 Oct 2017 12:03:36 +0100
"Daniel P. Berrange" <berrange@redhat.com> wrote:

> On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > See docs/specs/vmcoreinfo.txt for details.
> > 
> > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".  
> 
> I'm wondering if you considered just adding the entry to fw_cfg by
> default, without requiring any -device arg ? Unless I'm misunderstanding,
> this doesn't feel like a device to me - its just a well known bucket
> in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> the latest machine type for ABI reasons though. The benefit of this
> is that it would "just work" without us having to plumb it through to
> all the downstream applications that use QEMU for mgmt guest (OpenStack,
> oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
it follows model set by pvpanic device, it's easier to manage from migration
POV, one could use it even for old machine types with new qemu (just by adding
device, it makes instance not backwards migratable to old qemu but should work
for forward migration) and if user doesn't need it, device could be just omitted
from CLI.

> 
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Daniel P. Berrange]

On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> On Mon, 9 Oct 2017 12:03:36 +0100
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> 
> > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > See docs/specs/vmcoreinfo.txt for details.
> > > 
> > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".  
> > 
> > I'm wondering if you considered just adding the entry to fw_cfg by
> > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > this doesn't feel like a device to me - its just a well known bucket
> > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > the latest machine type for ABI reasons though. The benefit of this
> > is that it would "just work" without us having to plumb it through to
> > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> it follows model set by pvpanic device, it's easier to manage from migration
> POV, one could use it even for old machine types with new qemu (just by adding
> device, it makes instance not backwards migratable to old qemu but should work
> for forward migration) and if user doesn't need it, device could be just omitted
> from CLI.

Sure but it means that in effect no one will have this functionality enabled
for several years. pvpanic has been around a long time and I rarely see it
present in configured guests :-(


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

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Michael S. Tsirkin]

On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > On Mon, 9 Oct 2017 12:03:36 +0100
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > 
> > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > > See docs/specs/vmcoreinfo.txt for details.
> > > > 
> > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".  
> > > 
> > > I'm wondering if you considered just adding the entry to fw_cfg by
> > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > > this doesn't feel like a device to me - its just a well known bucket
> > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > > the latest machine type for ABI reasons though. The benefit of this
> > > is that it would "just work" without us having to plumb it through to
> > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > it follows model set by pvpanic device, it's easier to manage from migration
> > POV, one could use it even for old machine types with new qemu (just by adding
> > device, it makes instance not backwards migratable to old qemu but should work
> > for forward migration) and if user doesn't need it, device could be just omitted
> > from CLI.
> 
> Sure but it means that in effect no one will have this functionality enabled
> for several years. pvpanic has been around a long time and I rarely see it
> present in configured guests :-(
> 
> 
> Regards,
> Daniel

libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
shouldn't add optional devices anyway.

So it's up to you guys, you can add it to VMs by default if you want to.


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

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > 
> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > > > See docs/specs/vmcoreinfo.txt for details.
> > > > > 
> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".  
> > > > 
> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > > > this doesn't feel like a device to me - its just a well known bucket
> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > > > the latest machine type for ABI reasons though. The benefit of this
> > > > is that it would "just work" without us having to plumb it through to
> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > > it follows model set by pvpanic device, it's easier to manage from migration
> > > POV, one could use it even for old machine types with new qemu (just by adding
> > > device, it makes instance not backwards migratable to old qemu but should work
> > > for forward migration) and if user doesn't need it, device could be just omitted
> > > from CLI.
> > 
> > Sure but it means that in effect no one will have this functionality enabled
> > for several years. pvpanic has been around a long time and I rarely see it
> > present in configured guests :-(
> > 
> > 
> > Regards,
> > Daniel
> 
> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> shouldn't add optional devices anyway.

This isn't really adding a device though is it - it is just a well known
location in fw_cfg to receive data.

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

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Marc-André Lureau]

Hi

On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
<berrange@redhat.com> wrote:
> On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
>> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
>> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
>> > > On Mon, 9 Oct 2017 12:03:36 +0100
>> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> > >
>> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
>> > > > > See docs/specs/vmcoreinfo.txt for details.
>> > > > >
>> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
>> > > >
>> > > > I'm wondering if you considered just adding the entry to fw_cfg by
>> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
>> > > > this doesn't feel like a device to me - its just a well known bucket
>> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
>> > > > the latest machine type for ABI reasons though. The benefit of this
>> > > > is that it would "just work" without us having to plumb it through to
>> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
>> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
>> > > it follows model set by pvpanic device, it's easier to manage from migration
>> > > POV, one could use it even for old machine types with new qemu (just by adding
>> > > device, it makes instance not backwards migratable to old qemu but should work
>> > > for forward migration) and if user doesn't need it, device could be just omitted
>> > > from CLI.
>> >
>> > Sure but it means that in effect no one will have this functionality enabled
>> > for several years. pvpanic has been around a long time and I rarely see it
>> > present in configured guests :-(
>> >
>> >
>> > Regards,
>> > Daniel
>>
>> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
>> shouldn't add optional devices anyway.
>
> This isn't really adding a device though is it - it is just a well known
> location in fw_cfg to receive data.

Enabling the device on some configurations by default can be done as a
follow-up patch. Can we get this series reviewed & merged?

thanks

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Michael S. Tsirkin]

On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> <berrange@redhat.com> wrote:
> > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> >> > >
> >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> >> > > > > See docs/specs/vmcoreinfo.txt for details.
> >> > > > >
> >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> >> > > >
> >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> >> > > > this doesn't feel like a device to me - its just a well known bucket
> >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> >> > > > the latest machine type for ABI reasons though. The benefit of this
> >> > > > is that it would "just work" without us having to plumb it through to
> >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> >> > > it follows model set by pvpanic device, it's easier to manage from migration
> >> > > POV, one could use it even for old machine types with new qemu (just by adding
> >> > > device, it makes instance not backwards migratable to old qemu but should work
> >> > > for forward migration) and if user doesn't need it, device could be just omitted
> >> > > from CLI.
> >> >
> >> > Sure but it means that in effect no one will have this functionality enabled
> >> > for several years. pvpanic has been around a long time and I rarely see it
> >> > present in configured guests :-(
> >> >
> >> >
> >> > Regards,
> >> > Daniel
> >>
> >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> >> shouldn't add optional devices anyway.
> >
> > This isn't really adding a device though is it - it is just a well known
> > location in fw_cfg to receive data.
> 
> Enabling the device on some configurations by default can be done as a
> follow-up patch. Can we get this series reviewed & merged?
> 
> thanks

I plan to merge this in the next pull request.

> -- 
> Marc-André Lureau

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Daniel P. Berrange]

On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> <berrange@redhat.com> wrote:
> > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> >> > >
> >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> >> > > > > See docs/specs/vmcoreinfo.txt for details.
> >> > > > >
> >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> >> > > >
> >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> >> > > > this doesn't feel like a device to me - its just a well known bucket
> >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> >> > > > the latest machine type for ABI reasons though. The benefit of this
> >> > > > is that it would "just work" without us having to plumb it through to
> >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> >> > > it follows model set by pvpanic device, it's easier to manage from migration
> >> > > POV, one could use it even for old machine types with new qemu (just by adding
> >> > > device, it makes instance not backwards migratable to old qemu but should work
> >> > > for forward migration) and if user doesn't need it, device could be just omitted
> >> > > from CLI.
> >> >
> >> > Sure but it means that in effect no one will have this functionality enabled
> >> > for several years. pvpanic has been around a long time and I rarely see it
> >> > present in configured guests :-(
> >> >
> >> >
> >> > Regards,
> >> > Daniel
> >>
> >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> >> shouldn't add optional devices anyway.
> >
> > This isn't really adding a device though is it - it is just a well known
> > location in fw_cfg to receive data.
> 
> Enabling the device on some configurations by default can be done as a
> follow-up patch. Can we get this series reviewed & merged?

The problem with the -device approach + turning it on by default is that there
is no way to turn it off again if you don't want it. eg there's way to undo
an implicit '-device foo' except via -nodefaults, but since libvirt uses that
already it would negate the effect of enabling it by default unconditionally.

Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
respect, as you can trivially turn it on/off, overriding the default state
in both directions.

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

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Michael S. Tsirkin]

On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> > <berrange@redhat.com> wrote:
> > > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> > >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > >> > >
> > >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > >> > > > > See docs/specs/vmcoreinfo.txt for details.
> > >> > > > >
> > >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> > >> > > >
> > >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > >> > > > this doesn't feel like a device to me - its just a well known bucket
> > >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > >> > > > the latest machine type for ABI reasons though. The benefit of this
> > >> > > > is that it would "just work" without us having to plumb it through to
> > >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > >> > > it follows model set by pvpanic device, it's easier to manage from migration
> > >> > > POV, one could use it even for old machine types with new qemu (just by adding
> > >> > > device, it makes instance not backwards migratable to old qemu but should work
> > >> > > for forward migration) and if user doesn't need it, device could be just omitted
> > >> > > from CLI.
> > >> >
> > >> > Sure but it means that in effect no one will have this functionality enabled
> > >> > for several years. pvpanic has been around a long time and I rarely see it
> > >> > present in configured guests :-(
> > >> >
> > >> >
> > >> > Regards,
> > >> > Daniel
> > >>
> > >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> > >> shouldn't add optional devices anyway.
> > >
> > > This isn't really adding a device though is it - it is just a well known
> > > location in fw_cfg to receive data.
> > 
> > Enabling the device on some configurations by default can be done as a
> > follow-up patch. Can we get this series reviewed & merged?
> 
> The problem with the -device approach + turning it on by default is that there
> is no way to turn it off again if you don't want it. eg there's way to undo
> an implicit '-device foo' except via -nodefaults, but since libvirt uses that
> already it would negate the effect of enabling it by default unconditionally.
> 
> Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
> respect, as you can trivially turn it on/off, overriding the default state
> in both directions.
> 
> Regards,
> Daniel


Interesting. IIUC you are saying that a property can have on/off/auto
options, while -device only has on/off.

Seems like something that's worth looking into generally. I'm not sure
why is should this device be special though.

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

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Eduardo Habkost]

On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
> On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> > Hi
> > 
> > On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> > <berrange@redhat.com> wrote:
> > > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> > >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > >> > >
> > >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > >> > > > > See docs/specs/vmcoreinfo.txt for details.
> > >> > > > >
> > >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> > >> > > >
> > >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > >> > > > this doesn't feel like a device to me - its just a well known bucket
> > >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > >> > > > the latest machine type for ABI reasons though. The benefit of this
> > >> > > > is that it would "just work" without us having to plumb it through to
> > >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > >> > > it follows model set by pvpanic device, it's easier to manage from migration
> > >> > > POV, one could use it even for old machine types with new qemu (just by adding
> > >> > > device, it makes instance not backwards migratable to old qemu but should work
> > >> > > for forward migration) and if user doesn't need it, device could be just omitted
> > >> > > from CLI.
> > >> >
> > >> > Sure but it means that in effect no one will have this functionality enabled
> > >> > for several years. pvpanic has been around a long time and I rarely see it
> > >> > present in configured guests :-(
> > >> >
> > >> >
> > >> > Regards,
> > >> > Daniel
> > >>
> > >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> > >> shouldn't add optional devices anyway.
> > >
> > > This isn't really adding a device though is it - it is just a well known
> > > location in fw_cfg to receive data.
> > 
> > Enabling the device on some configurations by default can be done as a
> > follow-up patch. Can we get this series reviewed & merged?
> 
> The problem with the -device approach + turning it on by default is that there
> is no way to turn it off again if you don't want it. eg there's way to undo
> an implicit '-device foo' except via -nodefaults, but since libvirt uses that
> already it would negate the effect of enabling it by default unconditionally.

It's still possible to add a -machine option that can
enable/disable automatic creation of the device.

But I also don't see why it needs to be implemented using -device
if it's not really a device.  A boolean machine or fw_cfg
property is good enough for that.

> 
> Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
> respect, as you can trivially turn it on/off, overriding the default state
> in both directions.

Both "-global fw_cfg.vmcoreinfo=on|off" and
"-machine vmcoreinfo=on|off" sound good enough to me.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Eduardo Habkost]

On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > 
> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > > > See docs/specs/vmcoreinfo.txt for details.
> > > > > 
> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".  
> > > > 
> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > > > this doesn't feel like a device to me - its just a well known bucket
> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > > > the latest machine type for ABI reasons though. The benefit of this
> > > > is that it would "just work" without us having to plumb it through to
> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > > it follows model set by pvpanic device, it's easier to manage from migration
> > > POV, one could use it even for old machine types with new qemu (just by adding
> > > device, it makes instance not backwards migratable to old qemu but should work
> > > for forward migration) and if user doesn't need it, device could be just omitted
> > > from CLI.
> > 
> > Sure but it means that in effect no one will have this functionality enabled
> > for several years. pvpanic has been around a long time and I rarely see it
> > present in configured guests :-(
> > 
> > 
> > Regards,
> > Daniel
> 
> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> shouldn't add optional devices anyway.

Does it mean every time we make a PC device configurable, we
should make it be disabled by -nodefaults, and require libvirt to
adapt?

I don't think that would be a good idea.  Imagine the hassle the
"pc: make .* configurable" patches[1] would generate for libvirt.

> 
> So it's up to you guys, you can add it to VMs by default if you want to.

To be honest, I think "no defaults" is a misleading name for an
option.  If it really meant "create no optional device at all",
it would eventually become a synonym for "-machine none", and I
don't think that's its goal.

I expect PC to always have a set of devices/features that are
disabled by -nodefaults, and a set of devices/features that are
not disabled by -nodefaults.  We need good judgement to decide on
which set the device will be, and I believe Daniel exposed good
arguments to put vmcoreinfo in the second set.

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg393493.html
    Subject: [RFC PATCH v2 00/12] Guest startup time optimization

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Michael S. Tsirkin]

On Tue, Oct 10, 2017 at 03:01:10PM -0300, Eduardo Habkost wrote:
> On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
> > On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> > > <berrange@redhat.com> wrote:
> > > > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> > > >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > > >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > > >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > > >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > >> > >
> > > >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > >> > > > > See docs/specs/vmcoreinfo.txt for details.
> > > >> > > > >
> > > >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> > > >> > > >
> > > >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > > >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > > >> > > > this doesn't feel like a device to me - its just a well known bucket
> > > >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > > >> > > > the latest machine type for ABI reasons though. The benefit of this
> > > >> > > > is that it would "just work" without us having to plumb it through to
> > > >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > > >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > > >> > > it follows model set by pvpanic device, it's easier to manage from migration
> > > >> > > POV, one could use it even for old machine types with new qemu (just by adding
> > > >> > > device, it makes instance not backwards migratable to old qemu but should work
> > > >> > > for forward migration) and if user doesn't need it, device could be just omitted
> > > >> > > from CLI.
> > > >> >
> > > >> > Sure but it means that in effect no one will have this functionality enabled
> > > >> > for several years. pvpanic has been around a long time and I rarely see it
> > > >> > present in configured guests :-(
> > > >> >
> > > >> >
> > > >> > Regards,
> > > >> > Daniel
> > > >>
> > > >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> > > >> shouldn't add optional devices anyway.
> > > >
> > > > This isn't really adding a device though is it - it is just a well known
> > > > location in fw_cfg to receive data.
> > > 
> > > Enabling the device on some configurations by default can be done as a
> > > follow-up patch. Can we get this series reviewed & merged?
> > 
> > The problem with the -device approach + turning it on by default is that there
> > is no way to turn it off again if you don't want it. eg there's way to undo
> > an implicit '-device foo' except via -nodefaults, but since libvirt uses that
> > already it would negate the effect of enabling it by default unconditionally.
> 
> It's still possible to add a -machine option that can
> enable/disable automatic creation of the device.
> 
> But I also don't see why it needs to be implemented using -device
> if it's not really a device.  A boolean machine or fw_cfg
> property is good enough for that.

It certainly feels like a device. It has state
(that needs to be migrated), it has a host/guest interface.


> > 
> > Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
> > respect, as you can trivially turn it on/off, overriding the default state
> > in both directions.
> 
> Both "-global fw_cfg.vmcoreinfo=on|off" and
> "-machine vmcoreinfo=on|off" sound good enough to me.


Certainly not a fw cfg flag. Can be a machine flag I guess
but then we'd have to open-code each such device.
And don't forget auto - this is what Daniel asks for.

I don't necessarily see this device as so special and think a generic
interface to control what goes into the machine would be better (e.g.
look how you use hacky -global to control fw cfg options, it only works
if there's a single one), but if everyone thinks otherwise and agrees we
should have it in there by default, and a property to disable, fine.

Can be a patch on top though.


> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Michael S. Tsirkin]

On Tue, Oct 10, 2017 at 03:01:10PM -0300, Eduardo Habkost wrote:
> On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
> > On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> > > Hi
> > > 
> > > On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> > > <berrange@redhat.com> wrote:
> > > > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> > > >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > > >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > > >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > > >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > >> > >
> > > >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > >> > > > > See docs/specs/vmcoreinfo.txt for details.
> > > >> > > > >
> > > >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> > > >> > > >
> > > >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > > >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > > >> > > > this doesn't feel like a device to me - its just a well known bucket
> > > >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > > >> > > > the latest machine type for ABI reasons though. The benefit of this
> > > >> > > > is that it would "just work" without us having to plumb it through to
> > > >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > > >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > > >> > > it follows model set by pvpanic device, it's easier to manage from migration
> > > >> > > POV, one could use it even for old machine types with new qemu (just by adding
> > > >> > > device, it makes instance not backwards migratable to old qemu but should work
> > > >> > > for forward migration) and if user doesn't need it, device could be just omitted
> > > >> > > from CLI.
> > > >> >
> > > >> > Sure but it means that in effect no one will have this functionality enabled
> > > >> > for several years. pvpanic has been around a long time and I rarely see it
> > > >> > present in configured guests :-(
> > > >> >
> > > >> >
> > > >> > Regards,
> > > >> > Daniel
> > > >>
> > > >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> > > >> shouldn't add optional devices anyway.
> > > >
> > > > This isn't really adding a device though is it - it is just a well known
> > > > location in fw_cfg to receive data.
> > > 
> > > Enabling the device on some configurations by default can be done as a
> > > follow-up patch. Can we get this series reviewed & merged?
> > 
> > The problem with the -device approach + turning it on by default is that there
> > is no way to turn it off again if you don't want it. eg there's way to undo
> > an implicit '-device foo' except via -nodefaults, but since libvirt uses that
> > already it would negate the effect of enabling it by default unconditionally.
> 
> It's still possible to add a -machine option that can
> enable/disable automatic creation of the device.
>
>
> But I also don't see why it needs to be implemented using -device
> if it's not really a device.  A boolean machine or fw_cfg
> property is good enough for that.

Device imho is a combination of guest/host interface and state.


> > 
> > Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
> > respect, as you can trivially turn it on/off, overriding the default state
> > in both directions.
> 
> Both "-global fw_cfg.vmcoreinfo=on|off" and
> "-machine vmcoreinfo=on|off" sound good enough to me.

I can live with the second option if people really want it.
I'd like to see some way to add these things without
adding to the mess that is the pc initialization
but oh well.

> -- 
> Eduardo

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Eduardo Habkost]

On Sun, Oct 15, 2017 at 04:56:28AM +0300, Michael S. Tsirkin wrote:
> On Tue, Oct 10, 2017 at 03:01:10PM -0300, Eduardo Habkost wrote:
> > On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
> > > On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
> > > > Hi
> > > > 
> > > > On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
> > > > <berrange@redhat.com> wrote:
> > > > > On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
> > > > >> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
> > > > >> > On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
> > > > >> > > On Mon, 9 Oct 2017 12:03:36 +0100
> > > > >> > > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > > >> > >
> > > > >> > > > On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
> > > > >> > > > > See docs/specs/vmcoreinfo.txt for details.
> > > > >> > > > >
> > > > >> > > > > "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
> > > > >> > > >
> > > > >> > > > I'm wondering if you considered just adding the entry to fw_cfg by
> > > > >> > > > default, without requiring any -device arg ? Unless I'm misunderstanding,
> > > > >> > > > this doesn't feel like a device to me - its just a well known bucket
> > > > >> > > > in fw_cfg IIUC ?  Obviously its existance would need to be tied to
> > > > >> > > > the latest machine type for ABI reasons though. The benefit of this
> > > > >> > > > is that it would "just work" without us having to plumb it through to
> > > > >> > > > all the downstream applications that use QEMU for mgmt guest (OpenStack,
> > > > >> > > > oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
> > > > >> > > it follows model set by pvpanic device, it's easier to manage from migration
> > > > >> > > POV, one could use it even for old machine types with new qemu (just by adding
> > > > >> > > device, it makes instance not backwards migratable to old qemu but should work
> > > > >> > > for forward migration) and if user doesn't need it, device could be just omitted
> > > > >> > > from CLI.
> > > > >> >
> > > > >> > Sure but it means that in effect no one will have this functionality enabled
> > > > >> > for several years. pvpanic has been around a long time and I rarely see it
> > > > >> > present in configured guests :-(
> > > > >> >
> > > > >> >
> > > > >> > Regards,
> > > > >> > Daniel
> > > > >>
> > > > >> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
> > > > >> shouldn't add optional devices anyway.
> > > > >
> > > > > This isn't really adding a device though is it - it is just a well known
> > > > > location in fw_cfg to receive data.
> > > > 
> > > > Enabling the device on some configurations by default can be done as a
> > > > follow-up patch. Can we get this series reviewed & merged?
> > > 
> > > The problem with the -device approach + turning it on by default is that there
> > > is no way to turn it off again if you don't want it. eg there's way to undo
> > > an implicit '-device foo' except via -nodefaults, but since libvirt uses that
> > > already it would negate the effect of enabling it by default unconditionally.
> > 
> > It's still possible to add a -machine option that can
> > enable/disable automatic creation of the device.
> > 
> > But I also don't see why it needs to be implemented using -device
> > if it's not really a device.  A boolean machine or fw_cfg
> > property is good enough for that.
> 
> It certainly feels like a device. It has state
> (that needs to be migrated), it has a host/guest interface.

(Sorry for the late reply)

That's convincing enough to me.  :)


> > > 
> > > Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
> > > respect, as you can trivially turn it on/off, overriding the default state
> > > in both directions.
> > 
> > Both "-global fw_cfg.vmcoreinfo=on|off" and
> > "-machine vmcoreinfo=on|off" sound good enough to me.
> 
> 
> Certainly not a fw cfg flag. Can be a machine flag I guess
> but then we'd have to open-code each such device.
> And don't forget auto - this is what Daniel asks for.

I'm not sure Daniel is really asking for "auto": he is just
asking for a way to disable the new default.  If "vmcoreinfo=off"
and "vmcoreinfo=off" works, there's no need for a user-visible
"auto" value.

(Actually, "auto" values makes compatibility code even messier,
because we would need one additional compat property/field to
tell QEMU what "auto" means on each machine)

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Cole Robinson]

On 10/20/2017 02:48 PM, Eduardo Habkost wrote:
> On Sun, Oct 15, 2017 at 04:56:28AM +0300, Michael S. Tsirkin wrote:
>> On Tue, Oct 10, 2017 at 03:01:10PM -0300, Eduardo Habkost wrote:
>>> On Tue, Oct 10, 2017 at 04:06:28PM +0100, Daniel P. Berrange wrote:
>>>> On Tue, Oct 10, 2017 at 05:00:18PM +0200, Marc-André Lureau wrote:
>>>>> Hi
>>>>>
>>>>> On Tue, Oct 10, 2017 at 10:31 AM, Daniel P. Berrange
>>>>> <berrange@redhat.com> wrote:
>>>>>> On Tue, Oct 10, 2017 at 12:44:26AM +0300, Michael S. Tsirkin wrote:
>>>>>>> On Mon, Oct 09, 2017 at 02:02:18PM +0100, Daniel P. Berrange wrote:
>>>>>>>> On Mon, Oct 09, 2017 at 02:43:44PM +0200, Igor Mammedov wrote:
>>>>>>>>> On Mon, 9 Oct 2017 12:03:36 +0100
>>>>>>>>> "Daniel P. Berrange" <berrange@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> On Mon, Sep 11, 2017 at 06:59:24PM +0200, Marc-André Lureau wrote:
>>>>>>>>>>> See docs/specs/vmcoreinfo.txt for details.
>>>>>>>>>>>
>>>>>>>>>>> "etc/vmcoreinfo" fw_cfg entry is added when using "-device vmcoreinfo".
>>>>>>>>>>
>>>>>>>>>> I'm wondering if you considered just adding the entry to fw_cfg by
>>>>>>>>>> default, without requiring any -device arg ? Unless I'm misunderstanding,
>>>>>>>>>> this doesn't feel like a device to me - its just a well known bucket
>>>>>>>>>> in fw_cfg IIUC ?  Obviously its existance would need to be tied to
>>>>>>>>>> the latest machine type for ABI reasons though. The benefit of this
>>>>>>>>>> is that it would "just work" without us having to plumb it through to
>>>>>>>>>> all the downstream applications that use QEMU for mgmt guest (OpenStack,
>>>>>>>>>> oVirt, GNOME Boxes, virt-manager, and countless other mgmt apps).
>>>>>>>>> it follows model set by pvpanic device, it's easier to manage from migration
>>>>>>>>> POV, one could use it even for old machine types with new qemu (just by adding
>>>>>>>>> device, it makes instance not backwards migratable to old qemu but should work
>>>>>>>>> for forward migration) and if user doesn't need it, device could be just omitted
>>>>>>>>> from CLI.
>>>>>>>>
>>>>>>>> Sure but it means that in effect no one will have this functionality enabled
>>>>>>>> for several years. pvpanic has been around a long time and I rarely see it
>>>>>>>> present in configured guests :-(
>>>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Daniel
>>>>>>>
>>>>>>> libvirt runs with -nodefaults, right? I'd argue pretty strongly -nodefaults
>>>>>>> shouldn't add optional devices anyway.
>>>>>>
>>>>>> This isn't really adding a device though is it - it is just a well known
>>>>>> location in fw_cfg to receive data.
>>>>>
>>>>> Enabling the device on some configurations by default can be done as a
>>>>> follow-up patch. Can we get this series reviewed & merged?
>>>>
>>>> The problem with the -device approach + turning it on by default is that there
>>>> is no way to turn it off again if you don't want it. eg there's way to undo
>>>> an implicit '-device foo' except via -nodefaults, but since libvirt uses that
>>>> already it would negate the effect of enabling it by default unconditionally.
>>>
>>> It's still possible to add a -machine option that can
>>> enable/disable automatic creation of the device.
>>>
>>> But I also don't see why it needs to be implemented using -device
>>> if it's not really a device.  A boolean machine or fw_cfg
>>> property is good enough for that.
>>
>> It certainly feels like a device. It has state
>> (that needs to be migrated), it has a host/guest interface.
> 
> (Sorry for the late reply)
> 
> That's convincing enough to me.  :)
> 
> 
>>>>
>>>> Your previous approach of "-global fw_cfg.vmcoreinfo=on" is nicer in this
>>>> respect, as you can trivially turn it on/off, overriding the default state
>>>> in both directions.
>>>
>>> Both "-global fw_cfg.vmcoreinfo=on|off" and
>>> "-machine vmcoreinfo=on|off" sound good enough to me.
>>
>>
>> Certainly not a fw cfg flag. Can be a machine flag I guess
>> but then we'd have to open-code each such device.
>> And don't forget auto - this is what Daniel asks for.
> 
> I'm not sure Daniel is really asking for "auto": he is just
> asking for a way to disable the new default.  If "vmcoreinfo=off"
> and "vmcoreinfo=off" works, there's no need for a user-visible
> "auto" value.
> 
> (Actually, "auto" values makes compatibility code even messier,
> because we would need one additional compat property/field to
> tell QEMU what "auto" means on each machine)
> 

Reviving this... did any follow up changes happen?

Marc-André patched virt-manager a few months back to enable -device
vmcoreinfo for new VMs:

https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html

And I see there's at least a bug tracking adding this to openstack for
new VMs:

https://bugzilla.redhat.com/show_bug.cgi?id=1555276

If this feature doesn't really have any downsides, it would be nice to
get this tied to new machine types. Saves a lot of churn for higher
levels of the stack

Thanks,
Cole

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 03:12:03PM -0400, Cole Robinson wrote:
[...]
> Reviving this... did any follow up changes happen?
> 
> Marc-André patched virt-manager a few months back to enable -device
> vmcoreinfo for new VMs:
> 
> https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html
> 
> And I see there's at least a bug tracking adding this to openstack for
> new VMs:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1555276
> 
> If this feature doesn't really have any downsides, it would be nice to
> get this tied to new machine types. Saves a lot of churn for higher
> levels of the stack

I understand this would be nice to have considering the existing
stacks, but at the same time I would like the rest of the
stack(s) to really try to not depend on QEMU machine-types to
define policy/defaults.

Every feature that is hidden behind an opaque machine-type name
and not visible in the domain XML and QEMU command-line increases
the risk of migration and compatibility bugs.

This was being discussed in a mail thread at:
https://www.mail-archive.com/ovirt-devel@redhat.com/msg01196.html

Quoting Daniel, on that thread:

] Another case is the pvpanic device - while in theory that could
] have been enabled by default for all guests, by QEMU or a config
] generator library, doing so is not useful on its own. The hard
] bit of the work is adding code to the mgmt app to choose the
] action for when pvpanic triggers, and code to handle the results
] of that action.

>From that comment, I understand that simply making QEMU create a
pvpanic device by default on pc-2.13+ won't be useful at all?

-- 
Eduardo

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Cole Robinson]

On 04/17/2018 05:11 PM, Eduardo Habkost wrote:
> On Tue, Apr 17, 2018 at 03:12:03PM -0400, Cole Robinson wrote:
> [...]
>> Reviving this... did any follow up changes happen?
>>
>> Marc-André patched virt-manager a few months back to enable -device
>> vmcoreinfo for new VMs:
>>
>> https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html
>>
>> And I see there's at least a bug tracking adding this to openstack for
>> new VMs:
>>
>> https://bugzilla.redhat.com/show_bug.cgi?id=1555276
>>
>> If this feature doesn't really have any downsides, it would be nice to
>> get this tied to new machine types. Saves a lot of churn for higher
>> levels of the stack
> 
> I understand this would be nice to have considering the existing
> stacks, but at the same time I would like the rest of the
> stack(s) to really try to not depend on QEMU machine-types to
> define policy/defaults.
> 
> Every feature that is hidden behind an opaque machine-type name
> and not visible in the domain XML and QEMU command-line increases
> the risk of migration and compatibility bugs.
> 

What exactly is the migration compatibility issue with turning on the
equivalent of -device vmcoreinfo for -M *-2.13+ ? Possibly prevents
backwards migration to older qemu but is that even a goal?

> This was being discussed in a mail thread at:
> https://www.mail-archive.com/ovirt-devel@redhat.com/msg01196.html
> 
> Quoting Daniel, on that thread:
> 
> ] Another case is the pvpanic device - while in theory that could
> ] have been enabled by default for all guests, by QEMU or a config
> ] generator library, doing so is not useful on its own. The hard
> ] bit of the work is adding code to the mgmt app to choose the
> ] action for when pvpanic triggers, and code to handle the results
> ] of that action.
> 
> From that comment, I understand that simply making QEMU create a
> pvpanic device by default on pc-2.13+ won't be useful at all?
> 

This qemu-devel thread was about -device vmcoreinfo though, not pvpanic.
vmcoreinfo doesn't need anything else to work AFAICT and shouldn't need
any explicit config, heck it doesn't even have any -device properties.

Like Dan says pvpanic isn't a 'just works' thing, and I know for windows
VMs it shows up in device manager which has considerations for things
like SVVP. I think vmcoreinfo doesn't have the same impact

There are some guest visible things that we have turned on for new
machine types in the past, pveoi and x2apic comes to mind.

Thanks,
Cole

Re: [Qemu-devel] [PATCH v6 2/7] hw/misc: add vmcoreinfo device

[Eduardo Habkost]

On Tue, Apr 17, 2018 at 06:31:57PM -0400, Cole Robinson wrote:
> On 04/17/2018 05:11 PM, Eduardo Habkost wrote:
> > On Tue, Apr 17, 2018 at 03:12:03PM -0400, Cole Robinson wrote:
> > [...]
> >> Reviving this... did any follow up changes happen?
> >>
> >> Marc-André patched virt-manager a few months back to enable -device
> >> vmcoreinfo for new VMs:
> >>
> >> https://www.redhat.com/archives/virt-tools-list/2018-February/msg00020.html
> >>
> >> And I see there's at least a bug tracking adding this to openstack for
> >> new VMs:
> >>
> >> https://bugzilla.redhat.com/show_bug.cgi?id=1555276
> >>
> >> If this feature doesn't really have any downsides, it would be nice to
> >> get this tied to new machine types. Saves a lot of churn for higher
> >> levels of the stack
> > 
> > I understand this would be nice to have considering the existing
> > stacks, but at the same time I would like the rest of the
> > stack(s) to really try to not depend on QEMU machine-types to
> > define policy/defaults.
> > 
> > Every feature that is hidden behind an opaque machine-type name
> > and not visible in the domain XML and QEMU command-line increases
> > the risk of migration and compatibility bugs.
> > 
> 
> What exactly is the migration compatibility issue with turning on the
> equivalent of -device vmcoreinfo for -M *-2.13+ ? Possibly prevents
> backwards migration to older qemu but is that even a goal?

I mean the extra migration compatibility code that needs to be
maintained on older machine-types.  It's extra maintenance burden
on both upstream and downstream QEMU trees.


> 
> > This was being discussed in a mail thread at:
> > https://www.mail-archive.com/ovirt-devel@redhat.com/msg01196.html
> > 
> > Quoting Daniel, on that thread:
> > 
> > ] Another case is the pvpanic device - while in theory that could
> > ] have been enabled by default for all guests, by QEMU or a config
> > ] generator library, doing so is not useful on its own. The hard
> > ] bit of the work is adding code to the mgmt app to choose the
> > ] action for when pvpanic triggers, and code to handle the results
> > ] of that action.
> > 
> > From that comment, I understand that simply making QEMU create a
> > pvpanic device by default on pc-2.13+ won't be useful at all?
> > 
> 
> This qemu-devel thread was about -device vmcoreinfo though, not pvpanic.
> vmcoreinfo doesn't need anything else to work AFAICT and shouldn't need
> any explicit config, heck it doesn't even have any -device properties.
> 
> Like Dan says pvpanic isn't a 'just works' thing, and I know for windows
> VMs it shows up in device manager which has considerations for things
> like SVVP. I think vmcoreinfo doesn't have the same impact
> 

Oops, nevermind.  I confused both.


> There are some guest visible things that we have turned on for new
> machine types in the past, pveoi and x2apic comes to mind.

Yes, we have tons of guest-visible things that we tie to the
machine-type.  What I'm looking for is a solution to make this
less frequent in the future.

-- 
Eduardo