* [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
@ 2017-07-14 18:20 Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
` (9 more replies)
0 siblings, 10 replies; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek, 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
memory range to the guest to store various informations useful to
debug the guest OS. (it is greatly inspired by the VMGENID device
implementation). The version field with value 0 is meant to give
paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
different purposes or OSes. (note: some wanted to see pvpanic somehow
merged with this device, I have no clear idea how to do that, nor do I
think this is a good idea since the devices are quite different, used
at different time for different purposes. And it can be done as a
future iteration if it is appropriate, feel free to send patches)
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.
By priority, VMCOREINFO "phys_base" value is the most accurate. If not
available, qemu will keep the current guessed value.
The series implements the VMCOREINFO note addition in qemu ELF/kdump,
as well as the python scripts/dump-guest-memory.py.
To test:
Compile and run a guest kernel with CONFIG_RANDOMIZE_BASE=y.
Run qemu with -device vmcoreinfo.
Load the experimental vmcoreinfo module in guest
https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c.
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
$ crash vmlinux dump
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 (8):
vmgenid: replace x-write-pointer-available hack
acpi: add vmcoreinfo device
stubs: add vmcoreinfo_get() stub
tests: add simple vmcoreinfo test
dump: add vmcoreinfo ELF note
kdump: add vmcoreinfo ELF note
scripts/dump-guest-memory.py: add vmcoreinfo
MAINTAINERS: add Dump maintainers
scripts/dump-guest-memory.py | 47 ++++++++
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/bios-linker-loader.h | 2 +
include/hw/acpi/vmcoreinfo.h | 37 ++++++
include/hw/compat.h | 4 -
include/sysemu/dump.h | 2 +
dump.c | 154 +++++++++++++++++++++++++
hw/acpi/aml-build.c | 2 +
hw/acpi/bios-linker-loader.c | 10 ++
hw/acpi/vmcoreinfo.c | 211 +++++++++++++++++++++++++++++++++++
hw/acpi/vmgenid.c | 9 +-
hw/i386/acpi-build.c | 14 +++
stubs/vmcoreinfo.c | 9 ++
tests/vmcoreinfo-test.c | 141 +++++++++++++++++++++++
MAINTAINERS | 9 ++
default-configs/arm-softmmu.mak | 1 +
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
docs/specs/vmcoreinfo.txt | 138 +++++++++++++++++++++++
hw/acpi/Makefile.objs | 1 +
stubs/Makefile.objs | 1 +
tests/Makefile.include | 2 +
22 files changed, 785 insertions(+), 12 deletions(-)
create mode 100644 include/hw/acpi/vmcoreinfo.h
create mode 100644 hw/acpi/vmcoreinfo.c
create mode 100644 stubs/vmcoreinfo.c
create mode 100644 tests/vmcoreinfo-test.c
create mode 100644 docs/specs/vmcoreinfo.txt
--
2.13.1.395.gf7b71de06
^ permalink raw reply [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 19:19 ` Michael S. Tsirkin
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device Marc-André Lureau
` (8 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek,
Marc-André Lureau, Michael S. Tsirkin, Ben Warren
This compat property sole function is to prevent the device from being
instantiated. Instead of requiring an extra compat property, check if
fw_cfg has DMA enabled.
fw_cfg is a built-in device that is initialized very early by the
machine init code. We have at least one other device that also
assumes fw_cfg_find() can be safely used on realize: pvpanic.
This has the additional benefit of handling other cases properly, like:
$ qemu-system-x86_64 -device vmgenid -machine none
qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support in fw_cfg, which this machine type does not provide
$ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.9 -global fw_cfg.dma_enabled=off
qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support in fw_cfg, which this machine type does not provide
$ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.6 -global fw_cfg.dma_enabled=on
[boots normally]
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
Reviewed-by: Ben Warren <ben@skyportsystems.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
include/hw/acpi/bios-linker-loader.h | 2 ++
include/hw/compat.h | 4 ----
hw/acpi/bios-linker-loader.c | 10 ++++++++++
hw/acpi/vmgenid.c | 9 +--------
4 files changed, 13 insertions(+), 12 deletions(-)
diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
index efe17b0b9c..a711dbced8 100644
--- a/include/hw/acpi/bios-linker-loader.h
+++ b/include/hw/acpi/bios-linker-loader.h
@@ -7,6 +7,8 @@ typedef struct BIOSLinker {
GArray *file_list;
} BIOSLinker;
+bool bios_linker_loader_can_write_pointer(void);
+
BIOSLinker *bios_linker_loader_init(void);
void bios_linker_loader_alloc(BIOSLinker *linker,
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 08f36004da..f414786604 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -150,10 +150,6 @@
.driver = "fw_cfg_io",\
.property = "dma_enabled",\
.value = "off",\
- },{\
- .driver = "vmgenid",\
- .property = "x-write-pointer-available",\
- .value = "off",\
},
#define HW_COMPAT_2_3 \
diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
index 046183a0f1..d16b8bbcb1 100644
--- a/hw/acpi/bios-linker-loader.c
+++ b/hw/acpi/bios-linker-loader.c
@@ -168,6 +168,16 @@ bios_linker_find_file(const BIOSLinker *linker, const char *name)
return NULL;
}
+/*
+ * board code must realize fw_cfg first, as a fixed device, before
+ * another device realize function call bios_linker_loader_can_write_pointer()
+ */
+bool bios_linker_loader_can_write_pointer(void)
+{
+ FWCfgState *fw_cfg = fw_cfg_find();
+ return fw_cfg && fw_cfg_dma_enabled(fw_cfg);
+}
+
/*
* bios_linker_loader_alloc: ask guest to load file into guest memory.
*
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
index a32b847fe0..ab5da293fd 100644
--- a/hw/acpi/vmgenid.c
+++ b/hw/acpi/vmgenid.c
@@ -205,17 +205,11 @@ static void vmgenid_handle_reset(void *opaque)
memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
}
-static Property vmgenid_properties[] = {
- DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
- write_pointer_available, true),
- DEFINE_PROP_END_OF_LIST(),
-};
-
static void vmgenid_realize(DeviceState *dev, Error **errp)
{
VmGenIdState *vms = VMGENID(dev);
- if (!vms->write_pointer_available) {
+ if (!bios_linker_loader_can_write_pointer()) {
error_setg(errp, "%s requires DMA write support in fw_cfg, "
"which this machine type does not provide", VMGENID_DEVICE);
return;
@@ -239,7 +233,6 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data)
dc->vmsd = &vmstate_vmgenid;
dc->realize = vmgenid_realize;
dc->hotpluggable = false;
- dc->props = vmgenid_properties;
object_class_property_add_str(klass, VMGENID_GUID, NULL,
vmgenid_set_guid, NULL);
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 19:26 ` Michael S. Tsirkin
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 3/8] stubs: add vmcoreinfo_get() stub Marc-André Lureau
` (7 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek,
Marc-André Lureau, Michael S. Tsirkin, Paolo Bonzini,
Richard Henderson
The VM coreinfo (vmcoreinfo) device is an emulated device which
exposes a 4k memory range to the guest to store various informations
useful to debug the guest OS. (it is greatly inspired by the VMGENID
device implementation)
This is an early-boot alternative to the qemu-ga VMDUMP_INFO event
proposed in "[PATCH 00/21] WIP: dump: add kaslr support".
A proof-of-concept kernel module:
https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/vmcoreinfo.h | 37 +++++++
hw/acpi/aml-build.c | 2 +
hw/acpi/vmcoreinfo.c | 208 +++++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 14 +++
default-configs/arm-softmmu.mak | 1 +
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
docs/specs/vmcoreinfo.txt | 138 ++++++++++++++++++++++++
hw/acpi/Makefile.objs | 1 +
10 files changed, 404 insertions(+)
create mode 100644 include/hw/acpi/vmcoreinfo.h
create mode 100644 hw/acpi/vmcoreinfo.c
create mode 100644 docs/specs/vmcoreinfo.txt
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 88d0738d76..cf781bcd34 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -211,6 +211,7 @@ struct AcpiBuildTables {
GArray *rsdp;
GArray *tcpalog;
GArray *vmgenid;
+ GArray *vmcoreinfo;
BIOSLinker *linker;
} AcpiBuildTables;
diff --git a/include/hw/acpi/vmcoreinfo.h b/include/hw/acpi/vmcoreinfo.h
new file mode 100644
index 0000000000..6a73bcd1b2
--- /dev/null
+++ b/include/hw/acpi/vmcoreinfo.h
@@ -0,0 +1,37 @@
+#ifndef ACPI_VMCOREINFO_H
+#define ACPI_VMCOREINFO_H
+
+#include "hw/acpi/bios-linker-loader.h"
+#include "hw/qdev.h"
+
+#define VMCOREINFO_DEVICE "vmcoreinfo"
+#define VMCOREINFO_FW_CFG_FILE "etc/vmcoreinfo"
+#define VMCOREINFO_ADDR_FW_CFG_FILE "etc/vmcoreinfo-addr"
+
+/* Occupy a page of memory */
+#define VMCOREINFO_FW_CFG_SIZE 4096
+
+/* allow space for OVMF SDT Header Probe Supressor */
+#define VMCOREINFO_OFFSET sizeof(AcpiTableHeader)
+
+#define VMCOREINFO(obj) OBJECT_CHECK(VMCoreInfoState, (obj), VMCOREINFO_DEVICE)
+
+typedef struct VMCoreInfoState {
+ DeviceClass parent_obj;
+ uint8_t vmcoreinfo_addr_le[8]; /* Address of memory region */
+ bool write_pointer_available;
+} VMCoreInfoState;
+
+/* returns NULL unless there is exactly one device */
+static inline Object *find_vmcoreinfo_dev(void)
+{
+ return object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
+}
+
+void vmcoreinfo_build_acpi(VMCoreInfoState *vis, GArray *table_data,
+ GArray *vmci, BIOSLinker *linker);
+void vmcoreinfo_add_fw_cfg(VMCoreInfoState *vis, FWCfgState *s, GArray *vmci);
+bool vmcoreinfo_get(VMCoreInfoState *vis, uint64_t *paddr, uint32_t *size,
+ Error **errp);
+
+#endif
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 36a6cc450e..47043ade4a 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -1561,6 +1561,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables)
tables->table_data = g_array_new(false, true /* clear */, 1);
tables->tcpalog = g_array_new(false, true /* clear */, 1);
tables->vmgenid = g_array_new(false, true /* clear */, 1);
+ tables->vmcoreinfo = g_array_new(false, true /* clear */, 1);
tables->linker = bios_linker_loader_init();
}
@@ -1571,6 +1572,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
g_array_free(tables->table_data, true);
g_array_free(tables->tcpalog, mfre);
g_array_free(tables->vmgenid, mfre);
+ g_array_free(tables->vmcoreinfo, mfre);
}
/* Build rsdt table */
diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
new file mode 100644
index 0000000000..0ea41de8d9
--- /dev/null
+++ b/hw/acpi/vmcoreinfo.c
@@ -0,0 +1,208 @@
+/*
+ * Virtual Machine coreinfo device
+ * (based on Virtual Machine Generation ID Device)
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
+ * Copyright (C) 2017 Skyport Systems.
+ *
+ * Authors: Marc-André Lureau <marcandre.lureau@redhat.com>
+ * Ben Warren <ben@skyportsystems.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 "hw/acpi/acpi.h"
+#include "hw/acpi/aml-build.h"
+#include "hw/acpi/vmcoreinfo.h"
+#include "hw/nvram/fw_cfg.h"
+#include "sysemu/sysemu.h"
+#include "qapi/error.h"
+
+void vmcoreinfo_build_acpi(VMCoreInfoState *vis, GArray *table_data,
+ GArray *vmci, BIOSLinker *linker)
+{
+ Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
+ uint32_t vcia_offset;
+
+ g_array_set_size(vmci, VMCOREINFO_FW_CFG_SIZE);
+
+ /* Put this in a separate SSDT table */
+ ssdt = init_aml_allocator();
+
+ /* Reserve space for header */
+ acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
+
+ /* Storage address */
+ vcia_offset = table_data->len +
+ build_append_named_dword(ssdt->buf, "VCIA");
+ scope = aml_scope("\\_SB");
+ dev = aml_device("VMCI");
+ aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVMCI")));
+
+ /* Simple status method to check that address is linked and non-zero */
+ method = aml_method("_STA", 0, AML_NOTSERIALIZED);
+ addr = aml_local(0);
+ aml_append(method, aml_store(aml_int(0xf), addr));
+ if_ctx = aml_if(aml_equal(aml_name("VCIA"), aml_int(0)));
+ aml_append(if_ctx, aml_store(aml_int(0), addr));
+ aml_append(method, if_ctx);
+ aml_append(method, aml_return(addr));
+ aml_append(dev, method);
+
+ /* the ADDR method returns two 32-bit words representing the lower and
+ * upper halves of the physical address of the vmcoreinfo area
+ */
+ method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
+
+ addr = aml_local(0);
+ aml_append(method, aml_store(aml_package(2), addr));
+
+ aml_append(method, aml_store(aml_add(aml_name("VCIA"),
+ aml_int(VMCOREINFO_OFFSET), NULL),
+ aml_index(addr, aml_int(0))));
+ aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1))));
+ aml_append(method, aml_return(addr));
+
+ aml_append(dev, method);
+ aml_append(scope, dev);
+ aml_append(ssdt, scope);
+
+ g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
+
+ /* Allocate guest memory */
+ bios_linker_loader_alloc(linker, VMCOREINFO_FW_CFG_FILE, vmci, 4096,
+ false /* page boundary, high memory */);
+
+ /* Patch address of vmcoreinfo fw_cfg blob into the ADDR fw_cfg
+ * blob so QEMU can read the info from there. The address is
+ * expected to be < 4GB, but write 64 bits anyway.
+ * The address that is patched in is offset in order to implement
+ * the "OVMF SDT Header probe suppressor"
+ * see docs/specs/vmcoreinfo.txt for more details.
+ */
+ bios_linker_loader_write_pointer(linker,
+ VMCOREINFO_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
+ VMCOREINFO_FW_CFG_FILE, VMCOREINFO_OFFSET);
+
+ /* Patch address of vmcoreinfo into the AML so OSPM can retrieve
+ * and read it. Note that while we provide storage for 64 bits, only
+ * the least-signficant 32 get patched into AML.
+ */
+ bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE, vcia_offset, sizeof(uint32_t),
+ VMCOREINFO_FW_CFG_FILE, 0);
+
+ build_header(linker, table_data,
+ (void *)(table_data->data + table_data->len - ssdt->buf->len),
+ "SSDT", ssdt->buf->len, 1, NULL, "VMCOREIN");
+ free_aml_allocator();
+}
+
+void vmcoreinfo_add_fw_cfg(VMCoreInfoState *vis, FWCfgState *s, GArray *vmci)
+{
+ /* Create a read-only fw_cfg file for vmcoreinfo allocation */
+ /* XXX: linker could learn to allocate without backing fw_cfg? */
+ fw_cfg_add_file(s, VMCOREINFO_FW_CFG_FILE, vmci->data,
+ VMCOREINFO_FW_CFG_SIZE);
+ /* Create a read-write fw_cfg file for Address */
+ fw_cfg_add_file_callback(s, VMCOREINFO_ADDR_FW_CFG_FILE, NULL, NULL,
+ vis->vmcoreinfo_addr_le,
+ ARRAY_SIZE(vis->vmcoreinfo_addr_le), false);
+}
+
+bool vmcoreinfo_get(VMCoreInfoState *vis,
+ uint64_t *paddr, uint32_t *size,
+ Error **errp)
+{
+ uint32_t vmcoreinfo_addr;
+ uint32_t version;
+
+ assert(vis);
+ assert(paddr);
+ assert(size);
+
+ memcpy(&vmcoreinfo_addr, vis->vmcoreinfo_addr_le, sizeof(vmcoreinfo_addr));
+ vmcoreinfo_addr = le32_to_cpu(vmcoreinfo_addr);
+ if (!vmcoreinfo_addr) {
+ error_setg(errp, "BIOS has not yet written the address of %s",
+ VMCOREINFO_DEVICE);
+ return false;
+ }
+
+ cpu_physical_memory_read(vmcoreinfo_addr, &version, sizeof(version));
+ if (version != 0) {
+ error_setg(errp, "Unknown %s memory version", VMCOREINFO_DEVICE);
+ return false;
+ }
+
+ cpu_physical_memory_read(vmcoreinfo_addr + 4, paddr, sizeof(*paddr));
+ *paddr = le64_to_cpu(*paddr);
+ cpu_physical_memory_read(vmcoreinfo_addr + 12, size, sizeof(*size));
+ *size = le32_to_cpu(*size);
+
+ return true;
+}
+
+static const VMStateDescription vmstate_vmcoreinfo = {
+ .name = "vmcoreinfo",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_ARRAY(vmcoreinfo_addr_le, VMCoreInfoState, sizeof(uint64_t)),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static void vmcoreinfo_handle_reset(void *opaque)
+{
+ VMCoreInfoState *vis = VMCOREINFO(opaque);
+
+ /* Clear the guest-allocated address when the VM resets */
+ memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
+}
+
+static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
+{
+ if (!bios_linker_loader_can_write_pointer()) {
+ error_setg(errp, "%s requires DMA write support in fw_cfg, "
+ "which this machine type does not provide",
+ VMCOREINFO_DEVICE);
+ return;
+ }
+
+ /* Given that this function is executing, there is at least one VMCOREINFO
+ * device. Check if there are several.
+ */
+ if (!find_vmcoreinfo_dev()) {
+ error_setg(errp, "at most one %s device is permitted",
+ VMCOREINFO_DEVICE);
+ return;
+ }
+
+ qemu_register_reset(vmcoreinfo_handle_reset, dev);
+}
+
+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/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6b7bade183..7ac529680e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -43,6 +43,7 @@
#include "sysemu/tpm.h"
#include "hw/acpi/tpm.h"
#include "hw/acpi/vmgenid.h"
+#include "hw/acpi/vmcoreinfo.h"
#include "sysemu/tpm_backend.h"
#include "hw/timer/mc146818rtc_regs.h"
#include "sysemu/numa.h"
@@ -2631,6 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
GArray *tables_blob = tables->table_data;
AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
Object *vmgenid_dev;
+ Object *vmcoreinfo_dev;
acpi_get_pm_info(&pm);
acpi_get_misc_info(&misc);
@@ -2680,6 +2682,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
vmgenid_build_acpi(VMGENID(vmgenid_dev), tables_blob,
tables->vmgenid, tables->linker);
}
+ vmcoreinfo_dev = find_vmcoreinfo_dev();
+ if (vmcoreinfo_dev) {
+ acpi_add_table(table_offsets, tables_blob);
+ vmcoreinfo_build_acpi(VMCOREINFO(vmcoreinfo_dev), tables_blob,
+ tables->vmcoreinfo, tables->linker);
+ }
if (misc.has_hpet) {
acpi_add_table(table_offsets, tables_blob);
@@ -2856,6 +2864,7 @@ void acpi_setup(void)
AcpiBuildTables tables;
AcpiBuildState *build_state;
Object *vmgenid_dev;
+ Object *vmcoreinfo_dev;
if (!pcms->fw_cfg) {
ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
@@ -2897,6 +2906,11 @@ void acpi_setup(void)
vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
tables.vmgenid);
}
+ vmcoreinfo_dev = find_vmcoreinfo_dev();
+ if (vmcoreinfo_dev) {
+ vmcoreinfo_add_fw_cfg(VMCOREINFO(vmcoreinfo_dev), pcms->fw_cfg,
+ tables.vmcoreinfo);
+ }
if (!pcmc->rsdp_in_ram) {
/*
diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index 93e995d318..320dd3680d 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -120,6 +120,7 @@ CONFIG_XIO3130=y
CONFIG_IOH3420=y
CONFIG_I82801B11=y
CONFIG_ACPI=y
+CONFIG_ACPI_VMCOREINFO=y
CONFIG_SMBIOS=y
CONFIG_ASPEED_SOC=y
CONFIG_GPIO_KEY=y
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index d2ab2f6655..df68628895 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
CONFIG_PXB=y
CONFIG_ACPI_VMGENID=y
+CONFIG_ACPI_VMCOREINFO=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 9bde2f1c4b..e39ad5a680 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
CONFIG_PXB=y
CONFIG_ACPI_VMGENID=y
+CONFIG_ACPI_VMCOREINFO=y
diff --git a/docs/specs/vmcoreinfo.txt b/docs/specs/vmcoreinfo.txt
new file mode 100644
index 0000000000..36d5a39ab1
--- /dev/null
+++ b/docs/specs/vmcoreinfo.txt
@@ -0,0 +1,138 @@
+VIRTUAL MACHINE COREINFO DEVICE
+===============================
+
+Copyright (C) 2017 Red Hat, Inc.
+
+This work is licensed under the terms of the GNU GPL, version 2 or later.
+See the COPYING file in the top-level directory.
+
+===
+
+The VM coreinfo (vmcoreinfo) device is an emulated device which
+exposes a 4k memory range to the guest to store various informations
+useful to debug the guest OS.
+
+QEMU Implementation
+-------------------
+
+The vmcoreinfo device is put in its own ACPI descriptor table, in a
+Secondary System Description Table, or SSDT.
+
+The following is a dump of the contents from a running system:
+
+# iasl -p ./SSDT -d /sys/firmware/acpi/tables/SSDT
+/*
+ * Intel ACPI Component Architecture
+ * AML/ASL+ Disassembler version 20160831-64
+ * Copyright (c) 2000 - 2016 Intel Corporation
+ *
+ * Disassembling to symbolic ASL+ operators
+ *
+ * Disassembly of /sys/firmware/acpi/tables/SSDT, Mon Apr 24 15:59:53 2017
+ *
+ * Original Table Header:
+ * Signature "SSDT"
+ * Length 0x00000086 (134)
+ * Revision 0x01
+ * Checksum 0x5C
+ * OEM ID "BOCHS "
+ * OEM Table ID "VMCOREIN"
+ * OEM Revision 0x00000001 (1)
+ * Compiler ID "BXPC"
+ * Compiler Version 0x00000001 (1)
+ */
+DefinitionBlock ("", "SSDT", 1, "BOCHS ", "VMCOREIN", 0x00000001)
+{
+ Name (VCIA, 0x3FFFF000)
+ Scope (\_SB)
+ {
+ Device (VMCI)
+ {
+ Name (_HID, "QEMUVMCI") // _HID: Hardware ID
+ Method (_STA, 0, NotSerialized) // _STA: Status
+ {
+ Local0 = 0x0F
+ If (VCIA == Zero)
+ {
+ Local0 = Zero
+ }
+
+ Return (Local0)
+ }
+
+ Method (ADDR, 0, NotSerialized)
+ {
+ Local0 = Package (0x02) {}
+ Local0 [Zero] = (VCIA + 0x24)
+ Local0 [One] = Zero
+ Return (Local0)
+ }
+ }
+ }
+}
+
+
+Design Details:
+---------------
+
+QEMU must be able to read the contents of the device memory,
+specifically when starting a memory dump. In order to do this, QEMU
+must know the address that has been allocated.
+
+The mechanism chosen for this memory sharing is writeable fw_cfg blobs.
+These are data object that are visible to both QEMU and guests, and are
+addressable as sequential files.
+
+More information about fw_cfg can be found in "docs/specs/fw_cfg.txt"
+
+Two fw_cfg blobs are used in this case:
+
+/etc/vmcoreinfo - used to allocate memory range, read-only to the guest
+/etc/vmcoreinfo-addr - contains the address of the allocated range
+ - writeable by the guest
+
+
+QEMU sends the following commands to the guest at startup:
+
+1. Allocate memory for vmcoreinfo fw_cfg blob.
+2. Write the address of vmcoreinfo into the SSDT (VCIA ACPI variable as
+ shown above in the iasl dump). Note that this change is not propagated
+ back to QEMU.
+3. Write the address of vmcoreinfo back to QEMU's copy of vmcoreinfo-addr
+ via the fw_cfg DMA interface.
+
+After step 3, QEMU is able to read the contents of vmcoreinfo.
+
+The value of VCIA is persisted via the VMState mechanism.
+
+
+Storage Format:
+---------------
+
+The content is expected to use little-endian format.
+
+In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
+the vmcoreinfo blob has 36 bytes of padding:
+
++-----------------------------------+
+| SSDT with OEM Table ID = VMCOREIN |
++-----------------------------------+
+| ... | TOP OF PAGE
+| VCIA dword object ----------------|-----> +---------------------------+
+| ... | | fw-allocated array for |
+| _STA method referring to VCIA | | "etc/vmcoreinfo" |
+| ... | +---------------------------+
+| ADDR method referring to VCIA | | 0: OVMF SDT Header probe |
+| ... | | suppressor |
++-----------------------------------+ | 36: uint32 version field |
+ | 40: info contents |
+ | .... |
+ +---------------------------+
+ END OF PAGE
+
+Version 0 content:
+
+ uint64 paddr:
+ Physical address of the Linux vmcoreinfo ELF note.
+ uint32 size:
+ Size of the vmcoreinfo ELF note.
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index 11c35bcb44..9623078f95 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
+common-obj-$(CONFIG_ACPI_VMCOREINFO) += vmcoreinfo.o
common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
common-obj-y += acpi_interface.o
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 3/8] stubs: add vmcoreinfo_get() stub
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 20:09 ` Laszlo Ersek
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 4/8] tests: add simple vmcoreinfo test Marc-André Lureau
` (6 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek,
Marc-André Lureau, Paolo Bonzini
Common dump code may call vmcoreinfo_get() which is not available on
all targets.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
stubs/vmcoreinfo.c | 9 +++++++++
stubs/Makefile.objs | 1 +
2 files changed, 10 insertions(+)
create mode 100644 stubs/vmcoreinfo.c
diff --git a/stubs/vmcoreinfo.c b/stubs/vmcoreinfo.c
new file mode 100644
index 0000000000..a994153832
--- /dev/null
+++ b/stubs/vmcoreinfo.c
@@ -0,0 +1,9 @@
+#include "qemu/osdep.h"
+
+#include "hw/acpi/vmcoreinfo.h"
+
+bool vmcoreinfo_get(VMCoreInfoState *vis, uint64_t *paddr, uint32_t *size,
+ Error **errp)
+{
+ return false;
+}
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index f5b47bfd74..3bedc73252 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -36,6 +36,7 @@ stub-obj-y += qmp_pc_dimm_device_list.o
stub-obj-y += target-monitor-defs.o
stub-obj-y += target-get-monitor-def.o
stub-obj-y += pc_madt_cpu_entry.o
+stub-obj-y += vmcoreinfo.o
stub-obj-y += vmgenid.o
stub-obj-y += xen-common.o
stub-obj-y += xen-hvm.o
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 4/8] tests: add simple vmcoreinfo test
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (2 preceding siblings ...)
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 3/8] stubs: add vmcoreinfo_get() stub Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 20:10 ` Laszlo Ersek
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 5/8] dump: add vmcoreinfo ELF note Marc-André Lureau
` (5 subsequent siblings)
9 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek, Marc-André Lureau
This test is based off vmgenid test from Ben Warren
<ben@skyportsystems.com>. It simply checks the vmcoreinfo ACPI device
is present and that the memory region associated can be read.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
tests/vmcoreinfo-test.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
tests/Makefile.include | 2 +
2 files changed, 143 insertions(+)
create mode 100644 tests/vmcoreinfo-test.c
diff --git a/tests/vmcoreinfo-test.c b/tests/vmcoreinfo-test.c
new file mode 100644
index 0000000000..a54f3b5c6a
--- /dev/null
+++ b/tests/vmcoreinfo-test.c
@@ -0,0 +1,141 @@
+/*
+ * QTest testcase for VM coreinfo device
+ *
+ * Copyright (c) 2017 Red Hat, Inc.
+ * Copyright (c) 2017 Skyport Systems
+ *
+ * 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 <glib.h>
+#include <string.h>
+#include <unistd.h>
+#include "qemu/osdep.h"
+#include "qemu/bitmap.h"
+#include "qemu/uuid.h"
+#include "hw/acpi/acpi-defs.h"
+#include "boot-sector.h"
+#include "acpi-utils.h"
+#include "libqtest.h"
+
+#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
+
+typedef struct {
+ AcpiTableHeader header;
+ gchar name_op;
+ gchar vcia[4];
+ gchar val_op;
+ uint32_t vcia_val;
+} QEMU_PACKED VmciTable;
+
+static uint32_t acpi_find_vcia(void)
+{
+ uint32_t rsdp_offset;
+ AcpiRsdpDescriptor rsdp_table;
+ uint32_t rsdt;
+ AcpiRsdtDescriptorRev1 rsdt_table;
+ int tables_nr;
+ uint32_t *tables;
+ AcpiTableHeader ssdt_table;
+ VmciTable vmci_table;
+ int i;
+
+ /* Wait for guest firmware to finish and start the payload. */
+ boot_sector_test();
+
+ /* Tables should be initialized now. */
+ rsdp_offset = acpi_find_rsdp_address();
+
+ g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
+
+ acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
+
+ rsdt = rsdp_table.rsdt_physical_address;
+ /* read the header */
+ ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
+ ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
+
+ /* compute the table entries in rsdt */
+ tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
+ sizeof(uint32_t);
+ g_assert_cmpint(tables_nr, >, 0);
+
+ /* get the addresses of the tables pointed by rsdt */
+ tables = g_new0(uint32_t, tables_nr);
+ ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
+
+ for (i = 0; i < tables_nr; i++) {
+ ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
+ if (!strncmp((char *)ssdt_table.oem_table_id, "VMCOREIN", 8)) {
+ /* the first entry in the table should be VCIA
+ * That's all we need
+ */
+ ACPI_READ_FIELD(vmci_table.name_op, tables[i]);
+ g_assert(vmci_table.name_op == 0x08); /* name */
+ ACPI_READ_ARRAY(vmci_table.vcia, tables[i]);
+ g_assert(memcmp(vmci_table.vcia, "VCIA", 4) == 0);
+ ACPI_READ_FIELD(vmci_table.val_op, tables[i]);
+ g_assert(vmci_table.val_op == 0x0C); /* dword */
+ ACPI_READ_FIELD(vmci_table.vcia_val, tables[i]);
+ /* The GUID is written at a fixed offset into the fw_cfg file
+ * in order to implement the "OVMF SDT Header probe suppressor"
+ * see docs/specs/vmgenid.txt for more details
+ */
+ g_free(tables);
+ return vmci_table.vcia_val;
+ }
+ }
+ g_free(tables);
+ return 0;
+}
+
+static char disk[] = "tests/vmci-test-disk-XXXXXX";
+
+static char *cmd_strdup(void)
+{
+ return g_strdup_printf("-machine accel=tcg "
+ "-device vmcoreinfo,id=testvmvci "
+ "-drive id=hd0,if=none,file=%s,format=raw "
+ "-device ide-hd,drive=hd0 ",
+ disk);
+}
+
+static void vmcoreinfo_test(void)
+{
+ gchar *cmd;
+ uint32_t vmci_addr;
+ int i;
+
+ cmd = cmd_strdup();
+ qtest_start(cmd);
+
+ vmci_addr = acpi_find_vcia();
+ g_assert(vmci_addr);
+
+ for (i = 0; i < 4096; i++) {
+ /* check the memory region can be read */
+ readb(vmci_addr + i);
+ }
+
+ qtest_quit(global_qtest);
+ g_free(cmd);
+}
+
+int main(int argc, char **argv)
+{
+ int ret;
+
+ ret = boot_sector_init(disk);
+ if (ret) {
+ return ret;
+ }
+
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_add_func("/vmcoreinfo/test", vmcoreinfo_test);
+ ret = g_test_run();
+ boot_sector_cleanup(disk);
+
+ return ret;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index cfbb689e0e..dd515f2159 100644
--- a/tests/Makefile.include
+++ b/tests/Makefile.include
@@ -251,6 +251,7 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
check-qtest-i386-y += tests/q35-test$(EXESUF)
check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
+check-qtest-i386-y += tests/vmcoreinfo-test$(EXESUF)
gcov-files-i386-y += hw/pci-host/q35.c
check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
@@ -762,6 +763,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
tests/numa-test$(EXESUF): tests/numa-test.o
tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
+tests/vmcoreinfo-test$(EXESUF): tests/vmcoreinfo-test.o tests/boot-sector.o tests/acpi-utils.o
tests/migration/stress$(EXESUF): tests/migration/stress.o
$(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 5/8] dump: add vmcoreinfo ELF note
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (3 preceding siblings ...)
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 4/8] tests: add simple vmcoreinfo test Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 6/8] kdump: " Marc-André Lureau
` (4 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek, Marc-André Lureau
Read the vmcoreinfo ELF PT_NOTE from guest memory when vmcoreinfo
device provides the location, and write it as an ELF note in the dump.
There are now 2 possible sources of phys_base information.
(1) arch guessed value from cpu_get_dump_info()
(2) vmcoreinfo ELF note NUMBER(phys_base)= field
NUMBER(phys_base) in vmcoreinfo has only been recently introduced
in Linux 4.10 (401721ecd1dc "kexec: export the value of phys_base
instead of symbol address").
Since (2) has better chances to be accurate, the guessed value is
replaced by the value from the vmcoreinfo ELF note.
The phys_base value is stored in the same dump field locations as
before, and may duplicate the information available in the vmcoreinfo
ELF PT_NOTE. Crash tools should be prepared to handle this case.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
include/sysemu/dump.h | 2 +
dump.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 136 insertions(+)
diff --git a/include/sysemu/dump.h b/include/sysemu/dump.h
index 2672a15f8b..111a7dcaa4 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 *vmcoreinfo; /* ELF note content */
+ size_t vmcoreinfo_size;
} DumpState;
uint16_t cpu_to_dump16(DumpState *s, uint16_t val);
diff --git a/dump.c b/dump.c
index d9090a24cc..d86b601248 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/acpi/vmcoreinfo.h"
#include <zlib.h>
#ifdef CONFIG_LZO
@@ -38,6 +40,13 @@
#define ELF_MACHINE_UNAME "Unknown"
#endif
+#define MAX_VMCOREINFO_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->vmcoreinfo);
+ s->vmcoreinfo = 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_vmcoreinfo_note(WriteCoreDumpFunction f, DumpState *s,
+ Error **errp)
+{
+ int ret;
+
+ if (s->vmcoreinfo) {
+ ret = f(s->vmcoreinfo, s->vmcoreinfo_size, s);
+ if (ret < 0) {
+ error_setg(errp, "dump: failed to write vmcoreinfo");
+ }
+ }
+}
+
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_vmcoreinfo_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_vmcoreinfo_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)
{
@@ -1488,10 +1554,40 @@ 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;
+
+ get_note_sizes(s, s->vmcoreinfo, ¬e_head_size, &name_size, &size);
+ note_head_size = ROUND_UP(note_head_size, 4);
+ name_size = ROUND_UP(name_size, 4);
+ vmci = s->vmcoreinfo + note_head_size + name_size;
+ *(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)
{
+ Object *vmcoreinfo_dev = find_vmcoreinfo_dev();
CPUState *cpu;
int nr_cpus;
Error *err = NULL;
@@ -1563,6 +1659,44 @@ static void dump_init(DumpState *s, int fd, bool has_format,
goto cleanup;
}
+ /*
+ * The goal of this block is to (a) update the previously guessed
+ * phys_base, (b) copy the vmcoreinfo note out of the guest.
+ * Failure to do so is not fatal for dumping.
+ */
+ if (vmcoreinfo_dev) {
+ uint64_t addr, note_head_size, name_size, desc_size;
+ uint32_t size;
+
+ note_head_size = s->dump_info.d_class == ELFCLASS32 ?
+ sizeof(Elf32_Nhdr) : sizeof(Elf64_Nhdr);
+
+ if (!vmcoreinfo_get(VMCOREINFO(vmcoreinfo_dev),
+ &addr, &size, &err)) {
+ warn_report_err(err);
+ err = NULL;
+ } else if (size < note_head_size || size > MAX_VMCOREINFO_SIZE) {
+ warn_report("vmcoreinfo size is invalid: %" PRIu32, size);
+ } else {
+ s->vmcoreinfo = g_malloc(size + 1); /* +1 for adding \0 */
+ cpu_physical_memory_read(addr, s->vmcoreinfo, size);
+
+ get_note_sizes(s, s->vmcoreinfo, NULL, &name_size, &desc_size);
+ s->vmcoreinfo_size = ELF_NOTE_SIZE(note_head_size, name_size,
+ desc_size);
+ if (name_size > MAX_VMCOREINFO_SIZE ||
+ desc_size > MAX_VMCOREINFO_SIZE ||
+ s->vmcoreinfo_size > size) {
+ warn_report("Invalid vmcoreinfo header");
+ g_free(s->vmcoreinfo);
+ s->vmcoreinfo = NULL;
+ } else {
+ vmcoreinfo_update_phys_base(s);
+ s->note_size += s->vmcoreinfo_size;
+ }
+ }
+ }
+
/* get memory mapping */
if (paging) {
qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, &err);
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 6/8] kdump: add vmcoreinfo ELF note
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (4 preceding siblings ...)
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 5/8] dump: add vmcoreinfo ELF note Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 7/8] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
` (3 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek, Marc-André Lureau
kdump header provides offset and size of the vmcoreinfo ELF note,
append it if available.
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>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
dump.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/dump.c b/dump.c
index d86b601248..b78d58ab0a 100644
--- a/dump.c
+++ b/dump.c
@@ -841,6 +841,16 @@ 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->vmcoreinfo) {
+ uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo;
+
+ get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc);
+ offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_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);
@@ -941,6 +951,16 @@ 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->vmcoreinfo) {
+ uint64_t hsize, name_size, size_vmcoreinfo_desc, offset_vmcoreinfo;
+
+ get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc);
+ offset_vmcoreinfo = offset_note + s->note_size - s->vmcoreinfo_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.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 7/8] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (5 preceding siblings ...)
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 6/8] kdump: " Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 8/8] MAINTAINERS: add Dump maintainers Marc-André Lureau
` (2 subsequent siblings)
9 siblings, 0 replies; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek,
Marc-André Lureau, Michael S. Tsirkin
Add vmcoreinfo ELF note if vmcoreinfo device is ready.
To help the python script, add a static vmcoreinfo_gdb_helper value to
be looked up to get vmcoreinfo device singleton.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Acked-by: Laszlo Ersek <lersek@redhat.com>
---
scripts/dump-guest-memory.py | 47 ++++++++++++++++++++++++++++++++++++++++++++
hw/acpi/vmcoreinfo.c | 3 +++
2 files changed, 50 insertions(+)
diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index f7c6635f15..e0589e5b7c 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")
@@ -120,6 +121,22 @@ 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))
+ # 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 +522,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_realize::vmcoreinfo_gdb_helper"):
+ return
+
+ addr = gdb.parse_and_eval("vmcoreinfo_realize::vmcoreinfo_gdb_helper->"
+ "vmcoreinfo_addr_le")
+ addr = bytes([addr[i] for i in range(4)])
+ addr = struct.unpack("<I", addr)[0]
+
+ mem = self.phys_memory_read(addr, 16)
+ if not mem:
+ return
+ (version, addr, size) = struct.unpack("<IQI", mem)
+ if version != 0:
+ 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 +564,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)
diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
index 0ea41de8d9..78aaa03570 100644
--- a/hw/acpi/vmcoreinfo.c
+++ b/hw/acpi/vmcoreinfo.c
@@ -165,6 +165,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
{
+ static volatile VMCoreInfoState *vmcoreinfo_gdb_helper G_GNUC_UNUSED;
+
if (!bios_linker_loader_can_write_pointer()) {
error_setg(errp, "%s requires DMA write support in fw_cfg, "
"which this machine type does not provide",
@@ -181,6 +183,7 @@ static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
return;
}
+ vmcoreinfo_gdb_helper = VMCOREINFO(dev);
qemu_register_reset(vmcoreinfo_handle_reset, dev);
}
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [Qemu-devel] [PATCH v4 8/8] MAINTAINERS: add Dump maintainers
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (6 preceding siblings ...)
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 7/8] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
@ 2017-07-14 18:20 ` Marc-André Lureau
2017-07-14 19:59 ` [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Michael S. Tsirkin
2017-07-14 23:36 ` no-reply
9 siblings, 0 replies; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 18:20 UTC (permalink / raw)
To: qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, lersek, 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 | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index b9aa878b84..391a28315c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1274,6 +1274,15 @@ 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: stubs/dump.c
+F: include/sysemu/dump.h
+F: include/sysemu/dump-arch.h
+F: scripts/dump-guest-memory.py
+
Error reporting
M: Markus Armbruster <armbru@redhat.com>
S: Supported
--
2.13.1.395.gf7b71de06
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
@ 2017-07-14 19:19 ` Michael S. Tsirkin
0 siblings, 0 replies; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 19:19 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, imammedo, anderson, berrange, ehabkost, lersek, Ben Warren
On Fri, Jul 14, 2017 at 08:20:04PM +0200, Marc-André Lureau wrote:
> This compat property sole function is to prevent the device from being
> instantiated. Instead of requiring an extra compat property, check if
> fw_cfg has DMA enabled.
>
> fw_cfg is a built-in device that is initialized very early by the
> machine init code. We have at least one other device that also
> assumes fw_cfg_find() can be safely used on realize: pvpanic.
>
> This has the additional benefit of handling other cases properly, like:
>
> $ qemu-system-x86_64 -device vmgenid -machine none
> qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support in fw_cfg, which this machine type does not provide
> $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.9 -global fw_cfg.dma_enabled=off
> qemu-system-x86_64: -device vmgenid: vmgenid requires DMA write support in fw_cfg, which this machine type does not provide
> $ qemu-system-x86_64 -device vmgenid -machine pc-i440fx-2.6 -global fw_cfg.dma_enabled=on
> [boots normally]
>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> Reviewed-by: Ben Warren <ben@skyportsystems.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Why tree would you like to merge this through?
If it's mine pls copy me on all patches in the set.
> ---
> include/hw/acpi/bios-linker-loader.h | 2 ++
> include/hw/compat.h | 4 ----
> hw/acpi/bios-linker-loader.c | 10 ++++++++++
> hw/acpi/vmgenid.c | 9 +--------
> 4 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/include/hw/acpi/bios-linker-loader.h b/include/hw/acpi/bios-linker-loader.h
> index efe17b0b9c..a711dbced8 100644
> --- a/include/hw/acpi/bios-linker-loader.h
> +++ b/include/hw/acpi/bios-linker-loader.h
> @@ -7,6 +7,8 @@ typedef struct BIOSLinker {
> GArray *file_list;
> } BIOSLinker;
>
> +bool bios_linker_loader_can_write_pointer(void);
> +
> BIOSLinker *bios_linker_loader_init(void);
>
> void bios_linker_loader_alloc(BIOSLinker *linker,
> diff --git a/include/hw/compat.h b/include/hw/compat.h
> index 08f36004da..f414786604 100644
> --- a/include/hw/compat.h
> +++ b/include/hw/compat.h
> @@ -150,10 +150,6 @@
> .driver = "fw_cfg_io",\
> .property = "dma_enabled",\
> .value = "off",\
> - },{\
> - .driver = "vmgenid",\
> - .property = "x-write-pointer-available",\
> - .value = "off",\
> },
>
> #define HW_COMPAT_2_3 \
> diff --git a/hw/acpi/bios-linker-loader.c b/hw/acpi/bios-linker-loader.c
> index 046183a0f1..d16b8bbcb1 100644
> --- a/hw/acpi/bios-linker-loader.c
> +++ b/hw/acpi/bios-linker-loader.c
> @@ -168,6 +168,16 @@ bios_linker_find_file(const BIOSLinker *linker, const char *name)
> return NULL;
> }
>
> +/*
> + * board code must realize fw_cfg first, as a fixed device, before
> + * another device realize function call bios_linker_loader_can_write_pointer()
> + */
> +bool bios_linker_loader_can_write_pointer(void)
> +{
> + FWCfgState *fw_cfg = fw_cfg_find();
> + return fw_cfg && fw_cfg_dma_enabled(fw_cfg);
> +}
> +
> /*
> * bios_linker_loader_alloc: ask guest to load file into guest memory.
> *
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> index a32b847fe0..ab5da293fd 100644
> --- a/hw/acpi/vmgenid.c
> +++ b/hw/acpi/vmgenid.c
> @@ -205,17 +205,11 @@ static void vmgenid_handle_reset(void *opaque)
> memset(vms->vmgenid_addr_le, 0, ARRAY_SIZE(vms->vmgenid_addr_le));
> }
>
> -static Property vmgenid_properties[] = {
> - DEFINE_PROP_BOOL("x-write-pointer-available", VmGenIdState,
> - write_pointer_available, true),
> - DEFINE_PROP_END_OF_LIST(),
> -};
> -
> static void vmgenid_realize(DeviceState *dev, Error **errp)
> {
> VmGenIdState *vms = VMGENID(dev);
>
> - if (!vms->write_pointer_available) {
> + if (!bios_linker_loader_can_write_pointer()) {
> error_setg(errp, "%s requires DMA write support in fw_cfg, "
> "which this machine type does not provide", VMGENID_DEVICE);
> return;
> @@ -239,7 +233,6 @@ static void vmgenid_device_class_init(ObjectClass *klass, void *data)
> dc->vmsd = &vmstate_vmgenid;
> dc->realize = vmgenid_realize;
> dc->hotpluggable = false;
> - dc->props = vmgenid_properties;
>
> object_class_property_add_str(klass, VMGENID_GUID, NULL,
> vmgenid_set_guid, NULL);
> --
> 2.13.1.395.gf7b71de06
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device Marc-André Lureau
@ 2017-07-14 19:26 ` Michael S. Tsirkin
2017-07-14 20:04 ` Laszlo Ersek
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 19:26 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, imammedo, anderson, berrange, ehabkost, lersek,
Paolo Bonzini, Richard Henderson
On Fri, Jul 14, 2017 at 08:20:05PM +0200, Marc-André Lureau wrote:
> The VM coreinfo (vmcoreinfo) device is an emulated device which
> exposes a 4k memory range to the guest to store various informations
> useful to debug the guest OS. (it is greatly inspired by the VMGENID
> device implementation)
>
> This is an early-boot alternative to the qemu-ga VMDUMP_INFO event
> proposed in "[PATCH 00/21] WIP: dump: add kaslr support".
>
> A proof-of-concept kernel module:
> https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
It worries me that the format of this seems completely undefined
except in the patchset cover letter.
I don't think we should merge this before it is.
> ---
> include/hw/acpi/aml-build.h | 1 +
> include/hw/acpi/vmcoreinfo.h | 37 +++++++
> hw/acpi/aml-build.c | 2 +
> hw/acpi/vmcoreinfo.c | 208 +++++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 14 +++
> default-configs/arm-softmmu.mak | 1 +
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> docs/specs/vmcoreinfo.txt | 138 ++++++++++++++++++++++++
> hw/acpi/Makefile.objs | 1 +
> 10 files changed, 404 insertions(+)
> create mode 100644 include/hw/acpi/vmcoreinfo.h
> create mode 100644 hw/acpi/vmcoreinfo.c
> create mode 100644 docs/specs/vmcoreinfo.txt
>
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 88d0738d76..cf781bcd34 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
> GArray *rsdp;
> GArray *tcpalog;
> GArray *vmgenid;
> + GArray *vmcoreinfo;
> BIOSLinker *linker;
> } AcpiBuildTables;
>
> diff --git a/include/hw/acpi/vmcoreinfo.h b/include/hw/acpi/vmcoreinfo.h
> new file mode 100644
> index 0000000000..6a73bcd1b2
> --- /dev/null
> +++ b/include/hw/acpi/vmcoreinfo.h
> @@ -0,0 +1,37 @@
> +#ifndef ACPI_VMCOREINFO_H
> +#define ACPI_VMCOREINFO_H
> +
> +#include "hw/acpi/bios-linker-loader.h"
> +#include "hw/qdev.h"
> +
> +#define VMCOREINFO_DEVICE "vmcoreinfo"
> +#define VMCOREINFO_FW_CFG_FILE "etc/vmcoreinfo"
> +#define VMCOREINFO_ADDR_FW_CFG_FILE "etc/vmcoreinfo-addr"
> +
> +/* Occupy a page of memory */
> +#define VMCOREINFO_FW_CFG_SIZE 4096
> +
> +/* allow space for OVMF SDT Header Probe Supressor */
> +#define VMCOREINFO_OFFSET sizeof(AcpiTableHeader)
> +
> +#define VMCOREINFO(obj) OBJECT_CHECK(VMCoreInfoState, (obj), VMCOREINFO_DEVICE)
> +
> +typedef struct VMCoreInfoState {
> + DeviceClass parent_obj;
> + uint8_t vmcoreinfo_addr_le[8]; /* Address of memory region */
> + bool write_pointer_available;
> +} VMCoreInfoState;
> +
> +/* returns NULL unless there is exactly one device */
> +static inline Object *find_vmcoreinfo_dev(void)
> +{
> + return object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
> +}
> +
> +void vmcoreinfo_build_acpi(VMCoreInfoState *vis, GArray *table_data,
> + GArray *vmci, BIOSLinker *linker);
> +void vmcoreinfo_add_fw_cfg(VMCoreInfoState *vis, FWCfgState *s, GArray *vmci);
> +bool vmcoreinfo_get(VMCoreInfoState *vis, uint64_t *paddr, uint32_t *size,
> + Error **errp);
> +
> +#endif
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 36a6cc450e..47043ade4a 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -1561,6 +1561,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables)
> tables->table_data = g_array_new(false, true /* clear */, 1);
> tables->tcpalog = g_array_new(false, true /* clear */, 1);
> tables->vmgenid = g_array_new(false, true /* clear */, 1);
> + tables->vmcoreinfo = g_array_new(false, true /* clear */, 1);
> tables->linker = bios_linker_loader_init();
> }
>
> @@ -1571,6 +1572,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
> g_array_free(tables->table_data, true);
> g_array_free(tables->tcpalog, mfre);
> g_array_free(tables->vmgenid, mfre);
> + g_array_free(tables->vmcoreinfo, mfre);
> }
>
> /* Build rsdt table */
> diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
> new file mode 100644
> index 0000000000..0ea41de8d9
> --- /dev/null
> +++ b/hw/acpi/vmcoreinfo.c
> @@ -0,0 +1,208 @@
> +/*
> + * Virtual Machine coreinfo device
> + * (based on Virtual Machine Generation ID Device)
> + *
> + * Copyright (C) 2017 Red Hat, Inc.
> + * Copyright (C) 2017 Skyport Systems.
> + *
> + * Authors: Marc-André Lureau <marcandre.lureau@redhat.com>
> + * Ben Warren <ben@skyportsystems.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 "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/vmcoreinfo.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "sysemu/sysemu.h"
> +#include "qapi/error.h"
> +
> +void vmcoreinfo_build_acpi(VMCoreInfoState *vis, GArray *table_data,
> + GArray *vmci, BIOSLinker *linker)
> +{
> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
> + uint32_t vcia_offset;
> +
> + g_array_set_size(vmci, VMCOREINFO_FW_CFG_SIZE);
> +
> + /* Put this in a separate SSDT table */
> + ssdt = init_aml_allocator();
> +
> + /* Reserve space for header */
> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
> +
> + /* Storage address */
> + vcia_offset = table_data->len +
> + build_append_named_dword(ssdt->buf, "VCIA");
> + scope = aml_scope("\\_SB");
> + dev = aml_device("VMCI");
> + aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVMCI")));
> +
> + /* Simple status method to check that address is linked and non-zero */
> + method = aml_method("_STA", 0, AML_NOTSERIALIZED);
> + addr = aml_local(0);
> + aml_append(method, aml_store(aml_int(0xf), addr));
> + if_ctx = aml_if(aml_equal(aml_name("VCIA"), aml_int(0)));
> + aml_append(if_ctx, aml_store(aml_int(0), addr));
> + aml_append(method, if_ctx);
> + aml_append(method, aml_return(addr));
> + aml_append(dev, method);
> +
> + /* the ADDR method returns two 32-bit words representing the lower and
> + * upper halves of the physical address of the vmcoreinfo area
> + */
> + method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
> +
> + addr = aml_local(0);
> + aml_append(method, aml_store(aml_package(2), addr));
> +
> + aml_append(method, aml_store(aml_add(aml_name("VCIA"),
> + aml_int(VMCOREINFO_OFFSET), NULL),
> + aml_index(addr, aml_int(0))));
> + aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1))));
> + aml_append(method, aml_return(addr));
> +
> + aml_append(dev, method);
> + aml_append(scope, dev);
> + aml_append(ssdt, scope);
> +
> + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
> +
> + /* Allocate guest memory */
> + bios_linker_loader_alloc(linker, VMCOREINFO_FW_CFG_FILE, vmci, 4096,
> + false /* page boundary, high memory */);
> +
> + /* Patch address of vmcoreinfo fw_cfg blob into the ADDR fw_cfg
> + * blob so QEMU can read the info from there. The address is
> + * expected to be < 4GB, but write 64 bits anyway.
> + * The address that is patched in is offset in order to implement
> + * the "OVMF SDT Header probe suppressor"
> + * see docs/specs/vmcoreinfo.txt for more details.
> + */
> + bios_linker_loader_write_pointer(linker,
> + VMCOREINFO_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
> + VMCOREINFO_FW_CFG_FILE, VMCOREINFO_OFFSET);
> +
> + /* Patch address of vmcoreinfo into the AML so OSPM can retrieve
> + * and read it. Note that while we provide storage for 64 bits, only
> + * the least-signficant 32 get patched into AML.
> + */
> + bios_linker_loader_add_pointer(linker,
> + ACPI_BUILD_TABLE_FILE, vcia_offset, sizeof(uint32_t),
> + VMCOREINFO_FW_CFG_FILE, 0);
> +
> + build_header(linker, table_data,
> + (void *)(table_data->data + table_data->len - ssdt->buf->len),
> + "SSDT", ssdt->buf->len, 1, NULL, "VMCOREIN");
> + free_aml_allocator();
> +}
> +
> +void vmcoreinfo_add_fw_cfg(VMCoreInfoState *vis, FWCfgState *s, GArray *vmci)
> +{
> + /* Create a read-only fw_cfg file for vmcoreinfo allocation */
> + /* XXX: linker could learn to allocate without backing fw_cfg? */
> + fw_cfg_add_file(s, VMCOREINFO_FW_CFG_FILE, vmci->data,
> + VMCOREINFO_FW_CFG_SIZE);
> + /* Create a read-write fw_cfg file for Address */
> + fw_cfg_add_file_callback(s, VMCOREINFO_ADDR_FW_CFG_FILE, NULL, NULL,
> + vis->vmcoreinfo_addr_le,
> + ARRAY_SIZE(vis->vmcoreinfo_addr_le), false);
> +}
> +
> +bool vmcoreinfo_get(VMCoreInfoState *vis,
> + uint64_t *paddr, uint32_t *size,
> + Error **errp)
> +{
> + uint32_t vmcoreinfo_addr;
> + uint32_t version;
> +
> + assert(vis);
> + assert(paddr);
> + assert(size);
> +
> + memcpy(&vmcoreinfo_addr, vis->vmcoreinfo_addr_le, sizeof(vmcoreinfo_addr));
> + vmcoreinfo_addr = le32_to_cpu(vmcoreinfo_addr);
> + if (!vmcoreinfo_addr) {
> + error_setg(errp, "BIOS has not yet written the address of %s",
> + VMCOREINFO_DEVICE);
> + return false;
> + }
> +
> + cpu_physical_memory_read(vmcoreinfo_addr, &version, sizeof(version));
> + if (version != 0) {
> + error_setg(errp, "Unknown %s memory version", VMCOREINFO_DEVICE);
> + return false;
> + }
> +
> + cpu_physical_memory_read(vmcoreinfo_addr + 4, paddr, sizeof(*paddr));
> + *paddr = le64_to_cpu(*paddr);
> + cpu_physical_memory_read(vmcoreinfo_addr + 12, size, sizeof(*size));
> + *size = le32_to_cpu(*size);
> +
> + return true;
> +}
> +
> +static const VMStateDescription vmstate_vmcoreinfo = {
> + .name = "vmcoreinfo",
> + .version_id = 1,
> + .minimum_version_id = 1,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT8_ARRAY(vmcoreinfo_addr_le, VMCoreInfoState, sizeof(uint64_t)),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +static void vmcoreinfo_handle_reset(void *opaque)
> +{
> + VMCoreInfoState *vis = VMCOREINFO(opaque);
> +
> + /* Clear the guest-allocated address when the VM resets */
> + memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
> +}
> +
> +static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
> +{
> + if (!bios_linker_loader_can_write_pointer()) {
> + error_setg(errp, "%s requires DMA write support in fw_cfg, "
> + "which this machine type does not provide",
> + VMCOREINFO_DEVICE);
> + return;
> + }
> +
> + /* Given that this function is executing, there is at least one VMCOREINFO
> + * device. Check if there are several.
> + */
> + if (!find_vmcoreinfo_dev()) {
> + error_setg(errp, "at most one %s device is permitted",
> + VMCOREINFO_DEVICE);
> + return;
> + }
> +
> + qemu_register_reset(vmcoreinfo_handle_reset, dev);
> +}
> +
> +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/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 6b7bade183..7ac529680e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -43,6 +43,7 @@
> #include "sysemu/tpm.h"
> #include "hw/acpi/tpm.h"
> #include "hw/acpi/vmgenid.h"
> +#include "hw/acpi/vmcoreinfo.h"
> #include "sysemu/tpm_backend.h"
> #include "hw/timer/mc146818rtc_regs.h"
> #include "sysemu/numa.h"
> @@ -2631,6 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> GArray *tables_blob = tables->table_data;
> AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
> Object *vmgenid_dev;
> + Object *vmcoreinfo_dev;
>
> acpi_get_pm_info(&pm);
> acpi_get_misc_info(&misc);
> @@ -2680,6 +2682,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> vmgenid_build_acpi(VMGENID(vmgenid_dev), tables_blob,
> tables->vmgenid, tables->linker);
> }
> + vmcoreinfo_dev = find_vmcoreinfo_dev();
> + if (vmcoreinfo_dev) {
> + acpi_add_table(table_offsets, tables_blob);
> + vmcoreinfo_build_acpi(VMCOREINFO(vmcoreinfo_dev), tables_blob,
> + tables->vmcoreinfo, tables->linker);
> + }
>
> if (misc.has_hpet) {
> acpi_add_table(table_offsets, tables_blob);
> @@ -2856,6 +2864,7 @@ void acpi_setup(void)
> AcpiBuildTables tables;
> AcpiBuildState *build_state;
> Object *vmgenid_dev;
> + Object *vmcoreinfo_dev;
>
> if (!pcms->fw_cfg) {
> ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> @@ -2897,6 +2906,11 @@ void acpi_setup(void)
> vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
> tables.vmgenid);
> }
> + vmcoreinfo_dev = find_vmcoreinfo_dev();
> + if (vmcoreinfo_dev) {
> + vmcoreinfo_add_fw_cfg(VMCOREINFO(vmcoreinfo_dev), pcms->fw_cfg,
> + tables.vmcoreinfo);
> + }
>
> if (!pcmc->rsdp_in_ram) {
> /*
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 93e995d318..320dd3680d 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -120,6 +120,7 @@ CONFIG_XIO3130=y
> CONFIG_IOH3420=y
> CONFIG_I82801B11=y
> CONFIG_ACPI=y
> +CONFIG_ACPI_VMCOREINFO=y
> CONFIG_SMBIOS=y
> CONFIG_ASPEED_SOC=y
> CONFIG_GPIO_KEY=y
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index d2ab2f6655..df68628895 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> CONFIG_PXB=y
> CONFIG_ACPI_VMGENID=y
> +CONFIG_ACPI_VMCOREINFO=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 9bde2f1c4b..e39ad5a680 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> CONFIG_PXB=y
> CONFIG_ACPI_VMGENID=y
> +CONFIG_ACPI_VMCOREINFO=y
> diff --git a/docs/specs/vmcoreinfo.txt b/docs/specs/vmcoreinfo.txt
> new file mode 100644
> index 0000000000..36d5a39ab1
> --- /dev/null
> +++ b/docs/specs/vmcoreinfo.txt
> @@ -0,0 +1,138 @@
> +VIRTUAL MACHINE COREINFO DEVICE
> +===============================
> +
> +Copyright (C) 2017 Red Hat, Inc.
> +
> +This work is licensed under the terms of the GNU GPL, version 2 or later.
> +See the COPYING file in the top-level directory.
> +
> +===
> +
> +The VM coreinfo (vmcoreinfo) device is an emulated device which
> +exposes a 4k memory range to the guest to store various informations
> +useful to debug the guest OS.
> +
> +QEMU Implementation
> +-------------------
> +
> +The vmcoreinfo device is put in its own ACPI descriptor table, in a
> +Secondary System Description Table, or SSDT.
> +
> +The following is a dump of the contents from a running system:
> +
> +# iasl -p ./SSDT -d /sys/firmware/acpi/tables/SSDT
> +/*
> + * Intel ACPI Component Architecture
> + * AML/ASL+ Disassembler version 20160831-64
> + * Copyright (c) 2000 - 2016 Intel Corporation
> + *
> + * Disassembling to symbolic ASL+ operators
> + *
> + * Disassembly of /sys/firmware/acpi/tables/SSDT, Mon Apr 24 15:59:53 2017
> + *
> + * Original Table Header:
> + * Signature "SSDT"
> + * Length 0x00000086 (134)
> + * Revision 0x01
> + * Checksum 0x5C
> + * OEM ID "BOCHS "
> + * OEM Table ID "VMCOREIN"
> + * OEM Revision 0x00000001 (1)
> + * Compiler ID "BXPC"
> + * Compiler Version 0x00000001 (1)
> + */
> +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "VMCOREIN", 0x00000001)
> +{
> + Name (VCIA, 0x3FFFF000)
> + Scope (\_SB)
> + {
> + Device (VMCI)
> + {
> + Name (_HID, "QEMUVMCI") // _HID: Hardware ID
> + Method (_STA, 0, NotSerialized) // _STA: Status
> + {
> + Local0 = 0x0F
> + If (VCIA == Zero)
> + {
> + Local0 = Zero
> + }
> +
> + Return (Local0)
> + }
> +
> + Method (ADDR, 0, NotSerialized)
> + {
> + Local0 = Package (0x02) {}
> + Local0 [Zero] = (VCIA + 0x24)
> + Local0 [One] = Zero
> + Return (Local0)
> + }
> + }
> + }
> +}
> +
> +
> +Design Details:
> +---------------
> +
> +QEMU must be able to read the contents of the device memory,
> +specifically when starting a memory dump. In order to do this, QEMU
> +must know the address that has been allocated.
> +
> +The mechanism chosen for this memory sharing is writeable fw_cfg blobs.
> +These are data object that are visible to both QEMU and guests, and are
> +addressable as sequential files.
> +
> +More information about fw_cfg can be found in "docs/specs/fw_cfg.txt"
> +
> +Two fw_cfg blobs are used in this case:
> +
> +/etc/vmcoreinfo - used to allocate memory range, read-only to the guest
> +/etc/vmcoreinfo-addr - contains the address of the allocated range
> + - writeable by the guest
> +
> +
> +QEMU sends the following commands to the guest at startup:
> +
> +1. Allocate memory for vmcoreinfo fw_cfg blob.
> +2. Write the address of vmcoreinfo into the SSDT (VCIA ACPI variable as
> + shown above in the iasl dump). Note that this change is not propagated
> + back to QEMU.
> +3. Write the address of vmcoreinfo back to QEMU's copy of vmcoreinfo-addr
> + via the fw_cfg DMA interface.
> +
> +After step 3, QEMU is able to read the contents of vmcoreinfo.
> +
> +The value of VCIA is persisted via the VMState mechanism.
> +
> +
> +Storage Format:
> +---------------
> +
> +The content is expected to use little-endian format.
> +
> +In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
> +the vmcoreinfo blob has 36 bytes of padding:
> +
> ++-----------------------------------+
> +| SSDT with OEM Table ID = VMCOREIN |
> ++-----------------------------------+
> +| ... | TOP OF PAGE
> +| VCIA dword object ----------------|-----> +---------------------------+
> +| ... | | fw-allocated array for |
> +| _STA method referring to VCIA | | "etc/vmcoreinfo" |
> +| ... | +---------------------------+
> +| ADDR method referring to VCIA | | 0: OVMF SDT Header probe |
> +| ... | | suppressor |
> ++-----------------------------------+ | 36: uint32 version field |
> + | 40: info contents |
> + | .... |
> + +---------------------------+
> + END OF PAGE
> +
> +Version 0 content:
> +
> + uint64 paddr:
> + Physical address of the Linux vmcoreinfo ELF note.
> + uint32 size:
> + Size of the vmcoreinfo ELF note.
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 11c35bcb44..9623078f95 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
> +common-obj-$(CONFIG_ACPI_VMCOREINFO) += vmcoreinfo.o
> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>
> common-obj-y += acpi_interface.o
> --
> 2.13.1.395.gf7b71de06
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (7 preceding siblings ...)
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 8/8] MAINTAINERS: add Dump maintainers Marc-André Lureau
@ 2017-07-14 19:59 ` Michael S. Tsirkin
2017-07-14 20:21 ` Laszlo Ersek
2017-07-14 23:36 ` no-reply
9 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 19:59 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: qemu-devel, ehabkost, anderson, imammedo, lersek
On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
> memory range to the guest to store various informations useful to
> debug the guest OS. (it is greatly inspired by the VMGENID device
> implementation). The version field with value 0 is meant to give
> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
> different purposes or OSes. (note: some wanted to see pvpanic somehow
> merged with this device, I have no clear idea how to do that, nor do I
> think this is a good idea since the devices are quite different, used
> at different time for different purposes. And it can be done as a
> future iteration if it is appropriate, feel free to send patches)
First, I think you underestimate the difficulty of maintaining
compatibility.
Second, this seems racy - how do you know when is guest done writing out
the data?
Given you have very little data to export (PA, size - do
you even need size?) - how about just using an ACPI method do it,
instead of exporting a physical addess and storing address there. This
way you can add more methods as you add functionality.
VMGENID has very specific requirements around performance,
and does not care about consistency at all.
This does not apply here.
> 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.
>
> By priority, VMCOREINFO "phys_base" value is the most accurate. If not
> available, qemu will keep the current guessed value.
>
> The series implements the VMCOREINFO note addition in qemu ELF/kdump,
> as well as the python scripts/dump-guest-memory.py.
>
> To test:
>
> Compile and run a guest kernel with CONFIG_RANDOMIZE_BASE=y.
>
> Run qemu with -device vmcoreinfo.
>
> Load the experimental vmcoreinfo module in guest
> https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c.
>
> 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
> $ crash vmlinux dump
>
> 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 (8):
> vmgenid: replace x-write-pointer-available hack
> acpi: add vmcoreinfo device
> stubs: add vmcoreinfo_get() stub
> tests: add simple vmcoreinfo test
> dump: add vmcoreinfo ELF note
> kdump: add vmcoreinfo ELF note
> scripts/dump-guest-memory.py: add vmcoreinfo
> MAINTAINERS: add Dump maintainers
>
> scripts/dump-guest-memory.py | 47 ++++++++
> include/hw/acpi/aml-build.h | 1 +
> include/hw/acpi/bios-linker-loader.h | 2 +
> include/hw/acpi/vmcoreinfo.h | 37 ++++++
> include/hw/compat.h | 4 -
> include/sysemu/dump.h | 2 +
> dump.c | 154 +++++++++++++++++++++++++
> hw/acpi/aml-build.c | 2 +
> hw/acpi/bios-linker-loader.c | 10 ++
> hw/acpi/vmcoreinfo.c | 211 +++++++++++++++++++++++++++++++++++
> hw/acpi/vmgenid.c | 9 +-
> hw/i386/acpi-build.c | 14 +++
> stubs/vmcoreinfo.c | 9 ++
> tests/vmcoreinfo-test.c | 141 +++++++++++++++++++++++
> MAINTAINERS | 9 ++
> default-configs/arm-softmmu.mak | 1 +
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> docs/specs/vmcoreinfo.txt | 138 +++++++++++++++++++++++
> hw/acpi/Makefile.objs | 1 +
> stubs/Makefile.objs | 1 +
> tests/Makefile.include | 2 +
> 22 files changed, 785 insertions(+), 12 deletions(-)
> create mode 100644 include/hw/acpi/vmcoreinfo.h
> create mode 100644 hw/acpi/vmcoreinfo.c
> create mode 100644 stubs/vmcoreinfo.c
> create mode 100644 tests/vmcoreinfo-test.c
> create mode 100644 docs/specs/vmcoreinfo.txt
>
> --
> 2.13.1.395.gf7b71de06
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 19:26 ` Michael S. Tsirkin
@ 2017-07-14 20:04 ` Laszlo Ersek
2017-07-14 20:17 ` Michael S. Tsirkin
0 siblings, 1 reply; 34+ messages in thread
From: Laszlo Ersek @ 2017-07-14 20:04 UTC (permalink / raw)
To: Michael S. Tsirkin, Marc-André Lureau
Cc: qemu-devel, imammedo, anderson, berrange, ehabkost,
Paolo Bonzini, Richard Henderson
On 07/14/17 21:26, Michael S. Tsirkin wrote:
> On Fri, Jul 14, 2017 at 08:20:05PM +0200, Marc-André Lureau wrote:
>> The VM coreinfo (vmcoreinfo) device is an emulated device which
>> exposes a 4k memory range to the guest to store various informations
>> useful to debug the guest OS. (it is greatly inspired by the VMGENID
>> device implementation)
>>
>> This is an early-boot alternative to the qemu-ga VMDUMP_INFO event
>> proposed in "[PATCH 00/21] WIP: dump: add kaslr support".
>>
>> A proof-of-concept kernel module:
>> https://github.com/elmarco/vmgenid-test/blob/master/qemuvmci-test.c
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> It worries me that the format of this seems completely undefined
> except in the patchset cover letter.
> I don't think we should merge this before it is.
I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
That file is the first level contract between the guest firmware
(generally, via the linker/loader), the guest kernel driver
(specifically), and QEMU (also specifically).
The second level contract is the guest kernel's vmcoreinfo ELF note
(which is pointed-to by the first level contract). The layout of that is
specified elsewhere indeed (I don't think it belongs here).
We've taken care not to trust anything coming from the guest kernel.
Can you clarify please?
Thanks
Laszlo
>
>> ---
>> include/hw/acpi/aml-build.h | 1 +
>> include/hw/acpi/vmcoreinfo.h | 37 +++++++
>> hw/acpi/aml-build.c | 2 +
>> hw/acpi/vmcoreinfo.c | 208 +++++++++++++++++++++++++++++++++++++
>> hw/i386/acpi-build.c | 14 +++
>> default-configs/arm-softmmu.mak | 1 +
>> default-configs/i386-softmmu.mak | 1 +
>> default-configs/x86_64-softmmu.mak | 1 +
>> docs/specs/vmcoreinfo.txt | 138 ++++++++++++++++++++++++
>> hw/acpi/Makefile.objs | 1 +
>> 10 files changed, 404 insertions(+)
>> create mode 100644 include/hw/acpi/vmcoreinfo.h
>> create mode 100644 hw/acpi/vmcoreinfo.c
>> create mode 100644 docs/specs/vmcoreinfo.txt
>>
>> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
>> index 88d0738d76..cf781bcd34 100644
>> --- a/include/hw/acpi/aml-build.h
>> +++ b/include/hw/acpi/aml-build.h
>> @@ -211,6 +211,7 @@ struct AcpiBuildTables {
>> GArray *rsdp;
>> GArray *tcpalog;
>> GArray *vmgenid;
>> + GArray *vmcoreinfo;
>> BIOSLinker *linker;
>> } AcpiBuildTables;
>>
>> diff --git a/include/hw/acpi/vmcoreinfo.h b/include/hw/acpi/vmcoreinfo.h
>> new file mode 100644
>> index 0000000000..6a73bcd1b2
>> --- /dev/null
>> +++ b/include/hw/acpi/vmcoreinfo.h
>> @@ -0,0 +1,37 @@
>> +#ifndef ACPI_VMCOREINFO_H
>> +#define ACPI_VMCOREINFO_H
>> +
>> +#include "hw/acpi/bios-linker-loader.h"
>> +#include "hw/qdev.h"
>> +
>> +#define VMCOREINFO_DEVICE "vmcoreinfo"
>> +#define VMCOREINFO_FW_CFG_FILE "etc/vmcoreinfo"
>> +#define VMCOREINFO_ADDR_FW_CFG_FILE "etc/vmcoreinfo-addr"
>> +
>> +/* Occupy a page of memory */
>> +#define VMCOREINFO_FW_CFG_SIZE 4096
>> +
>> +/* allow space for OVMF SDT Header Probe Supressor */
>> +#define VMCOREINFO_OFFSET sizeof(AcpiTableHeader)
>> +
>> +#define VMCOREINFO(obj) OBJECT_CHECK(VMCoreInfoState, (obj), VMCOREINFO_DEVICE)
>> +
>> +typedef struct VMCoreInfoState {
>> + DeviceClass parent_obj;
>> + uint8_t vmcoreinfo_addr_le[8]; /* Address of memory region */
>> + bool write_pointer_available;
>> +} VMCoreInfoState;
>> +
>> +/* returns NULL unless there is exactly one device */
>> +static inline Object *find_vmcoreinfo_dev(void)
>> +{
>> + return object_resolve_path_type("", VMCOREINFO_DEVICE, NULL);
>> +}
>> +
>> +void vmcoreinfo_build_acpi(VMCoreInfoState *vis, GArray *table_data,
>> + GArray *vmci, BIOSLinker *linker);
>> +void vmcoreinfo_add_fw_cfg(VMCoreInfoState *vis, FWCfgState *s, GArray *vmci);
>> +bool vmcoreinfo_get(VMCoreInfoState *vis, uint64_t *paddr, uint32_t *size,
>> + Error **errp);
>> +
>> +#endif
>> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
>> index 36a6cc450e..47043ade4a 100644
>> --- a/hw/acpi/aml-build.c
>> +++ b/hw/acpi/aml-build.c
>> @@ -1561,6 +1561,7 @@ void acpi_build_tables_init(AcpiBuildTables *tables)
>> tables->table_data = g_array_new(false, true /* clear */, 1);
>> tables->tcpalog = g_array_new(false, true /* clear */, 1);
>> tables->vmgenid = g_array_new(false, true /* clear */, 1);
>> + tables->vmcoreinfo = g_array_new(false, true /* clear */, 1);
>> tables->linker = bios_linker_loader_init();
>> }
>>
>> @@ -1571,6 +1572,7 @@ void acpi_build_tables_cleanup(AcpiBuildTables *tables, bool mfre)
>> g_array_free(tables->table_data, true);
>> g_array_free(tables->tcpalog, mfre);
>> g_array_free(tables->vmgenid, mfre);
>> + g_array_free(tables->vmcoreinfo, mfre);
>> }
>>
>> /* Build rsdt table */
>> diff --git a/hw/acpi/vmcoreinfo.c b/hw/acpi/vmcoreinfo.c
>> new file mode 100644
>> index 0000000000..0ea41de8d9
>> --- /dev/null
>> +++ b/hw/acpi/vmcoreinfo.c
>> @@ -0,0 +1,208 @@
>> +/*
>> + * Virtual Machine coreinfo device
>> + * (based on Virtual Machine Generation ID Device)
>> + *
>> + * Copyright (C) 2017 Red Hat, Inc.
>> + * Copyright (C) 2017 Skyport Systems.
>> + *
>> + * Authors: Marc-André Lureau <marcandre.lureau@redhat.com>
>> + * Ben Warren <ben@skyportsystems.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 "hw/acpi/acpi.h"
>> +#include "hw/acpi/aml-build.h"
>> +#include "hw/acpi/vmcoreinfo.h"
>> +#include "hw/nvram/fw_cfg.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qapi/error.h"
>> +
>> +void vmcoreinfo_build_acpi(VMCoreInfoState *vis, GArray *table_data,
>> + GArray *vmci, BIOSLinker *linker)
>> +{
>> + Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
>> + uint32_t vcia_offset;
>> +
>> + g_array_set_size(vmci, VMCOREINFO_FW_CFG_SIZE);
>> +
>> + /* Put this in a separate SSDT table */
>> + ssdt = init_aml_allocator();
>> +
>> + /* Reserve space for header */
>> + acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader));
>> +
>> + /* Storage address */
>> + vcia_offset = table_data->len +
>> + build_append_named_dword(ssdt->buf, "VCIA");
>> + scope = aml_scope("\\_SB");
>> + dev = aml_device("VMCI");
>> + aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVMCI")));
>> +
>> + /* Simple status method to check that address is linked and non-zero */
>> + method = aml_method("_STA", 0, AML_NOTSERIALIZED);
>> + addr = aml_local(0);
>> + aml_append(method, aml_store(aml_int(0xf), addr));
>> + if_ctx = aml_if(aml_equal(aml_name("VCIA"), aml_int(0)));
>> + aml_append(if_ctx, aml_store(aml_int(0), addr));
>> + aml_append(method, if_ctx);
>> + aml_append(method, aml_return(addr));
>> + aml_append(dev, method);
>> +
>> + /* the ADDR method returns two 32-bit words representing the lower and
>> + * upper halves of the physical address of the vmcoreinfo area
>> + */
>> + method = aml_method("ADDR", 0, AML_NOTSERIALIZED);
>> +
>> + addr = aml_local(0);
>> + aml_append(method, aml_store(aml_package(2), addr));
>> +
>> + aml_append(method, aml_store(aml_add(aml_name("VCIA"),
>> + aml_int(VMCOREINFO_OFFSET), NULL),
>> + aml_index(addr, aml_int(0))));
>> + aml_append(method, aml_store(aml_int(0), aml_index(addr, aml_int(1))));
>> + aml_append(method, aml_return(addr));
>> +
>> + aml_append(dev, method);
>> + aml_append(scope, dev);
>> + aml_append(ssdt, scope);
>> +
>> + g_array_append_vals(table_data, ssdt->buf->data, ssdt->buf->len);
>> +
>> + /* Allocate guest memory */
>> + bios_linker_loader_alloc(linker, VMCOREINFO_FW_CFG_FILE, vmci, 4096,
>> + false /* page boundary, high memory */);
>> +
>> + /* Patch address of vmcoreinfo fw_cfg blob into the ADDR fw_cfg
>> + * blob so QEMU can read the info from there. The address is
>> + * expected to be < 4GB, but write 64 bits anyway.
>> + * The address that is patched in is offset in order to implement
>> + * the "OVMF SDT Header probe suppressor"
>> + * see docs/specs/vmcoreinfo.txt for more details.
>> + */
>> + bios_linker_loader_write_pointer(linker,
>> + VMCOREINFO_ADDR_FW_CFG_FILE, 0, sizeof(uint64_t),
>> + VMCOREINFO_FW_CFG_FILE, VMCOREINFO_OFFSET);
>> +
>> + /* Patch address of vmcoreinfo into the AML so OSPM can retrieve
>> + * and read it. Note that while we provide storage for 64 bits, only
>> + * the least-signficant 32 get patched into AML.
>> + */
>> + bios_linker_loader_add_pointer(linker,
>> + ACPI_BUILD_TABLE_FILE, vcia_offset, sizeof(uint32_t),
>> + VMCOREINFO_FW_CFG_FILE, 0);
>> +
>> + build_header(linker, table_data,
>> + (void *)(table_data->data + table_data->len - ssdt->buf->len),
>> + "SSDT", ssdt->buf->len, 1, NULL, "VMCOREIN");
>> + free_aml_allocator();
>> +}
>> +
>> +void vmcoreinfo_add_fw_cfg(VMCoreInfoState *vis, FWCfgState *s, GArray *vmci)
>> +{
>> + /* Create a read-only fw_cfg file for vmcoreinfo allocation */
>> + /* XXX: linker could learn to allocate without backing fw_cfg? */
>> + fw_cfg_add_file(s, VMCOREINFO_FW_CFG_FILE, vmci->data,
>> + VMCOREINFO_FW_CFG_SIZE);
>> + /* Create a read-write fw_cfg file for Address */
>> + fw_cfg_add_file_callback(s, VMCOREINFO_ADDR_FW_CFG_FILE, NULL, NULL,
>> + vis->vmcoreinfo_addr_le,
>> + ARRAY_SIZE(vis->vmcoreinfo_addr_le), false);
>> +}
>> +
>> +bool vmcoreinfo_get(VMCoreInfoState *vis,
>> + uint64_t *paddr, uint32_t *size,
>> + Error **errp)
>> +{
>> + uint32_t vmcoreinfo_addr;
>> + uint32_t version;
>> +
>> + assert(vis);
>> + assert(paddr);
>> + assert(size);
>> +
>> + memcpy(&vmcoreinfo_addr, vis->vmcoreinfo_addr_le, sizeof(vmcoreinfo_addr));
>> + vmcoreinfo_addr = le32_to_cpu(vmcoreinfo_addr);
>> + if (!vmcoreinfo_addr) {
>> + error_setg(errp, "BIOS has not yet written the address of %s",
>> + VMCOREINFO_DEVICE);
>> + return false;
>> + }
>> +
>> + cpu_physical_memory_read(vmcoreinfo_addr, &version, sizeof(version));
>> + if (version != 0) {
>> + error_setg(errp, "Unknown %s memory version", VMCOREINFO_DEVICE);
>> + return false;
>> + }
>> +
>> + cpu_physical_memory_read(vmcoreinfo_addr + 4, paddr, sizeof(*paddr));
>> + *paddr = le64_to_cpu(*paddr);
>> + cpu_physical_memory_read(vmcoreinfo_addr + 12, size, sizeof(*size));
>> + *size = le32_to_cpu(*size);
>> +
>> + return true;
>> +}
>> +
>> +static const VMStateDescription vmstate_vmcoreinfo = {
>> + .name = "vmcoreinfo",
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT8_ARRAY(vmcoreinfo_addr_le, VMCoreInfoState, sizeof(uint64_t)),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> +static void vmcoreinfo_handle_reset(void *opaque)
>> +{
>> + VMCoreInfoState *vis = VMCOREINFO(opaque);
>> +
>> + /* Clear the guest-allocated address when the VM resets */
>> + memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
>> +}
>> +
>> +static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
>> +{
>> + if (!bios_linker_loader_can_write_pointer()) {
>> + error_setg(errp, "%s requires DMA write support in fw_cfg, "
>> + "which this machine type does not provide",
>> + VMCOREINFO_DEVICE);
>> + return;
>> + }
>> +
>> + /* Given that this function is executing, there is at least one VMCOREINFO
>> + * device. Check if there are several.
>> + */
>> + if (!find_vmcoreinfo_dev()) {
>> + error_setg(errp, "at most one %s device is permitted",
>> + VMCOREINFO_DEVICE);
>> + return;
>> + }
>> +
>> + qemu_register_reset(vmcoreinfo_handle_reset, dev);
>> +}
>> +
>> +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/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index 6b7bade183..7ac529680e 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -43,6 +43,7 @@
>> #include "sysemu/tpm.h"
>> #include "hw/acpi/tpm.h"
>> #include "hw/acpi/vmgenid.h"
>> +#include "hw/acpi/vmcoreinfo.h"
>> #include "sysemu/tpm_backend.h"
>> #include "hw/timer/mc146818rtc_regs.h"
>> #include "sysemu/numa.h"
>> @@ -2631,6 +2632,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>> GArray *tables_blob = tables->table_data;
>> AcpiSlicOem slic_oem = { .id = NULL, .table_id = NULL };
>> Object *vmgenid_dev;
>> + Object *vmcoreinfo_dev;
>>
>> acpi_get_pm_info(&pm);
>> acpi_get_misc_info(&misc);
>> @@ -2680,6 +2682,12 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
>> vmgenid_build_acpi(VMGENID(vmgenid_dev), tables_blob,
>> tables->vmgenid, tables->linker);
>> }
>> + vmcoreinfo_dev = find_vmcoreinfo_dev();
>> + if (vmcoreinfo_dev) {
>> + acpi_add_table(table_offsets, tables_blob);
>> + vmcoreinfo_build_acpi(VMCOREINFO(vmcoreinfo_dev), tables_blob,
>> + tables->vmcoreinfo, tables->linker);
>> + }
>>
>> if (misc.has_hpet) {
>> acpi_add_table(table_offsets, tables_blob);
>> @@ -2856,6 +2864,7 @@ void acpi_setup(void)
>> AcpiBuildTables tables;
>> AcpiBuildState *build_state;
>> Object *vmgenid_dev;
>> + Object *vmcoreinfo_dev;
>>
>> if (!pcms->fw_cfg) {
>> ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
>> @@ -2897,6 +2906,11 @@ void acpi_setup(void)
>> vmgenid_add_fw_cfg(VMGENID(vmgenid_dev), pcms->fw_cfg,
>> tables.vmgenid);
>> }
>> + vmcoreinfo_dev = find_vmcoreinfo_dev();
>> + if (vmcoreinfo_dev) {
>> + vmcoreinfo_add_fw_cfg(VMCOREINFO(vmcoreinfo_dev), pcms->fw_cfg,
>> + tables.vmcoreinfo);
>> + }
>>
>> if (!pcmc->rsdp_in_ram) {
>> /*
>> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
>> index 93e995d318..320dd3680d 100644
>> --- a/default-configs/arm-softmmu.mak
>> +++ b/default-configs/arm-softmmu.mak
>> @@ -120,6 +120,7 @@ CONFIG_XIO3130=y
>> CONFIG_IOH3420=y
>> CONFIG_I82801B11=y
>> CONFIG_ACPI=y
>> +CONFIG_ACPI_VMCOREINFO=y
>> CONFIG_SMBIOS=y
>> CONFIG_ASPEED_SOC=y
>> CONFIG_GPIO_KEY=y
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index d2ab2f6655..df68628895 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
>> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
>> CONFIG_PXB=y
>> CONFIG_ACPI_VMGENID=y
>> +CONFIG_ACPI_VMCOREINFO=y
>> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
>> index 9bde2f1c4b..e39ad5a680 100644
>> --- a/default-configs/x86_64-softmmu.mak
>> +++ b/default-configs/x86_64-softmmu.mak
>> @@ -59,3 +59,4 @@ CONFIG_SMBIOS=y
>> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
>> CONFIG_PXB=y
>> CONFIG_ACPI_VMGENID=y
>> +CONFIG_ACPI_VMCOREINFO=y
>> diff --git a/docs/specs/vmcoreinfo.txt b/docs/specs/vmcoreinfo.txt
>> new file mode 100644
>> index 0000000000..36d5a39ab1
>> --- /dev/null
>> +++ b/docs/specs/vmcoreinfo.txt
>> @@ -0,0 +1,138 @@
>> +VIRTUAL MACHINE COREINFO DEVICE
>> +===============================
>> +
>> +Copyright (C) 2017 Red Hat, Inc.
>> +
>> +This work is licensed under the terms of the GNU GPL, version 2 or later.
>> +See the COPYING file in the top-level directory.
>> +
>> +===
>> +
>> +The VM coreinfo (vmcoreinfo) device is an emulated device which
>> +exposes a 4k memory range to the guest to store various informations
>> +useful to debug the guest OS.
>> +
>> +QEMU Implementation
>> +-------------------
>> +
>> +The vmcoreinfo device is put in its own ACPI descriptor table, in a
>> +Secondary System Description Table, or SSDT.
>> +
>> +The following is a dump of the contents from a running system:
>> +
>> +# iasl -p ./SSDT -d /sys/firmware/acpi/tables/SSDT
>> +/*
>> + * Intel ACPI Component Architecture
>> + * AML/ASL+ Disassembler version 20160831-64
>> + * Copyright (c) 2000 - 2016 Intel Corporation
>> + *
>> + * Disassembling to symbolic ASL+ operators
>> + *
>> + * Disassembly of /sys/firmware/acpi/tables/SSDT, Mon Apr 24 15:59:53 2017
>> + *
>> + * Original Table Header:
>> + * Signature "SSDT"
>> + * Length 0x00000086 (134)
>> + * Revision 0x01
>> + * Checksum 0x5C
>> + * OEM ID "BOCHS "
>> + * OEM Table ID "VMCOREIN"
>> + * OEM Revision 0x00000001 (1)
>> + * Compiler ID "BXPC"
>> + * Compiler Version 0x00000001 (1)
>> + */
>> +DefinitionBlock ("", "SSDT", 1, "BOCHS ", "VMCOREIN", 0x00000001)
>> +{
>> + Name (VCIA, 0x3FFFF000)
>> + Scope (\_SB)
>> + {
>> + Device (VMCI)
>> + {
>> + Name (_HID, "QEMUVMCI") // _HID: Hardware ID
>> + Method (_STA, 0, NotSerialized) // _STA: Status
>> + {
>> + Local0 = 0x0F
>> + If (VCIA == Zero)
>> + {
>> + Local0 = Zero
>> + }
>> +
>> + Return (Local0)
>> + }
>> +
>> + Method (ADDR, 0, NotSerialized)
>> + {
>> + Local0 = Package (0x02) {}
>> + Local0 [Zero] = (VCIA + 0x24)
>> + Local0 [One] = Zero
>> + Return (Local0)
>> + }
>> + }
>> + }
>> +}
>> +
>> +
>> +Design Details:
>> +---------------
>> +
>> +QEMU must be able to read the contents of the device memory,
>> +specifically when starting a memory dump. In order to do this, QEMU
>> +must know the address that has been allocated.
>> +
>> +The mechanism chosen for this memory sharing is writeable fw_cfg blobs.
>> +These are data object that are visible to both QEMU and guests, and are
>> +addressable as sequential files.
>> +
>> +More information about fw_cfg can be found in "docs/specs/fw_cfg.txt"
>> +
>> +Two fw_cfg blobs are used in this case:
>> +
>> +/etc/vmcoreinfo - used to allocate memory range, read-only to the guest
>> +/etc/vmcoreinfo-addr - contains the address of the allocated range
>> + - writeable by the guest
>> +
>> +
>> +QEMU sends the following commands to the guest at startup:
>> +
>> +1. Allocate memory for vmcoreinfo fw_cfg blob.
>> +2. Write the address of vmcoreinfo into the SSDT (VCIA ACPI variable as
>> + shown above in the iasl dump). Note that this change is not propagated
>> + back to QEMU.
>> +3. Write the address of vmcoreinfo back to QEMU's copy of vmcoreinfo-addr
>> + via the fw_cfg DMA interface.
>> +
>> +After step 3, QEMU is able to read the contents of vmcoreinfo.
>> +
>> +The value of VCIA is persisted via the VMState mechanism.
>> +
>> +
>> +Storage Format:
>> +---------------
>> +
>> +The content is expected to use little-endian format.
>> +
>> +In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
>> +the vmcoreinfo blob has 36 bytes of padding:
>> +
>> ++-----------------------------------+
>> +| SSDT with OEM Table ID = VMCOREIN |
>> ++-----------------------------------+
>> +| ... | TOP OF PAGE
>> +| VCIA dword object ----------------|-----> +---------------------------+
>> +| ... | | fw-allocated array for |
>> +| _STA method referring to VCIA | | "etc/vmcoreinfo" |
>> +| ... | +---------------------------+
>> +| ADDR method referring to VCIA | | 0: OVMF SDT Header probe |
>> +| ... | | suppressor |
>> ++-----------------------------------+ | 36: uint32 version field |
>> + | 40: info contents |
>> + | .... |
>> + +---------------------------+
>> + END OF PAGE
>> +
>> +Version 0 content:
>> +
>> + uint64 paddr:
>> + Physical address of the Linux vmcoreinfo ELF note.
>> + uint32 size:
>> + Size of the vmcoreinfo ELF note.
>> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
>> index 11c35bcb44..9623078f95 100644
>> --- a/hw/acpi/Makefile.objs
>> +++ b/hw/acpi/Makefile.objs
>> @@ -6,6 +6,7 @@ common-obj-$(CONFIG_ACPI_MEMORY_HOTPLUG) += memory_hotplug.o
>> common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu.o
>> common-obj-$(CONFIG_ACPI_NVDIMM) += nvdimm.o
>> common-obj-$(CONFIG_ACPI_VMGENID) += vmgenid.o
>> +common-obj-$(CONFIG_ACPI_VMCOREINFO) += vmcoreinfo.o
>> common-obj-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>>
>> common-obj-y += acpi_interface.o
>> --
>> 2.13.1.395.gf7b71de06
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 3/8] stubs: add vmcoreinfo_get() stub
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 3/8] stubs: add vmcoreinfo_get() stub Marc-André Lureau
@ 2017-07-14 20:09 ` Laszlo Ersek
0 siblings, 0 replies; 34+ messages in thread
From: Laszlo Ersek @ 2017-07-14 20:09 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, Paolo Bonzini
On 07/14/17 20:20, Marc-André Lureau wrote:
> Common dump code may call vmcoreinfo_get() which is not available on
> all targets.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> stubs/vmcoreinfo.c | 9 +++++++++
> stubs/Makefile.objs | 1 +
> 2 files changed, 10 insertions(+)
> create mode 100644 stubs/vmcoreinfo.c
>
> diff --git a/stubs/vmcoreinfo.c b/stubs/vmcoreinfo.c
> new file mode 100644
> index 0000000000..a994153832
> --- /dev/null
> +++ b/stubs/vmcoreinfo.c
> @@ -0,0 +1,9 @@
> +#include "qemu/osdep.h"
> +
> +#include "hw/acpi/vmcoreinfo.h"
> +
> +bool vmcoreinfo_get(VMCoreInfoState *vis, uint64_t *paddr, uint32_t *size,
> + Error **errp)
> +{
> + return false;
> +}
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index f5b47bfd74..3bedc73252 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -36,6 +36,7 @@ stub-obj-y += qmp_pc_dimm_device_list.o
> stub-obj-y += target-monitor-defs.o
> stub-obj-y += target-get-monitor-def.o
> stub-obj-y += pc_madt_cpu_entry.o
> +stub-obj-y += vmcoreinfo.o
> stub-obj-y += vmgenid.o
> stub-obj-y += xen-common.o
> stub-obj-y += xen-hvm.o
>
Looks like a good idea -- but now that you are adding this patch, I
think we should actually report an error like "unsupported" via errp:
error_setg(errp, QERR_UNSUPPORTED);
In patch #5 you report the error (as a warning) with warn_report_err(),
and I think that's going to crash there -- err is pre-inited to NULL,
the above impl. doesn't change it, and warn_report_err() doesn't expect
NULL.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 4/8] tests: add simple vmcoreinfo test
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 4/8] tests: add simple vmcoreinfo test Marc-André Lureau
@ 2017-07-14 20:10 ` Laszlo Ersek
0 siblings, 0 replies; 34+ messages in thread
From: Laszlo Ersek @ 2017-07-14 20:10 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, Michael S. Tsirkin
On 07/14/17 20:20, Marc-André Lureau wrote:
> This test is based off vmgenid test from Ben Warren
> <ben@skyportsystems.com>. It simply checks the vmcoreinfo ACPI device
> is present and that the memory region associated can be read.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> tests/vmcoreinfo-test.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++
> tests/Makefile.include | 2 +
> 2 files changed, 143 insertions(+)
> create mode 100644 tests/vmcoreinfo-test.c
>
> diff --git a/tests/vmcoreinfo-test.c b/tests/vmcoreinfo-test.c
> new file mode 100644
> index 0000000000..a54f3b5c6a
> --- /dev/null
> +++ b/tests/vmcoreinfo-test.c
> @@ -0,0 +1,141 @@
> +/*
> + * QTest testcase for VM coreinfo device
> + *
> + * Copyright (c) 2017 Red Hat, Inc.
> + * Copyright (c) 2017 Skyport Systems
> + *
> + * 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 <glib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "qemu/osdep.h"
> +#include "qemu/bitmap.h"
> +#include "qemu/uuid.h"
> +#include "hw/acpi/acpi-defs.h"
> +#include "boot-sector.h"
> +#include "acpi-utils.h"
> +#include "libqtest.h"
> +
> +#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
> +
> +typedef struct {
> + AcpiTableHeader header;
> + gchar name_op;
> + gchar vcia[4];
> + gchar val_op;
> + uint32_t vcia_val;
> +} QEMU_PACKED VmciTable;
> +
> +static uint32_t acpi_find_vcia(void)
> +{
> + uint32_t rsdp_offset;
> + AcpiRsdpDescriptor rsdp_table;
> + uint32_t rsdt;
> + AcpiRsdtDescriptorRev1 rsdt_table;
> + int tables_nr;
> + uint32_t *tables;
> + AcpiTableHeader ssdt_table;
> + VmciTable vmci_table;
> + int i;
> +
> + /* Wait for guest firmware to finish and start the payload. */
> + boot_sector_test();
> +
> + /* Tables should be initialized now. */
> + rsdp_offset = acpi_find_rsdp_address();
> +
> + g_assert_cmphex(rsdp_offset, <, RSDP_ADDR_INVALID);
> +
> + acpi_parse_rsdp_table(rsdp_offset, &rsdp_table);
> +
> + rsdt = rsdp_table.rsdt_physical_address;
> + /* read the header */
> + ACPI_READ_TABLE_HEADER(&rsdt_table, rsdt);
> + ACPI_ASSERT_CMP(rsdt_table.signature, "RSDT");
> +
> + /* compute the table entries in rsdt */
> + tables_nr = (rsdt_table.length - sizeof(AcpiRsdtDescriptorRev1)) /
> + sizeof(uint32_t);
> + g_assert_cmpint(tables_nr, >, 0);
> +
> + /* get the addresses of the tables pointed by rsdt */
> + tables = g_new0(uint32_t, tables_nr);
> + ACPI_READ_ARRAY_PTR(tables, tables_nr, rsdt);
> +
> + for (i = 0; i < tables_nr; i++) {
> + ACPI_READ_TABLE_HEADER(&ssdt_table, tables[i]);
> + if (!strncmp((char *)ssdt_table.oem_table_id, "VMCOREIN", 8)) {
> + /* the first entry in the table should be VCIA
> + * That's all we need
> + */
> + ACPI_READ_FIELD(vmci_table.name_op, tables[i]);
> + g_assert(vmci_table.name_op == 0x08); /* name */
> + ACPI_READ_ARRAY(vmci_table.vcia, tables[i]);
> + g_assert(memcmp(vmci_table.vcia, "VCIA", 4) == 0);
> + ACPI_READ_FIELD(vmci_table.val_op, tables[i]);
> + g_assert(vmci_table.val_op == 0x0C); /* dword */
> + ACPI_READ_FIELD(vmci_table.vcia_val, tables[i]);
> + /* The GUID is written at a fixed offset into the fw_cfg file
> + * in order to implement the "OVMF SDT Header probe suppressor"
> + * see docs/specs/vmgenid.txt for more details
> + */
> + g_free(tables);
> + return vmci_table.vcia_val;
> + }
> + }
> + g_free(tables);
> + return 0;
> +}
> +
> +static char disk[] = "tests/vmci-test-disk-XXXXXX";
> +
> +static char *cmd_strdup(void)
> +{
> + return g_strdup_printf("-machine accel=tcg "
> + "-device vmcoreinfo,id=testvmvci "
> + "-drive id=hd0,if=none,file=%s,format=raw "
> + "-device ide-hd,drive=hd0 ",
> + disk);
> +}
> +
> +static void vmcoreinfo_test(void)
> +{
> + gchar *cmd;
> + uint32_t vmci_addr;
> + int i;
> +
> + cmd = cmd_strdup();
> + qtest_start(cmd);
> +
> + vmci_addr = acpi_find_vcia();
> + g_assert(vmci_addr);
> +
> + for (i = 0; i < 4096; i++) {
> + /* check the memory region can be read */
> + readb(vmci_addr + i);
> + }
> +
> + qtest_quit(global_qtest);
> + g_free(cmd);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + int ret;
> +
> + ret = boot_sector_init(disk);
> + if (ret) {
> + return ret;
> + }
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + qtest_add_func("/vmcoreinfo/test", vmcoreinfo_test);
> + ret = g_test_run();
> + boot_sector_cleanup(disk);
> +
> + return ret;
> +}
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index cfbb689e0e..dd515f2159 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -251,6 +251,7 @@ gcov-files-i386-y += hw/usb/hcd-xhci.c
> check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
> check-qtest-i386-y += tests/q35-test$(EXESUF)
> check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
> +check-qtest-i386-y += tests/vmcoreinfo-test$(EXESUF)
> gcov-files-i386-y += hw/pci-host/q35.c
> check-qtest-i386-$(CONFIG_VHOST_NET_TEST_i386) += tests/vhost-user-test$(EXESUF)
> ifeq ($(CONFIG_VHOST_NET_TEST_i386),)
> @@ -762,6 +763,7 @@ tests/test-arm-mptimer$(EXESUF): tests/test-arm-mptimer.o
> tests/test-qapi-util$(EXESUF): tests/test-qapi-util.o $(test-util-obj-y)
> tests/numa-test$(EXESUF): tests/numa-test.o
> tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o tests/boot-sector.o tests/acpi-utils.o
> +tests/vmcoreinfo-test$(EXESUF): tests/vmcoreinfo-test.o tests/boot-sector.o tests/acpi-utils.o
>
> tests/migration/stress$(EXESUF): tests/migration/stress.o
> $(call quiet-command, $(LINKPROG) -static -O3 $(PTHREAD_LIB) -o $@ $< ,"LINK","$(TARGET_DIR)$@")
>
I think I'll let Michael review this, given that he just converted
vmgenid to the boot sector trick.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 20:04 ` Laszlo Ersek
@ 2017-07-14 20:17 ` Michael S. Tsirkin
2017-07-14 22:12 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 20:17 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marc-André Lureau, qemu-devel, imammedo, anderson, berrange,
ehabkost, Paolo Bonzini, Richard Henderson
On Fri, Jul 14, 2017 at 10:04:31PM +0200, Laszlo Ersek wrote:
> > It worries me that the format of this seems completely undefined
> > except in the patchset cover letter.
> > I don't think we should merge this before it is.
>
> I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
> That file is the first level contract between the guest firmware
> (generally, via the linker/loader), the guest kernel driver
> (specifically), and QEMU (also specifically).
>
> The second level contract is the guest kernel's vmcoreinfo ELF note
> (which is pointed-to by the first level contract). The layout of that is
> specified elsewhere indeed (I don't think it belongs here).
>
> We've taken care not to trust anything coming from the guest kernel.
>
> Can you clarify please?
>
> Thanks
> Laszlo
All there is is this:
+Version 0 content:
+
+ uint64 paddr:
+ Physical address of the Linux vmcoreinfo ELF note.
+ uint32 size:
+ Size of the vmcoreinfo ELF note.
It isn't defined here what is the Linux vmcoreinfo ELF note.
You want a bit more info so people trying to use it know where to start
and what they can get out of it.
--
MST
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 19:59 ` [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Michael S. Tsirkin
@ 2017-07-14 20:21 ` Laszlo Ersek
2017-07-14 22:23 ` Michael S. Tsirkin
0 siblings, 1 reply; 34+ messages in thread
From: Laszlo Ersek @ 2017-07-14 20:21 UTC (permalink / raw)
To: Michael S. Tsirkin, Marc-André Lureau
Cc: qemu-devel, ehabkost, anderson, imammedo
On 07/14/17 21:59, Michael S. Tsirkin wrote:
> On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
>> memory range to the guest to store various informations useful to
>> debug the guest OS. (it is greatly inspired by the VMGENID device
>> implementation). The version field with value 0 is meant to give
>> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
>> different purposes or OSes. (note: some wanted to see pvpanic somehow
>> merged with this device, I have no clear idea how to do that, nor do I
>> think this is a good idea since the devices are quite different, used
>> at different time for different purposes. And it can be done as a
>> future iteration if it is appropriate, feel free to send patches)
>
> First, I think you underestimate the difficulty of maintaining
> compatibility.
>
> Second, this seems racy - how do you know when is guest done writing out
> the data?
What data exactly?
The guest kernel module points the fields in the "vmcoreinfo page" to
the then-existent vmcoreinfo ELF note. At that point, the ELF note is
complete.
If we catch the guest with a dump request while the kernel module is
setting up the fields (i.e., the fields are not consistent), then we'll
catch that in our sanity checks, and the note won't be extracted. This
is no different from the case when you simply dump the guest RAM before
the module got invoked.
> Given you have very little data to export (PA, size - do
> you even need size?)
Yes, it tells us in advance how much memory to allocate before we copy
out the vmcoreinfo ELF note (and we enforce a sanity limit on the size).
> - how about just using an ACPI method do it,
Do what exactly?
> instead of exporting a physical addess and storing address there. This
> way you can add more methods as you add functionality.
I'm not saying this is a bad idea (especially because I don't fully
understand your point), but I will say that I'm quite sad that you are
sending Marc-André back to the drawing board after he posted v4 -- also
invalidating my review efforts. :/
Laszlo
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 20:17 ` Michael S. Tsirkin
@ 2017-07-14 22:12 ` Marc-André Lureau
2017-07-14 23:09 ` Michael S. Tsirkin
0 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 22:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Dave Anderson, Igor Mammedov, Richard Henderson
Hi
On Fri, Jul 14, 2017 at 10:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jul 14, 2017 at 10:04:31PM +0200, Laszlo Ersek wrote:
>> > It worries me that the format of this seems completely undefined
>> > except in the patchset cover letter.
>> > I don't think we should merge this before it is.
>>
>> I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
>> That file is the first level contract between the guest firmware
>> (generally, via the linker/loader), the guest kernel driver
>> (specifically), and QEMU (also specifically).
>>
>> The second level contract is the guest kernel's vmcoreinfo ELF note
>> (which is pointed-to by the first level contract). The layout of that is
>> specified elsewhere indeed (I don't think it belongs here).
>>
>> We've taken care not to trust anything coming from the guest kernel.
>>
>> Can you clarify please?
>>
>> Thanks
>> Laszlo
>
> All there is is this:
>
> +Version 0 content:
> +
> + uint64 paddr:
> + Physical address of the Linux vmcoreinfo ELF note.
> + uint32 size:
> + Size of the vmcoreinfo ELF note.
>
> It isn't defined here what is the Linux vmcoreinfo ELF note.
> You want a bit more info so people trying to use it know where to start
> and what they can get out of it.
QEMU is not responsible for the content of the ELF note. All there is afaik is:
https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-kernel-vmcoreinfo
The rest you need to dig in the kernel code and git history I think.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 20:21 ` Laszlo Ersek
@ 2017-07-14 22:23 ` Michael S. Tsirkin
2017-07-14 22:31 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 22:23 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Marc-André Lureau, qemu-devel, ehabkost, anderson, imammedo
On Fri, Jul 14, 2017 at 10:21:43PM +0200, Laszlo Ersek wrote:
> On 07/14/17 21:59, Michael S. Tsirkin wrote:
> > On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
> >> memory range to the guest to store various informations useful to
> >> debug the guest OS. (it is greatly inspired by the VMGENID device
> >> implementation). The version field with value 0 is meant to give
> >> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
> >> different purposes or OSes. (note: some wanted to see pvpanic somehow
> >> merged with this device, I have no clear idea how to do that, nor do I
> >> think this is a good idea since the devices are quite different, used
> >> at different time for different purposes. And it can be done as a
> >> future iteration if it is appropriate, feel free to send patches)
> >
> > First, I think you underestimate the difficulty of maintaining
> > compatibility.
> >
> > Second, this seems racy - how do you know when is guest done writing out
> > the data?
>
> What data exactly?
>
> The guest kernel module points the fields in the "vmcoreinfo page" to
> the then-existent vmcoreinfo ELF note. At that point, the ELF note is
> complete.
When does this happen?
> If we catch the guest with a dump request while the kernel module is
> setting up the fields (i.e., the fields are not consistent), then we'll
> catch that in our sanity checks, and the note won't be extracted.
Are there assumptions about e.g. in which order pa and size
are written out then? Atomicity of these writes?
> This
> is no different from the case when you simply dump the guest RAM before
> the module got invoked.
>
> > Given you have very little data to export (PA, size - do
> > you even need size?)
>
> Yes, it tells us in advance how much memory to allocate before we copy
> out the vmcoreinfo ELF note (and we enforce a sanity limit on the size).
>
> > - how about just using an ACPI method do it,
>
> Do what exactly?
Pass address + size to host - that's what the interface is doing,
isn't it?
> > instead of exporting a physical addess and storing address there. This
> > way you can add more methods as you add functionality.
>
> I'm not saying this is a bad idea (especially because I don't fully
> understand your point), but I will say that I'm quite sad that you are
> sending Marc-André back to the drawing board after he posted v4 -- also
> invalidating my review efforts. :/
>
> Laszlo
You are right, I should have looked at this sooner. Early RFC
suggested writing into fw cfg directly. I couldn't find any
discussion around this - why was this abandoned?
--
MST
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 22:23 ` Michael S. Tsirkin
@ 2017-07-14 22:31 ` Marc-André Lureau
2017-07-14 23:29 ` Michael S. Tsirkin
0 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 22:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Laszlo Ersek, Igor Mammedov, Dave Anderson, QEMU, Eduardo Habkost
Hi
On Sat, Jul 15, 2017 at 12:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Fri, Jul 14, 2017 at 10:21:43PM +0200, Laszlo Ersek wrote:
>> On 07/14/17 21:59, Michael S. Tsirkin wrote:
>> > On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
>> >> memory range to the guest to store various informations useful to
>> >> debug the guest OS. (it is greatly inspired by the VMGENID device
>> >> implementation). The version field with value 0 is meant to give
>> >> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
>> >> different purposes or OSes. (note: some wanted to see pvpanic somehow
>> >> merged with this device, I have no clear idea how to do that, nor do I
>> >> think this is a good idea since the devices are quite different, used
>> >> at different time for different purposes. And it can be done as a
>> >> future iteration if it is appropriate, feel free to send patches)
>> >
>> > First, I think you underestimate the difficulty of maintaining
>> > compatibility.
>> >
>> > Second, this seems racy - how do you know when is guest done writing out
>> > the data?
>>
>> What data exactly?
>>
>> The guest kernel module points the fields in the "vmcoreinfo page" to
>> the then-existent vmcoreinfo ELF note. At that point, the ELF note is
>> complete.
>
> When does this happen?
Very early boot afaik. But the exact details on when to expose it is
left to the kernel side. For now, it's a test module I load manually.
>
>> If we catch the guest with a dump request while the kernel module is
>> setting up the fields (i.e., the fields are not consistent), then we'll
>> catch that in our sanity checks, and the note won't be extracted.
>
> Are there assumptions about e.g. in which order pa and size
> are written out then? Atomicity of these writes?
I expect it to be atomic, but as Laszlo said, the code should be quite
careful when trying to read the data.
>
>> This
>> is no different from the case when you simply dump the guest RAM before
>> the module got invoked.
>>
>> > Given you have very little data to export (PA, size - do
>> > you even need size?)
>>
>> Yes, it tells us in advance how much memory to allocate before we copy
>> out the vmcoreinfo ELF note (and we enforce a sanity limit on the size).
>>
>> > - how about just using an ACPI method do it,
>>
>> Do what exactly?
>
> Pass address + size to host - that's what the interface is doing,
> isn't it?
>
The memory region is meant to be usable for other OS, or to export
more details in the future. I think if we add a method, it would be to
tell qemu that the memory has been written, but it may still be
corrupted at the time we read it. So I am not sure it will really help
>> > instead of exporting a physical addess and storing address there. This
>> > way you can add more methods as you add functionality.
>>
>> I'm not saying this is a bad idea (especially because I don't fully
>> understand your point), but I will say that I'm quite sad that you are
>> sending Marc-André back to the drawing board after he posted v4 -- also
>> invalidating my review efforts. :/
>>
>> Laszlo
>
> You are right, I should have looked at this sooner. Early RFC
> suggested writing into fw cfg directly. I couldn't find any
> discussion around this - why was this abandoned?
Violation (or rather abuse) of layers iirc
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 22:12 ` Marc-André Lureau
@ 2017-07-14 23:09 ` Michael S. Tsirkin
2017-07-14 23:30 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 23:09 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Dave Anderson, Igor Mammedov, Richard Henderson
On Sat, Jul 15, 2017 at 12:12:06AM +0200, Marc-André Lureau wrote:
> Hi
>
> On Fri, Jul 14, 2017 at 10:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Jul 14, 2017 at 10:04:31PM +0200, Laszlo Ersek wrote:
> >> > It worries me that the format of this seems completely undefined
> >> > except in the patchset cover letter.
> >> > I don't think we should merge this before it is.
> >>
> >> I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
> >> That file is the first level contract between the guest firmware
> >> (generally, via the linker/loader), the guest kernel driver
> >> (specifically), and QEMU (also specifically).
> >>
> >> The second level contract is the guest kernel's vmcoreinfo ELF note
> >> (which is pointed-to by the first level contract). The layout of that is
> >> specified elsewhere indeed (I don't think it belongs here).
> >>
> >> We've taken care not to trust anything coming from the guest kernel.
> >>
> >> Can you clarify please?
> >>
> >> Thanks
> >> Laszlo
> >
> > All there is is this:
> >
> > +Version 0 content:
> > +
> > + uint64 paddr:
> > + Physical address of the Linux vmcoreinfo ELF note.
> > + uint32 size:
> > + Size of the vmcoreinfo ELF note.
> >
> > It isn't defined here what is the Linux vmcoreinfo ELF note.
> > You want a bit more info so people trying to use it know where to start
> > and what they can get out of it.
>
> QEMU is not responsible for the content of the ELF note. All there is afaik is:
>
> https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-kernel-vmcoreinfo
>
> The rest you need to dig in the kernel code and git history I think.
We need to do a good enough job to at least make it possible for people
to make use of it without reading your python code.
Documentation/kdump/kdump.txt has a bit more information.
All of the necessary information about the system kernel's core image is
encoded in the ELF format, and stored in a reserved area of memory
before a crash. The physical address of the start of the ELF header is
passed to the dump-capture kernel through the elfcorehdr= boot
parameter. Optionally the size of the ELF header can also be passed
when using the elfcorehdr=[size[KMG]@]offset[KMG] syntax.
With the dump-capture kernel, you can access the memory image through
/proc/vmcore. This exports the dump as an ELF-format file that you can
write out using file copy commands such as cp or scp. Further, you can
use analysis tools such as the GNU Debugger (GDB) and the Crash tool to
debug the dump file. This method ensures that the dump pages are correctly
ordered.
I know where to find it but most people likely won't be able to.
BTW why does it pass ELF header size? Any idea?
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 22:31 ` Marc-André Lureau
@ 2017-07-14 23:29 ` Michael S. Tsirkin
2017-07-18 13:29 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 23:29 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laszlo Ersek, Igor Mammedov, Dave Anderson, QEMU, Eduardo Habkost
On Sat, Jul 15, 2017 at 12:31:36AM +0200, Marc-André Lureau wrote:
> Hi
>
> On Sat, Jul 15, 2017 at 12:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Fri, Jul 14, 2017 at 10:21:43PM +0200, Laszlo Ersek wrote:
> >> On 07/14/17 21:59, Michael S. Tsirkin wrote:
> >> > On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
> >> >> memory range to the guest to store various informations useful to
> >> >> debug the guest OS. (it is greatly inspired by the VMGENID device
> >> >> implementation). The version field with value 0 is meant to give
> >> >> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
> >> >> different purposes or OSes. (note: some wanted to see pvpanic somehow
> >> >> merged with this device, I have no clear idea how to do that, nor do I
> >> >> think this is a good idea since the devices are quite different, used
> >> >> at different time for different purposes. And it can be done as a
> >> >> future iteration if it is appropriate, feel free to send patches)
> >> >
> >> > First, I think you underestimate the difficulty of maintaining
> >> > compatibility.
> >> >
> >> > Second, this seems racy - how do you know when is guest done writing out
> >> > the data?
> >>
> >> What data exactly?
> >>
> >> The guest kernel module points the fields in the "vmcoreinfo page" to
> >> the then-existent vmcoreinfo ELF note. At that point, the ELF note is
> >> complete.
> >
> > When does this happen?
>
> Very early boot afaik. But the exact details on when to expose it is
> left to the kernel side. For now, it's a test module I load manually.
>
> >
> >> If we catch the guest with a dump request while the kernel module is
> >> setting up the fields (i.e., the fields are not consistent), then we'll
> >> catch that in our sanity checks, and the note won't be extracted.
> >
> > Are there assumptions about e.g. in which order pa and size
> > are written out then? Atomicity of these writes?
>
> I expect it to be atomic, but as Laszlo said, the code should be quite
> careful when trying to read the data.
Same when writing it out. Worth documenting too.
> >
> >> This
> >> is no different from the case when you simply dump the guest RAM before
> >> the module got invoked.
> >>
> >> > Given you have very little data to export (PA, size - do
> >> > you even need size?)
> >>
> >> Yes, it tells us in advance how much memory to allocate before we copy
> >> out the vmcoreinfo ELF note (and we enforce a sanity limit on the size).
> >>
> >> > - how about just using an ACPI method do it,
> >>
> >> Do what exactly?
> >
> > Pass address + size to host - that's what the interface is doing,
> > isn't it?
> >
>
>
> The memory region is meant to be usable for other OS, or to export
> more details in the future.
That's the issue. You will never be able to increment version
just to add more data because that will break old hypervisors.
> I think if we add a method, it would be to
> tell qemu that the memory has been written, but it may still be
> corrupted at the time we read it. So I am not sure it will really help
So don't. Just pass PA and size to method as arguments and let it figure
out how to pass it to QEMU. To extend, you will simply add another
method - which one is present tells you what does hypervisor
support, which one is called tells you what does guest support.
What to do there internally? I don't mind it if it sticks this data
in reserved memory like you did here. And then you won't need to
reserve a full 4K, just a couple of bytes as it will be safe to extend.
>
> >> > instead of exporting a physical addess and storing address there. This
> >> > way you can add more methods as you add functionality.
> >>
> >> I'm not saying this is a bad idea (especially because I don't fully
> >> understand your point), but I will say that I'm quite sad that you are
> >> sending Marc-André back to the drawing board after he posted v4 -- also
> >> invalidating my review efforts. :/
> >>
> >> Laszlo
> >
> > You are right, I should have looked at this sooner. Early RFC
> > suggested writing into fw cfg directly. I couldn't find any
> > discussion around this - why was this abandoned?
>
> Violation (or rather abuse) of layers iirc
Hmm. I tried to find discussion about that and failed. It is available
through QEMU0002 in ACPI - would it be OK if guests went through that?
I do not mind the extra indirection so much, but I don't think
host/guest interface compatibility issues are fully thought through.
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 23:09 ` Michael S. Tsirkin
@ 2017-07-14 23:30 ` Marc-André Lureau
2017-07-14 23:40 ` Michael S. Tsirkin
0 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 23:30 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Dave Anderson, Igor Mammedov, Richard Henderson
Hi
On Sat, Jul 15, 2017 at 1:09 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sat, Jul 15, 2017 at 12:12:06AM +0200, Marc-André Lureau wrote:
> > Hi
> >
> > On Fri, Jul 14, 2017 at 10:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Fri, Jul 14, 2017 at 10:04:31PM +0200, Laszlo Ersek wrote:
> > >> > It worries me that the format of this seems completely undefined
> > >> > except in the patchset cover letter.
> > >> > I don't think we should merge this before it is.
> > >>
> > >> I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
> > >> That file is the first level contract between the guest firmware
> > >> (generally, via the linker/loader), the guest kernel driver
> > >> (specifically), and QEMU (also specifically).
> > >>
> > >> The second level contract is the guest kernel's vmcoreinfo ELF note
> > >> (which is pointed-to by the first level contract). The layout of that is
> > >> specified elsewhere indeed (I don't think it belongs here).
> > >>
> > >> We've taken care not to trust anything coming from the guest kernel.
> > >>
> > >> Can you clarify please?
> > >>
> > >> Thanks
> > >> Laszlo
> > >
> > > All there is is this:
> > >
> > > +Version 0 content:
> > > +
> > > + uint64 paddr:
> > > + Physical address of the Linux vmcoreinfo ELF note.
> > > + uint32 size:
> > > + Size of the vmcoreinfo ELF note.
> > >
> > > It isn't defined here what is the Linux vmcoreinfo ELF note.
> > > You want a bit more info so people trying to use it know where to start
> > > and what they can get out of it.
> >
> > QEMU is not responsible for the content of the ELF note. All there is afaik is:
> >
> > https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-kernel-vmcoreinfo
> >
> > The rest you need to dig in the kernel code and git history I think.
>
> We need to do a good enough job to at least make it possible for people
> to make use of it without reading your python code.
>
> Documentation/kdump/kdump.txt has a bit more information.
>
> All of the necessary information about the system kernel's core image is
> encoded in the ELF format, and stored in a reserved area of memory
> before a crash. The physical address of the start of the ELF header is
> passed to the dump-capture kernel through the elfcorehdr= boot
> parameter. Optionally the size of the ELF header can also be passed
> when using the elfcorehdr=[size[KMG]@]offset[KMG] syntax.
>
>
> With the dump-capture kernel, you can access the memory image through
> /proc/vmcore. This exports the dump as an ELF-format file that you can
> write out using file copy commands such as cp or scp. Further, you can
> use analysis tools such as the GNU Debugger (GDB) and the Crash tool to
> debug the dump file. This method ensures that the dump pages are correctly
> ordered.
>
> I know where to find it but most people likely won't be able to.
>
What do you learn from that regarding vmcoreinfo? That is it being
used by kdump? and that you can use gdb/crash, that we already use to
analyse our dumps.
>
> BTW why does it pass ELF header size? Any idea?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d3bf37955d46718ee1a7f1fc69f953d2328ba7c2
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
` (8 preceding siblings ...)
2017-07-14 19:59 ` [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Michael S. Tsirkin
@ 2017-07-14 23:36 ` no-reply
9 siblings, 0 replies; 34+ messages in thread
From: no-reply @ 2017-07-14 23:36 UTC (permalink / raw)
To: marcandre.lureau; +Cc: famz, qemu-devel, ehabkost, anderson, imammedo, lersek
Hi,
This series seems to have some coding style problems. See output below for
more information:
Message-id: 20170714182012.4595-1-marcandre.lureau@redhat.com
Subject: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
04e62b8 MAINTAINERS: add Dump maintainers
947599d scripts/dump-guest-memory.py: add vmcoreinfo
7f07c2f kdump: add vmcoreinfo ELF note
dffc775 dump: add vmcoreinfo ELF note
5f6bf26 tests: add simple vmcoreinfo test
6dd3154 stubs: add vmcoreinfo_get() stub
232c73c acpi: add vmcoreinfo device
e13fbf6 vmgenid: replace x-write-pointer-available hack
=== OUTPUT BEGIN ===
Checking PATCH 1/8: vmgenid: replace x-write-pointer-available hack...
Checking PATCH 2/8: acpi: add vmcoreinfo device...
WARNING: line over 80 characters
#388: FILE: hw/acpi/vmcoreinfo.c:153:
+ VMSTATE_UINT8_ARRAY(vmcoreinfo_addr_le, VMCoreInfoState, sizeof(uint64_t)),
total: 0 errors, 1 warnings, 470 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/8: stubs: add vmcoreinfo_get() stub...
Checking PATCH 4/8: tests: add simple vmcoreinfo test...
Checking PATCH 5/8: dump: add vmcoreinfo ELF note...
Checking PATCH 6/8: kdump: add vmcoreinfo ELF note...
WARNING: line over 80 characters
#30: FILE: dump.c:847:
+ get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc);
WARNING: line over 80 characters
#47: FILE: dump.c:957:
+ get_note_sizes(s, s->vmcoreinfo, &hsize, &name_size, &size_vmcoreinfo_desc);
total: 0 errors, 2 warnings, 32 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 7/8: scripts/dump-guest-memory.py: add vmcoreinfo...
ERROR: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
#26: FILE: hw/acpi/vmcoreinfo.c:168:
+ static volatile VMCoreInfoState *vmcoreinfo_gdb_helper G_GNUC_UNUSED;
total: 1 errors, 0 warnings, 86 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 8/8: MAINTAINERS: add Dump maintainers...
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 23:30 ` Marc-André Lureau
@ 2017-07-14 23:40 ` Michael S. Tsirkin
2017-07-14 23:47 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-14 23:40 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Dave Anderson, Igor Mammedov, Richard Henderson
On Sat, Jul 15, 2017 at 01:30:56AM +0200, Marc-André Lureau wrote:
> Hi
>
> On Sat, Jul 15, 2017 at 1:09 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sat, Jul 15, 2017 at 12:12:06AM +0200, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Fri, Jul 14, 2017 at 10:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Fri, Jul 14, 2017 at 10:04:31PM +0200, Laszlo Ersek wrote:
> > > >> > It worries me that the format of this seems completely undefined
> > > >> > except in the patchset cover letter.
> > > >> > I don't think we should merge this before it is.
> > > >>
> > > >> I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
> > > >> That file is the first level contract between the guest firmware
> > > >> (generally, via the linker/loader), the guest kernel driver
> > > >> (specifically), and QEMU (also specifically).
> > > >>
> > > >> The second level contract is the guest kernel's vmcoreinfo ELF note
> > > >> (which is pointed-to by the first level contract). The layout of that is
> > > >> specified elsewhere indeed (I don't think it belongs here).
> > > >>
> > > >> We've taken care not to trust anything coming from the guest kernel.
> > > >>
> > > >> Can you clarify please?
> > > >>
> > > >> Thanks
> > > >> Laszlo
> > > >
> > > > All there is is this:
> > > >
> > > > +Version 0 content:
> > > > +
> > > > + uint64 paddr:
> > > > + Physical address of the Linux vmcoreinfo ELF note.
> > > > + uint32 size:
> > > > + Size of the vmcoreinfo ELF note.
> > > >
> > > > It isn't defined here what is the Linux vmcoreinfo ELF note.
> > > > You want a bit more info so people trying to use it know where to start
> > > > and what they can get out of it.
> > >
> > > QEMU is not responsible for the content of the ELF note. All there is afaik is:
> > >
> > > https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-kernel-vmcoreinfo
> > >
> > > The rest you need to dig in the kernel code and git history I think.
> >
> > We need to do a good enough job to at least make it possible for people
> > to make use of it without reading your python code.
> >
> > Documentation/kdump/kdump.txt has a bit more information.
> >
> > All of the necessary information about the system kernel's core image is
> > encoded in the ELF format, and stored in a reserved area of memory
> > before a crash. The physical address of the start of the ELF header is
> > passed to the dump-capture kernel through the elfcorehdr= boot
> > parameter. Optionally the size of the ELF header can also be passed
> > when using the elfcorehdr=[size[KMG]@]offset[KMG] syntax.
> >
> >
> > With the dump-capture kernel, you can access the memory image through
> > /proc/vmcore. This exports the dump as an ELF-format file that you can
> > write out using file copy commands such as cp or scp. Further, you can
> > use analysis tools such as the GNU Debugger (GDB) and the Crash tool to
> > debug the dump file. This method ensures that the dump pages are correctly
> > ordered.
> >
> > I know where to find it but most people likely won't be able to.
> >
>
> What do you learn from that regarding vmcoreinfo? That is it being
> used by kdump? and that you can use gdb/crash, that we already use to
> analyse our dumps.
Some things I learn:
That its written by a dump-capture kernel (so you need to set one up
if you want it to work!).
That it's an ELF-format file, that one can use analysis tools such as
the GNU Debugger (GDB) and the Crash tool to debug the dump file.
There's more info scattered in other places.
Why do you get to document it? Because you are the one exposing it
across the hypervisor/vm boundary where it will need to be
understood by people/tools not running within guest.
So "just read the script in qemu source" is not how an interface
should be documented.
> >
> > BTW why does it pass ELF header size? Any idea?
>
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d3bf37955d46718ee1a7f1fc69f953d2328ba7c2
Right but I mean, ELF header has two options: 32 and 64 bit, that's all.
--
MST
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 23:40 ` Michael S. Tsirkin
@ 2017-07-14 23:47 ` Marc-André Lureau
2017-07-26 17:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-14 23:47 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Dave Anderson, Igor Mammedov, Richard Henderson
On Sat, Jul 15, 2017 at 1:40 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Jul 15, 2017 at 01:30:56AM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Sat, Jul 15, 2017 at 1:09 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> >
>> > On Sat, Jul 15, 2017 at 12:12:06AM +0200, Marc-André Lureau wrote:
>> > > Hi
>> > >
>> > > On Fri, Jul 14, 2017 at 10:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > > > On Fri, Jul 14, 2017 at 10:04:31PM +0200, Laszlo Ersek wrote:
>> > > >> > It worries me that the format of this seems completely undefined
>> > > >> > except in the patchset cover letter.
>> > > >> > I don't think we should merge this before it is.
>> > > >>
>> > > >> I'm not sure what you mean, this patch adds "docs/specs/vmcoreinfo.txt".
>> > > >> That file is the first level contract between the guest firmware
>> > > >> (generally, via the linker/loader), the guest kernel driver
>> > > >> (specifically), and QEMU (also specifically).
>> > > >>
>> > > >> The second level contract is the guest kernel's vmcoreinfo ELF note
>> > > >> (which is pointed-to by the first level contract). The layout of that is
>> > > >> specified elsewhere indeed (I don't think it belongs here).
>> > > >>
>> > > >> We've taken care not to trust anything coming from the guest kernel.
>> > > >>
>> > > >> Can you clarify please?
>> > > >>
>> > > >> Thanks
>> > > >> Laszlo
>> > > >
>> > > > All there is is this:
>> > > >
>> > > > +Version 0 content:
>> > > > +
>> > > > + uint64 paddr:
>> > > > + Physical address of the Linux vmcoreinfo ELF note.
>> > > > + uint32 size:
>> > > > + Size of the vmcoreinfo ELF note.
>> > > >
>> > > > It isn't defined here what is the Linux vmcoreinfo ELF note.
>> > > > You want a bit more info so people trying to use it know where to start
>> > > > and what they can get out of it.
>> > >
>> > > QEMU is not responsible for the content of the ELF note. All there is afaik is:
>> > >
>> > > https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-kernel-vmcoreinfo
>> > >
>> > > The rest you need to dig in the kernel code and git history I think.
>> >
>> > We need to do a good enough job to at least make it possible for people
>> > to make use of it without reading your python code.
>> >
>> > Documentation/kdump/kdump.txt has a bit more information.
>> >
>> > All of the necessary information about the system kernel's core image is
>> > encoded in the ELF format, and stored in a reserved area of memory
>> > before a crash. The physical address of the start of the ELF header is
>> > passed to the dump-capture kernel through the elfcorehdr= boot
>> > parameter. Optionally the size of the ELF header can also be passed
>> > when using the elfcorehdr=[size[KMG]@]offset[KMG] syntax.
>> >
>> >
>> > With the dump-capture kernel, you can access the memory image through
>> > /proc/vmcore. This exports the dump as an ELF-format file that you can
>> > write out using file copy commands such as cp or scp. Further, you can
>> > use analysis tools such as the GNU Debugger (GDB) and the Crash tool to
>> > debug the dump file. This method ensures that the dump pages are correctly
>> > ordered.
>> >
>> > I know where to find it but most people likely won't be able to.
>> >
>>
>> What do you learn from that regarding vmcoreinfo? That is it being
>> used by kdump? and that you can use gdb/crash, that we already use to
>> analyse our dumps.
>
> Some things I learn:
>
> That its written by a dump-capture kernel (so you need to set one up
> if you want it to work!).
I don't follow you, you don't need a dump-capture kernel / kdump to
have the vmcoreinfo note.
>
> That it's an ELF-format file, that one can use analysis tools such as
> the GNU Debugger (GDB) and the Crash tool to debug the dump file.
Well, that's what kdump does, but qemu is the one making the dump in
our case. Quite irrelevant for us.
>
> There's more info scattered in other places.
>
> Why do you get to document it? Because you are the one exposing it
> across the hypervisor/vm boundary where it will need to be
> understood by people/tools not running within guest.
>
> So "just read the script in qemu source" is not how an interface
> should be documented.
I don't understand the issue, it's a kernel ELF note that qemu passes
for dump/crash tools in the dump headers/sections.
>
>> >
>> > BTW why does it pass ELF header size? Any idea?
>>
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d3bf37955d46718ee1a7f1fc69f953d2328ba7c2
>
> Right but I mean, ELF header has two options: 32 and 64 bit, that's all.
>
> --
> MST
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-14 23:29 ` Michael S. Tsirkin
@ 2017-07-18 13:29 ` Marc-André Lureau
2017-07-18 16:05 ` Ladi Prosek
0 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-18 13:29 UTC (permalink / raw)
To: Michael S. Tsirkin, Ladi Prosek
Cc: Laszlo Ersek, Igor Mammedov, Dave Anderson, QEMU, Eduardo Habkost
Hi
On Fri, Jul 14, 2017 at 4:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Jul 15, 2017 at 12:31:36AM +0200, Marc-André Lureau wrote:
>> Hi
>>
>> On Sat, Jul 15, 2017 at 12:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> > On Fri, Jul 14, 2017 at 10:21:43PM +0200, Laszlo Ersek wrote:
>> >> On 07/14/17 21:59, Michael S. Tsirkin wrote:
>> >> > On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
>> >> >> memory range to the guest to store various informations useful to
>> >> >> debug the guest OS. (it is greatly inspired by the VMGENID device
>> >> >> implementation). The version field with value 0 is meant to give
>> >> >> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
>> >> >> different purposes or OSes. (note: some wanted to see pvpanic somehow
>> >> >> merged with this device, I have no clear idea how to do that, nor do I
>> >> >> think this is a good idea since the devices are quite different, used
>> >> >> at different time for different purposes. And it can be done as a
>> >> >> future iteration if it is appropriate, feel free to send patches)
>> >> >
>> >> > First, I think you underestimate the difficulty of maintaining
>> >> > compatibility.
>> >> >
>> >> > Second, this seems racy - how do you know when is guest done writing out
>> >> > the data?
>> >>
>> >> What data exactly?
>> >>
>> >> The guest kernel module points the fields in the "vmcoreinfo page" to
>> >> the then-existent vmcoreinfo ELF note. At that point, the ELF note is
>> >> complete.
>> >
>> > When does this happen?
>>
>> Very early boot afaik. But the exact details on when to expose it is
>> left to the kernel side. For now, it's a test module I load manually.
>>
>> >
>> >> If we catch the guest with a dump request while the kernel module is
>> >> setting up the fields (i.e., the fields are not consistent), then we'll
>> >> catch that in our sanity checks, and the note won't be extracted.
>> >
>> > Are there assumptions about e.g. in which order pa and size
>> > are written out then? Atomicity of these writes?
>>
>> I expect it to be atomic, but as Laszlo said, the code should be quite
>> careful when trying to read the data.
>
> Same when writing it out. Worth documenting too.
>
>> >
>> >> This
>> >> is no different from the case when you simply dump the guest RAM before
>> >> the module got invoked.
>> >>
>> >> > Given you have very little data to export (PA, size - do
>> >> > you even need size?)
>> >>
>> >> Yes, it tells us in advance how much memory to allocate before we copy
>> >> out the vmcoreinfo ELF note (and we enforce a sanity limit on the size).
>> >>
>> >> > - how about just using an ACPI method do it,
>> >>
>> >> Do what exactly?
>> >
>> > Pass address + size to host - that's what the interface is doing,
>> > isn't it?
>> >
>>
>>
>> The memory region is meant to be usable for other OS, or to export
>> more details in the future.
>
> That's the issue. You will never be able to increment version
> just to add more data because that will break old hypervisors.
Could you be more explicit on what will break?
>
>> I think if we add a method, it would be to
>> tell qemu that the memory has been written, but it may still be
>> corrupted at the time we read it. So I am not sure it will really help
>
> So don't. Just pass PA and size to method as arguments and let it figure
> out how to pass it to QEMU. To extend, you will simply add another
> method - which one is present tells you what does hypervisor
> support, which one is called tells you what does guest support.
>
> What to do there internally? I don't mind it if it sticks this data
> in reserved memory like you did here. And then you won't need to
> reserve a full 4K, just a couple of bytes as it will be safe to extend.
>
I can see how for example nvdimm methods are implemented, there is a
memory region reserved for data exchange, and an IO NTFY to give qemu
execution context. Is this how we should design the interface?
I would like to hear from Ladi how he intended to use the device in
the future, and if he would also prefer ACPI methods and what those
methods should be.
>>
>> >> > instead of exporting a physical addess and storing address there. This
>> >> > way you can add more methods as you add functionality.
>> >>
>> >> I'm not saying this is a bad idea (especially because I don't fully
>> >> understand your point), but I will say that I'm quite sad that you are
>> >> sending Marc-André back to the drawing board after he posted v4 -- also
>> >> invalidating my review efforts. :/
>> >>
>> >> Laszlo
>> >
>> > You are right, I should have looked at this sooner. Early RFC
>> > suggested writing into fw cfg directly. I couldn't find any
>> > discussion around this - why was this abandoned?
>>
>> Violation (or rather abuse) of layers iirc
>
> Hmm. I tried to find discussion about that and failed. It is available
> through QEMU0002 in ACPI - would it be OK if guests went through that?
I wouldn't mind, although merging functionality in a single device
isn't what I would think of first. I guess Ladi would be happier with
a single device. I suppose it shouldn't break drivers if we had
memory, io, methods etc to the device?
Laszlo, what do you think if we add vmcoreinfo methods to QEMU0002?
> I do not mind the extra indirection so much, but I don't think
> host/guest interface compatibility issues are fully thought through.
>
>>
>>
>> --
>> Marc-André Lureau
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-18 13:29 ` Marc-André Lureau
@ 2017-07-18 16:05 ` Ladi Prosek
2017-07-18 16:18 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Ladi Prosek @ 2017-07-18 16:05 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael S. Tsirkin, Laszlo Ersek, Igor Mammedov, Dave Anderson,
QEMU, Eduardo Habkost
Hi Marc-Andre,
On Tue, Jul 18, 2017 at 3:29 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Fri, Jul 14, 2017 at 4:29 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Jul 15, 2017 at 12:31:36AM +0200, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Sat, Jul 15, 2017 at 12:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>>> > On Fri, Jul 14, 2017 at 10:21:43PM +0200, Laszlo Ersek wrote:
>>> >> On 07/14/17 21:59, Michael S. Tsirkin wrote:
>>> >> > On Fri, Jul 14, 2017 at 08:20:03PM +0200, Marc-André Lureau 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 vmcoreinfo device is an emulated ACPI device that exposes a 4k
>>> >> >> memory range to the guest to store various informations useful to
>>> >> >> debug the guest OS. (it is greatly inspired by the VMGENID device
>>> >> >> implementation). The version field with value 0 is meant to give
>>> >> >> paddr/size of the VMCOREINFO ELF PT_NOTE, other values can be used for
>>> >> >> different purposes or OSes. (note: some wanted to see pvpanic somehow
>>> >> >> merged with this device, I have no clear idea how to do that, nor do I
>>> >> >> think this is a good idea since the devices are quite different, used
>>> >> >> at different time for different purposes. And it can be done as a
>>> >> >> future iteration if it is appropriate, feel free to send patches)
>>> >> >
>>> >> > First, I think you underestimate the difficulty of maintaining
>>> >> > compatibility.
>>> >> >
>>> >> > Second, this seems racy - how do you know when is guest done writing out
>>> >> > the data?
>>> >>
>>> >> What data exactly?
>>> >>
>>> >> The guest kernel module points the fields in the "vmcoreinfo page" to
>>> >> the then-existent vmcoreinfo ELF note. At that point, the ELF note is
>>> >> complete.
>>> >
>>> > When does this happen?
>>>
>>> Very early boot afaik. But the exact details on when to expose it is
>>> left to the kernel side. For now, it's a test module I load manually.
>>>
>>> >
>>> >> If we catch the guest with a dump request while the kernel module is
>>> >> setting up the fields (i.e., the fields are not consistent), then we'll
>>> >> catch that in our sanity checks, and the note won't be extracted.
>>> >
>>> > Are there assumptions about e.g. in which order pa and size
>>> > are written out then? Atomicity of these writes?
>>>
>>> I expect it to be atomic, but as Laszlo said, the code should be quite
>>> careful when trying to read the data.
>>
>> Same when writing it out. Worth documenting too.
>>
>>> >
>>> >> This
>>> >> is no different from the case when you simply dump the guest RAM before
>>> >> the module got invoked.
>>> >>
>>> >> > Given you have very little data to export (PA, size - do
>>> >> > you even need size?)
>>> >>
>>> >> Yes, it tells us in advance how much memory to allocate before we copy
>>> >> out the vmcoreinfo ELF note (and we enforce a sanity limit on the size).
>>> >>
>>> >> > - how about just using an ACPI method do it,
>>> >>
>>> >> Do what exactly?
>>> >
>>> > Pass address + size to host - that's what the interface is doing,
>>> > isn't it?
>>> >
>>>
>>>
>>> The memory region is meant to be usable for other OS, or to export
>>> more details in the future.
>>
>> That's the issue. You will never be able to increment version
>> just to add more data because that will break old hypervisors.
>
> Could you be more explicit on what will break?
>
>>
>>> I think if we add a method, it would be to
>>> tell qemu that the memory has been written, but it may still be
>>> corrupted at the time we read it. So I am not sure it will really help
>>
>> So don't. Just pass PA and size to method as arguments and let it figure
>> out how to pass it to QEMU. To extend, you will simply add another
>> method - which one is present tells you what does hypervisor
>> support, which one is called tells you what does guest support.
>>
>> What to do there internally? I don't mind it if it sticks this data
>> in reserved memory like you did here. And then you won't need to
>> reserve a full 4K, just a couple of bytes as it will be safe to extend.
>>
>
> I can see how for example nvdimm methods are implemented, there is a
> memory region reserved for data exchange, and an IO NTFY to give qemu
> execution context. Is this how we should design the interface?
>
> I would like to hear from Ladi how he intended to use the device in
> the future, and if he would also prefer ACPI methods and what those
> methods should be.
We should be able to drive pretty much anything from Windows. I wrote
a dummy driver for your earlier prototype just to be sure that ACPI
methods are fine, as I had not done or seen that before.
There are constraints which may be unique to Windows, though. If the
dump-support data is kept in guest-allocated memory, the guest must be
able to revoke it (set / call the method with NULL PA?) because
Windows drivers must free everything on unload. Unlike Linux, I don't
think we can get a piece of memory at startup and keep it for as long
as the OS running. It would be flagged as a memory leak. But that
should be easy to have. Can't really think of anything else.
>>>
>>> >> > instead of exporting a physical addess and storing address there. This
>>> >> > way you can add more methods as you add functionality.
>>> >>
>>> >> I'm not saying this is a bad idea (especially because I don't fully
>>> >> understand your point), but I will say that I'm quite sad that you are
>>> >> sending Marc-André back to the drawing board after he posted v4 -- also
>>> >> invalidating my review efforts. :/
>>> >>
>>> >> Laszlo
>>> >
>>> > You are right, I should have looked at this sooner. Early RFC
>>> > suggested writing into fw cfg directly. I couldn't find any
>>> > discussion around this - why was this abandoned?
>>>
>>> Violation (or rather abuse) of layers iirc
>>
>> Hmm. I tried to find discussion about that and failed. It is available
>> through QEMU0002 in ACPI - would it be OK if guests went through that?
>
> I wouldn't mind, although merging functionality in a single device
> isn't what I would think of first. I guess Ladi would be happier with
> a single device. I suppose it shouldn't break drivers if we had
> memory, io, methods etc to the device?
Yeah, it would be nice if this was part of pvpanic. Even something
really simple like:
/* The bit of supported pv event */
#define PVPANIC_F_PANICKED 0
+#define PVPANIC_F_DUMP_INFO_SET 1
- memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic", 1);
+ memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s,
"pvpanic", 32); // or whatever
The guest writes to two or three registers: PA, size, type?, then sets
the PVPANIC_F_DUMP_INFO_SET bit.
Although not sure if extending the I/O region is OK. And of course I
only need this on x86 :p
Thanks!
> Laszlo, what do you think if we add vmcoreinfo methods to QEMU0002?
>
>> I do not mind the extra indirection so much, but I don't think
>> host/guest interface compatibility issues are fully thought through.
>>
>>>
>>>
>>> --
>>> Marc-André Lureau
>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-18 16:05 ` Ladi Prosek
@ 2017-07-18 16:18 ` Marc-André Lureau
2017-07-19 6:06 ` Ladi Prosek
0 siblings, 1 reply; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-18 16:18 UTC (permalink / raw)
To: Ladi Prosek
Cc: Michael S. Tsirkin, Laszlo Ersek, Igor Mammedov, Dave Anderson,
QEMU, Eduardo Habkost
Hi
On Tue, Jul 18, 2017 at 6:05 PM Ladi Prosek <lprosek@redhat.com> wrote:
>
> > I would like to hear from Ladi how he intended to use the device in
> > the future, and if he would also prefer ACPI methods and what those
> > methods should be.
>
> We should be able to drive pretty much anything from Windows. I wrote
> a dummy driver for your earlier prototype just to be sure that ACPI
> methods are fine, as I had not done or seen that before.
>
> There are constraints which may be unique to Windows, though. If the
> dump-support data is kept in guest-allocated memory, the guest must be
> able to revoke it (set / call the method with NULL PA?) because
> Windows drivers must free everything on unload. Unlike Linux, I don't
>
Well, the currently proposed vmcoreinfo device has a 4k memory region to
put anything you want, Windows shouldn't be allowed to touch it directly
(e820 regions iirc)
think we can get a piece of memory at startup and keep it for as long
> as the OS running. It would be flagged as a memory leak. But that
> should be easy to have. Can't really think of anything else.
>
The question is what kind of data you want to give to the host to help with
debug. Is this something that can be as simple as addr/size, or you would
rather have a 4k page to put various things there.
>
> >>>
> >>> >> > instead of exporting a physical addess and storing address
> there. This
> >>> >> > way you can add more methods as you add functionality.
> >>> >>
> >>> >> I'm not saying this is a bad idea (especially because I don't fully
> >>> >> understand your point), but I will say that I'm quite sad that you
> are
> >>> >> sending Marc-André back to the drawing board after he posted v4 --
> also
> >>> >> invalidating my review efforts. :/
> >>> >>
> >>> >> Laszlo
> >>> >
> >>> > You are right, I should have looked at this sooner. Early RFC
> >>> > suggested writing into fw cfg directly. I couldn't find any
> >>> > discussion around this - why was this abandoned?
> >>>
> >>> Violation (or rather abuse) of layers iirc
> >>
> >> Hmm. I tried to find discussion about that and failed. It is available
> >> through QEMU0002 in ACPI - would it be OK if guests went through that?
> >
> > I wouldn't mind, although merging functionality in a single device
> > isn't what I would think of first. I guess Ladi would be happier with
> > a single device. I suppose it shouldn't break drivers if we had
> > memory, io, methods etc to the device?
>
> Yeah, it would be nice if this was part of pvpanic. Even something
> really simple like:
>
> /* The bit of supported pv event */
> #define PVPANIC_F_PANICKED 0
> +#define PVPANIC_F_DUMP_INFO_SET 1
>
QEMU0002 is fw_cfg
>
> - memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic",
> 1);
> + memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s,
> "pvpanic", 32); // or whatever
>
> The guest writes to two or three registers: PA, size, type?, then sets
> the PVPANIC_F_DUMP_INFO_SET bit.
>
> Although not sure if extending the I/O region is OK. And of course I
> only need this on x86 :p
>
>
I would rather have a solution that works on various archs. It's a shame
pvpanic was designed with x86 only in mind imho.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support
2017-07-18 16:18 ` Marc-André Lureau
@ 2017-07-19 6:06 ` Ladi Prosek
0 siblings, 0 replies; 34+ messages in thread
From: Ladi Prosek @ 2017-07-19 6:06 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Michael S. Tsirkin, Laszlo Ersek, Igor Mammedov, Dave Anderson,
QEMU, Eduardo Habkost
On Tue, Jul 18, 2017 at 6:18 PM, Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
> Hi
>
> On Tue, Jul 18, 2017 at 6:05 PM Ladi Prosek <lprosek@redhat.com> wrote:
>>
>>
>> > I would like to hear from Ladi how he intended to use the device in
>> > the future, and if he would also prefer ACPI methods and what those
>> > methods should be.
>>
>> We should be able to drive pretty much anything from Windows. I wrote
>> a dummy driver for your earlier prototype just to be sure that ACPI
>> methods are fine, as I had not done or seen that before.
>>
>> There are constraints which may be unique to Windows, though. If the
>> dump-support data is kept in guest-allocated memory, the guest must be
>> able to revoke it (set / call the method with NULL PA?) because
>> Windows drivers must free everything on unload. Unlike Linux, I don't
>
>
> Well, the currently proposed vmcoreinfo device has a 4k memory region to put
> anything you want, Windows shouldn't be allowed to touch it directly (e820
> regions iirc)
Got it. And we would likely use it to put just addr/size there to make
updates atomic. I think we're supposed to update our dump-support data
on memory hot-plug for example.
>> think we can get a piece of memory at startup and keep it for as long
>> as the OS running. It would be flagged as a memory leak. But that
>> should be easy to have. Can't really think of anything else.
>
>
> The question is what kind of data you want to give to the host to help with
> debug. Is this something that can be as simple as addr/size, or you would
> rather have a 4k page to put various things there.
The size of the dump header as currently provided by Windows (that's
the dump-support data we want to pass to the host) is 4k. So I
wouldn't put it directly in the page anyway. That, plus the desire to
update the data at least somewhat atomically, would make me prefer a
simple addr/size pair I think.
>>
>>
>> >>>
>> >>> >> > instead of exporting a physical addess and storing address there.
>> >>> >> > This
>> >>> >> > way you can add more methods as you add functionality.
>> >>> >>
>> >>> >> I'm not saying this is a bad idea (especially because I don't fully
>> >>> >> understand your point), but I will say that I'm quite sad that you
>> >>> >> are
>> >>> >> sending Marc-André back to the drawing board after he posted v4 --
>> >>> >> also
>> >>> >> invalidating my review efforts. :/
>> >>> >>
>> >>> >> Laszlo
>> >>> >
>> >>> > You are right, I should have looked at this sooner. Early RFC
>> >>> > suggested writing into fw cfg directly. I couldn't find any
>> >>> > discussion around this - why was this abandoned?
>> >>>
>> >>> Violation (or rather abuse) of layers iirc
>> >>
>> >> Hmm. I tried to find discussion about that and failed. It is
>> >> available
>> >> through QEMU0002 in ACPI - would it be OK if guests went through that?
>> >
>> > I wouldn't mind, although merging functionality in a single device
>> > isn't what I would think of first. I guess Ladi would be happier with
>> > a single device. I suppose it shouldn't break drivers if we had
>> > memory, io, methods etc to the device?
>>
>> Yeah, it would be nice if this was part of pvpanic. Even something
>> really simple like:
>>
>> /* The bit of supported pv event */
>> #define PVPANIC_F_PANICKED 0
>> +#define PVPANIC_F_DUMP_INFO_SET 1
>
>
> QEMU0002 is fw_cfg
Ah, sorry, I got confused.
>>
>>
>> - memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s, "pvpanic",
>> 1);
>> + memory_region_init_io(&s->io, OBJECT(s), &pvpanic_ops, s,
>> "pvpanic", 32); // or whatever
>>
>> The guest writes to two or three registers: PA, size, type?, then sets
>> the PVPANIC_F_DUMP_INFO_SET bit.
>>
>> Although not sure if extending the I/O region is OK. And of course I
>> only need this on x86 :p
>>
>
> I would rather have a solution that works on various archs. It's a shame
> pvpanic was designed with x86 only in mind imho.
Understood. Thanks!
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-14 23:47 ` Marc-André Lureau
@ 2017-07-26 17:21 ` Michael S. Tsirkin
2017-07-28 14:52 ` Marc-André Lureau
0 siblings, 1 reply; 34+ messages in thread
From: Michael S. Tsirkin @ 2017-07-26 17:21 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Dave Anderson, Igor Mammedov, Richard Henderson
On Sat, Jul 15, 2017 at 01:47:50AM +0200, Marc-André Lureau wrote:
> >
> > There's more info scattered in other places.
> >
> > Why do you get to document it? Because you are the one exposing it
> > across the hypervisor/vm boundary where it will need to be
> > understood by people/tools not running within guest.
> >
> > So "just read the script in qemu source" is not how an interface
> > should be documented.
>
> I don't understand the issue, it's a kernel ELF note that qemu passes
> for dump/crash tools in the dump headers/sections.
The way it looks to me, this patchset is exposing an internal kernel
detail and making it part of ABI maybe it already is, my point was 1.
should we get a confirmation from upstream it's not going to change? 2.
if it's ABI let's document what do we expect to be there.
But again since there's not a whole lot of documentation here
that you provided, I might be misunderstanding completely.
> >
> >> >
> >> > BTW why does it pass ELF header size? Any idea?
> >>
> >>
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d3bf37955d46718ee1a7f1fc69f953d2328ba7c2
> >
> > Right but I mean, ELF header has two options: 32 and 64 bit, that's all.
> >
> > --
> > MST
>
>
>
> --
> Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-26 17:21 ` Michael S. Tsirkin
@ 2017-07-28 14:52 ` Marc-André Lureau
2017-07-28 15:55 ` Laszlo Ersek
2017-08-07 15:44 ` Dave Anderson
0 siblings, 2 replies; 34+ messages in thread
From: Marc-André Lureau @ 2017-07-28 14:52 UTC (permalink / raw)
To: Dave Anderson
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Igor Mammedov, Richard Henderson, Michael S. Tsirkin
Hi Dave
On Wed, Jul 26, 2017 at 10:21 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Sat, Jul 15, 2017 at 01:47:50AM +0200, Marc-André Lureau wrote:
>> >
>> > There's more info scattered in other places.
>> >
>> > Why do you get to document it? Because you are the one exposing it
>> > across the hypervisor/vm boundary where it will need to be
>> > understood by people/tools not running within guest.
>> >
>> > So "just read the script in qemu source" is not how an interface
>> > should be documented.
>>
>> I don't understand the issue, it's a kernel ELF note that qemu passes
>> for dump/crash tools in the dump headers/sections.
>
> The way it looks to me, this patchset is exposing an internal kernel
> detail and making it part of ABI maybe it already is, my point was 1.
> should we get a confirmation from upstream it's not going to change? 2.
> if it's ABI let's document what do we expect to be there.
Could you help explain the expectations and stability guarantees of
vmcoreinfo ELF note ?
I am a bit stuck here, after all, vmcoreinfo is mostly used by crash
so I thought you could help.
The only thing qemu does with it is try to get NUMBER(phys_base)=
field to update the phys_base used in the various dump headers. (this
could be dropped, and qemu ignoring the note content, if the debug
tools take vmcoreinfo values with higher priority than other header
fields)
> But again since there's not a whole lot of documentation here
> that you provided, I might be misunderstanding completely.
Because there isn't much available in the kernel either, except
Documentation/ABI/testing/sysfs-kernel-vmcoreinfo.
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-28 14:52 ` Marc-André Lureau
@ 2017-07-28 15:55 ` Laszlo Ersek
2017-08-07 15:44 ` Dave Anderson
1 sibling, 0 replies; 34+ messages in thread
From: Laszlo Ersek @ 2017-07-28 15:55 UTC (permalink / raw)
To: Marc-André Lureau, Dave Anderson
Cc: Eduardo Habkost, QEMU, Paolo Bonzini, Igor Mammedov,
Richard Henderson, Michael S. Tsirkin
On 07/28/17 16:52, Marc-André Lureau wrote:
> Hi Dave
>
> On Wed, Jul 26, 2017 at 10:21 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sat, Jul 15, 2017 at 01:47:50AM +0200, Marc-André Lureau wrote:
>>>>
>>>> There's more info scattered in other places.
>>>>
>>>> Why do you get to document it? Because you are the one exposing it
>>>> across the hypervisor/vm boundary where it will need to be
>>>> understood by people/tools not running within guest.
>>>>
>>>> So "just read the script in qemu source" is not how an interface
>>>> should be documented.
>>>
>>> I don't understand the issue, it's a kernel ELF note that qemu passes
>>> for dump/crash tools in the dump headers/sections.
>>
>> The way it looks to me, this patchset is exposing an internal kernel
>> detail and making it part of ABI maybe it already is, my point was 1.
>> should we get a confirmation from upstream it's not going to change? 2.
>> if it's ABI let's document what do we expect to be there.
>
>
> Could you help explain the expectations and stability guarantees of
> vmcoreinfo ELF note ?
>
> I am a bit stuck here, after all, vmcoreinfo is mostly used by crash
> so I thought you could help.
>
> The only thing qemu does with it is try to get NUMBER(phys_base)=
> field to update the phys_base used in the various dump headers. (this
> could be dropped, and qemu ignoring the note content, if the debug
> tools take vmcoreinfo values with higher priority than other header
> fields)
I agree; if "crash" guarantees that the vmcoreinfo note will override
whatever phys_base value QEMU may have guessed otherwise (from other
places) and written to some dedicated phys_base header fields, then in
QEMU we don't have to propagate phys_base from the vmcoreinfo note to
said other fields -- we can treat the vmcoreinfo note entirely opaquely.
Thanks
Laszlo
>
>> But again since there's not a whole lot of documentation here
>> that you provided, I might be misunderstanding completely.
>
> Because there isn't much available in the kernel either, except
> Documentation/ABI/testing/sysfs-kernel-vmcoreinfo.
>
>
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device
2017-07-28 14:52 ` Marc-André Lureau
2017-07-28 15:55 ` Laszlo Ersek
@ 2017-08-07 15:44 ` Dave Anderson
1 sibling, 0 replies; 34+ messages in thread
From: Dave Anderson @ 2017-08-07 15:44 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laszlo Ersek, Eduardo Habkost, QEMU, Paolo Bonzini,
Igor Mammedov, Richard Henderson, Michael S. Tsirkin
----- Original Message -----
> Hi Dave
>
> On Wed, Jul 26, 2017 at 10:21 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Sat, Jul 15, 2017 at 01:47:50AM +0200, Marc-André Lureau wrote:
> >> >
> >> > There's more info scattered in other places.
> >> >
> >> > Why do you get to document it? Because you are the one exposing it
> >> > across the hypervisor/vm boundary where it will need to be
> >> > understood by people/tools not running within guest.
> >> >
> >> > So "just read the script in qemu source" is not how an interface
> >> > should be documented.
> >>
> >> I don't understand the issue, it's a kernel ELF note that qemu passes
> >> for dump/crash tools in the dump headers/sections.
> >
> > The way it looks to me, this patchset is exposing an internal kernel
> > detail and making it part of ABI maybe it already is, my point was 1.
> > should we get a confirmation from upstream it's not going to change? 2.
> > if it's ABI let's document what do we expect to be there.
>
>
> Could you help explain the expectations and stability guarantees of
> vmcoreinfo ELF note ?
>
> I am a bit stuck here, after all, vmcoreinfo is mostly used by crash
> so I thought you could help.
Sorry for the delay, I'm just back from vacation...
The vmcoreinfo string data is *primarily* used by the makedumpfile facility,
because it needs to find its way around the dumped kernel memory without having
the the vmlinux file's debuginfo data when it's running in the second kernel.
Since crash utilizes the vmlinux file, it only reads a small handful of
the vmcoreinfo items for things like phys_base that cannot be gleaned
from the vmlinux debuginfo data.
Anyway, I'm not sure what you mean by "expectations and stability guarantees"?
It's just a block of ASCII string data of a given size that simply needs
to be transferred to the vmcore. New strings may be added to it at any
time, but that shouldn't have any impact on what you're doing.
Dave
> The only thing qemu does with it is try to get NUMBER(phys_base)=
> field to update the phys_base used in the various dump headers. (this
> could be dropped, and qemu ignoring the note content, if the debug
> tools take vmcoreinfo values with higher priority than other header
> fields)
>
> > But again since there's not a whole lot of documentation here
> > that you provided, I might be misunderstanding completely.
>
> Because there isn't much available in the kernel either, except
> Documentation/ABI/testing/sysfs-kernel-vmcoreinfo.
>
>
> --
> Marc-André Lureau
>
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2017-08-07 15:51 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-14 18:20 [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 1/8] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
2017-07-14 19:19 ` Michael S. Tsirkin
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 2/8] acpi: add vmcoreinfo device Marc-André Lureau
2017-07-14 19:26 ` Michael S. Tsirkin
2017-07-14 20:04 ` Laszlo Ersek
2017-07-14 20:17 ` Michael S. Tsirkin
2017-07-14 22:12 ` Marc-André Lureau
2017-07-14 23:09 ` Michael S. Tsirkin
2017-07-14 23:30 ` Marc-André Lureau
2017-07-14 23:40 ` Michael S. Tsirkin
2017-07-14 23:47 ` Marc-André Lureau
2017-07-26 17:21 ` Michael S. Tsirkin
2017-07-28 14:52 ` Marc-André Lureau
2017-07-28 15:55 ` Laszlo Ersek
2017-08-07 15:44 ` Dave Anderson
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 3/8] stubs: add vmcoreinfo_get() stub Marc-André Lureau
2017-07-14 20:09 ` Laszlo Ersek
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 4/8] tests: add simple vmcoreinfo test Marc-André Lureau
2017-07-14 20:10 ` Laszlo Ersek
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 5/8] dump: add vmcoreinfo ELF note Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 6/8] kdump: " Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 7/8] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
2017-07-14 18:20 ` [Qemu-devel] [PATCH v4 8/8] MAINTAINERS: add Dump maintainers Marc-André Lureau
2017-07-14 19:59 ` [Qemu-devel] [PATCH v4 0/8] KASLR kernel dump support Michael S. Tsirkin
2017-07-14 20:21 ` Laszlo Ersek
2017-07-14 22:23 ` Michael S. Tsirkin
2017-07-14 22:31 ` Marc-André Lureau
2017-07-14 23:29 ` Michael S. Tsirkin
2017-07-18 13:29 ` Marc-André Lureau
2017-07-18 16:05 ` Ladi Prosek
2017-07-18 16:18 ` Marc-André Lureau
2017-07-19 6:06 ` Ladi Prosek
2017-07-14 23:36 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.