* [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support
@ 2017-07-06 10:16 Marc-André Lureau
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
` (6 more replies)
0 siblings, 7 replies; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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
v2: from Laszlo review
- vmci: fix guest endianess handling
- vmci: fix wrong sizeof()
- vmci: add back reset logic from vmgenid
- dump: have 1MB size limit for vmcoreinfo
- dump: fix potential off-by-1 buffer manipulation
- dump: use temporary variable for qemu_strtou64
- dump: fixed VMCOREINFO duplication in kdump
- update gdb script to not call into qemu process
- update MAINTAINERS with some new files
Marc-André Lureau (7):
vmgenid: replace x-write-pointer-available hack
acpi: add vmcoreinfo device
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 | 40 +++++++
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/bios-linker-loader.h | 2 +
include/hw/acpi/vmcoreinfo.h | 36 ++++++
include/hw/compat.h | 4 -
include/sysemu/dump.h | 2 +
dump.c | 145 ++++++++++++++++++++++++
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 +++
tests/vmcoreinfo-test.c | 130 +++++++++++++++++++++
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 +
tests/Makefile.include | 2 +
20 files changed, 747 insertions(+), 12 deletions(-)
create mode 100644 include/hw/acpi/vmcoreinfo.h
create mode 100644 hw/acpi/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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 15:55 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device Marc-André Lureau
` (5 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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>
---
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..388d932538 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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 16:08 ` Laszlo Ersek
2017-07-07 13:13 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 3/7] tests: add simple vmcoreinfo test Marc-André Lureau
` (4 subsequent siblings)
6 siblings, 2 replies; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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>
---
include/hw/acpi/aml-build.h | 1 +
include/hw/acpi/vmcoreinfo.h | 36 +++++++
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, 403 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..9f72a1f263
--- /dev/null
+++ b/include/hw/acpi/vmcoreinfo.h
@@ -0,0 +1,36 @@
+#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"
+
+#define VMCOREINFO_FW_CFG_SIZE 4096 /* Occupy a page of memory */
+#define VMCOREINFO_OFFSET 40 /* allow space for
+ * OVMF SDT Header Probe Supressor
+ */
+
+#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 5464977424..6266b9d1bf 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..70d9716fe0
--- /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 + 0x28)
+ 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 40 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 |
++-----------------------------------+ | 40: uint32 version field |
+ | 44: 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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 3/7] tests: add simple vmcoreinfo test
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 16:18 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 4/7] dump: add vmcoreinfo ELF note Marc-André Lureau
` (3 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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 | 130 ++++++++++++++++++++++++++++++++++++++++++++++++
tests/Makefile.include | 2 +
2 files changed, 132 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..c8b073366e
--- /dev/null
+++ b/tests/vmcoreinfo-test.c
@@ -0,0 +1,130 @@
+/*
+ * 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 "acpi-utils.h"
+#include "libqtest.h"
+
+#define VMCOREINFO_OFFSET 40 /* allow space for
+ * OVMF SDT Header Probe Supressor
+ */
+#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
+#define RSDP_SLEEP_US 100000 /* Sleep for 100ms between tries */
+#define RSDP_TRIES_MAX 100 /* Max total time is 10 seconds */
+
+typedef struct {
+ AcpiTableHeader header;
+ gchar name_op;
+ gchar vcia[4];
+ gchar val_op;
+ uint32_t vcia_val;
+} QEMU_PACKED VgidTable;
+
+static uint32_t acpi_find_vcia(void)
+{
+ uint32_t off;
+ AcpiRsdpDescriptor rsdp_table;
+ uint32_t rsdt;
+ AcpiRsdtDescriptorRev1 rsdt_table;
+ int tables_nr;
+ uint32_t *tables;
+ AcpiTableHeader ssdt_table;
+ VgidTable vgid_table;
+ int i;
+
+ /* Tables may take a short time to be set up by the guest */
+ for (i = 0; i < RSDP_TRIES_MAX; i++) {
+ off = acpi_find_rsdp_address();
+ if (off < RSDP_ADDR_INVALID) {
+ break;
+ }
+ g_usleep(RSDP_SLEEP_US);
+ }
+ g_assert_cmphex(off, <, RSDP_ADDR_INVALID);
+
+ acpi_parse_rsdp_table(off, &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(vgid_table.name_op, tables[i]);
+ g_assert(vgid_table.name_op == 0x08); /* name */
+ ACPI_READ_ARRAY(vgid_table.vcia, tables[i]);
+ g_assert(memcmp(vgid_table.vcia, "VCIA", 4) == 0);
+ ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
+ g_assert(vgid_table.val_op == 0x0C); /* dword */
+ ACPI_READ_FIELD(vgid_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 vgid_table.vcia_val;
+ }
+ }
+ g_free(tables);
+ return 0;
+}
+
+static void vmcoreinfo_test(void)
+{
+ gchar *cmd;
+ uint32_t vmci_addr;
+ int i;
+
+ cmd = g_strdup_printf("-machine accel=tcg -device vmcoreinfo,id=vmci");
+ 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;
+
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_add_func("/vmcoreinfo/test", vmcoreinfo_test);
+ ret = g_test_run();
+
+ return ret;
+}
diff --git a/tests/Makefile.include b/tests/Makefile.include
index 18cd06a6b3..f573c1d3c8 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/acpi-utils.o
+tests/vmcoreinfo-test$(EXESUF): tests/vmcoreinfo-test.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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 4/7] dump: add vmcoreinfo ELF note
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
` (2 preceding siblings ...)
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 3/7] tests: add simple vmcoreinfo test Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 17:01 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 5/7] kdump: " Marc-André Lureau
` (2 subsequent siblings)
6 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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_dump_info_get()
(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>
---
include/sysemu/dump.h | 2 +
dump.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 127 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..f699198204 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,11 @@
#define ELF_MACHINE_UNAME "Unknown"
#endif
+#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 +83,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 +244,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 +280,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 +327,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 +740,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 +1552,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) {
+ error_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 +1657,37 @@ static void dump_init(DumpState *s, int fd, bool has_format,
goto cleanup;
}
+#define MAX_VMCOREINFO_SIZE (1 << 20) /* 1MB should be enough */
+ 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)) {
+ error_report_err(err);
+ } else if (size < note_head_size || size > MAX_VMCOREINFO_SIZE) {
+ error_report("vmcoreinfo size is invalid: %u", 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 (s->vmcoreinfo_size > size) {
+ error_report("Invalid vmcoreinfo header, size mismatch");
+ 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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 5/7] kdump: add vmcoreinfo ELF note
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
` (3 preceding siblings ...)
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 4/7] dump: add vmcoreinfo ELF note Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 17:13 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 7/7] MAINTAINERS: add Dump maintainers Marc-André Lureau
6 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
dump.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/dump.c b/dump.c
index f699198204..dd416ad271 100644
--- a/dump.c
+++ b/dump.c
@@ -839,6 +839,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);
@@ -939,6 +949,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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
` (4 preceding siblings ...)
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 5/7] kdump: " Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 17:29 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 7/7] MAINTAINERS: add Dump maintainers Marc-André Lureau
6 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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 little global vmcoreinfo_gdb
structure, that is populated with vmcoreinfo_gdb_update().
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
scripts/dump-guest-memory.py | 40 ++++++++++++++++++++++++++++++++++++++++
hw/acpi/vmcoreinfo.c | 3 +++
2 files changed, 43 insertions(+)
diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
index f7c6635f15..2dd2ed6983 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,20 @@ 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."""
+ chead = type(get_arch_note(self.endianness, 0, 0))
+ header = chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
+ note = get_arch_note(self.endianness,
+ header.n_namesz - 1, header.n_descsz)
+ ctypes.memmove(ctypes.pointer(note), vmcoreinfo, ctypes.sizeof(note))
+ header_size = ctypes.sizeof(note) - header.n_descsz
+
+ 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 +520,30 @@ 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 < block["target_end"]:
+ haddr = block["host_addr"] + (addr - block["target_start"])
+ return qemu_core.read_memory(haddr, size)
+
+ def add_vmcoreinfo(self):
+ if not gdb.parse_and_eval("vmcoreinfo_gdb_helper"):
+ return
+
+ addr = gdb.parse_and_eval("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)
+ (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 +557,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..b6bcb47506 100644
--- a/hw/acpi/vmcoreinfo.c
+++ b/hw/acpi/vmcoreinfo.c
@@ -163,6 +163,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
}
+static VMCoreInfoState *vmcoreinfo_gdb_helper;
+
static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
{
if (!bios_linker_loader_can_write_pointer()) {
@@ -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] 26+ messages in thread
* [Qemu-devel] [PATCH v2 7/7] MAINTAINERS: add Dump maintainers
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
` (5 preceding siblings ...)
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
@ 2017-07-06 10:16 ` Marc-André Lureau
2017-07-06 17:14 ` Laszlo Ersek
6 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 10:16 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>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 839f7ca063..ba17ce5b85 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1272,6 +1272,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] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
@ 2017-07-06 15:55 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 15:55 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: ehabkost, Ben Warren, Michael S. Tsirkin, anderson, imammedo
On 07/06/17 12:16, 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>
> ---
> 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..388d932538 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()
> +*/
The closing "*/" is not correctly indented.
With that fixed,
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> +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);
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device Marc-André Lureau
@ 2017-07-06 16:08 ` Laszlo Ersek
2017-07-07 13:13 ` Laszlo Ersek
1 sibling, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 16:08 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: ehabkost, Michael S. Tsirkin, Paolo Bonzini, anderson, imammedo,
Richard Henderson
On 07/06/17 12:16, 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>
> ---
> include/hw/acpi/aml-build.h | 1 +
> include/hw/acpi/vmcoreinfo.h | 36 +++++++
> 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, 403 insertions(+)
> create mode 100644 include/hw/acpi/vmcoreinfo.h
> create mode 100644 hw/acpi/vmcoreinfo.c
> create mode 100644 docs/specs/vmcoreinfo.txt
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/7] tests: add simple vmcoreinfo test
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 3/7] tests: add simple vmcoreinfo test Marc-André Lureau
@ 2017-07-06 16:18 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 16:18 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: ehabkost, anderson, imammedo
I didn't look at this patch in the previous version, hoping that someone
else would. Apparently that hasn't happened, so I'll comment, purely
based on a comparison with the vmgenid test:
On 07/06/17 12:16, 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 | 130 ++++++++++++++++++++++++++++++++++++++++++++++++
> tests/Makefile.include | 2 +
> 2 files changed, 132 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..c8b073366e
> --- /dev/null
> +++ b/tests/vmcoreinfo-test.c
> @@ -0,0 +1,130 @@
> +/*
> + * 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 "acpi-utils.h"
> +#include "libqtest.h"
> +
> +#define VMCOREINFO_OFFSET 40 /* allow space for
> + * OVMF SDT Header Probe Supressor
> + */
> +#define RSDP_ADDR_INVALID 0x100000 /* RSDP must be below this address */
> +#define RSDP_SLEEP_US 100000 /* Sleep for 100ms between tries */
> +#define RSDP_TRIES_MAX 100 /* Max total time is 10 seconds */
> +
> +typedef struct {
> + AcpiTableHeader header;
> + gchar name_op;
> + gchar vcia[4];
> + gchar val_op;
> + uint32_t vcia_val;
> +} QEMU_PACKED VgidTable;
This should be called something resembling "vmcoreinfo table".
> +
> +static uint32_t acpi_find_vcia(void)
> +{
> + uint32_t off;
> + AcpiRsdpDescriptor rsdp_table;
> + uint32_t rsdt;
> + AcpiRsdtDescriptorRev1 rsdt_table;
> + int tables_nr;
> + uint32_t *tables;
> + AcpiTableHeader ssdt_table;
> + VgidTable vgid_table;
ditto.
With these fixed:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
> + int i;
> +
> + /* Tables may take a short time to be set up by the guest */
> + for (i = 0; i < RSDP_TRIES_MAX; i++) {
> + off = acpi_find_rsdp_address();
> + if (off < RSDP_ADDR_INVALID) {
> + break;
> + }
> + g_usleep(RSDP_SLEEP_US);
> + }
> + g_assert_cmphex(off, <, RSDP_ADDR_INVALID);
> +
> + acpi_parse_rsdp_table(off, &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(vgid_table.name_op, tables[i]);
> + g_assert(vgid_table.name_op == 0x08); /* name */
> + ACPI_READ_ARRAY(vgid_table.vcia, tables[i]);
> + g_assert(memcmp(vgid_table.vcia, "VCIA", 4) == 0);
> + ACPI_READ_FIELD(vgid_table.val_op, tables[i]);
> + g_assert(vgid_table.val_op == 0x0C); /* dword */
> + ACPI_READ_FIELD(vgid_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 vgid_table.vcia_val;
> + }
> + }
> + g_free(tables);
> + return 0;
> +}
> +
> +static void vmcoreinfo_test(void)
> +{
> + gchar *cmd;
> + uint32_t vmci_addr;
> + int i;
> +
> + cmd = g_strdup_printf("-machine accel=tcg -device vmcoreinfo,id=vmci");
> + 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;
> +
> + g_test_init(&argc, &argv, NULL);
> +
> + qtest_add_func("/vmcoreinfo/test", vmcoreinfo_test);
> + ret = g_test_run();
> +
> + return ret;
> +}
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 18cd06a6b3..f573c1d3c8 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/acpi-utils.o
> +tests/vmcoreinfo-test$(EXESUF): tests/vmcoreinfo-test.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)$@")
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/7] dump: add vmcoreinfo ELF note
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 4/7] dump: add vmcoreinfo ELF note Marc-André Lureau
@ 2017-07-06 17:01 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 17:01 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: ehabkost, anderson, imammedo
On 07/06/17 12:16, Marc-André Lureau wrote:
> 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_dump_info_get()
I recommend using the clipboard; the function is still called
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>
> ---
> include/sysemu/dump.h | 2 +
> dump.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 127 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..f699198204 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,11 @@
> #define ELF_MACHINE_UNAME "Unknown"
> #endif
>
> +#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 +83,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 +244,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 +280,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 +327,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 +740,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 +1552,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) {
> + error_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 +1657,37 @@ static void dump_init(DumpState *s, int fd, bool has_format,
> goto cleanup;
> }
>
> +#define MAX_VMCOREINFO_SIZE (1 << 20) /* 1MB should be enough */
I think we generally place macros like this near the top of the file.
You already #define ELF_NOTE_SIZE up there.
> + if (vmcoreinfo_dev) {
I'm sorry, I should have asked for this in the previous version -- can
you place a comment here, above the "if", saying that the goal of this
block is to (a) update the previously guessed phys_base, (b) copy the
vmcoreinfo note out of the guest? And that failure to do so is not fatal
for dumping.
> + 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)) {
> + error_report_err(err);
Apologies, I should have noticed this too in the previous version. After
you print and free the error with error_report_err(), you should also
null the "err" variable. The dump_init() function goes on to receive
further errors into "err", and if "err" is not NULL at that point, two
things can happen:
- if a function uses error_propagate() into "err", then "err" will
appear to have been set earlier, and error_propagate() will drop the new
error.
- if error_setg() or similar is used instead, then that will trip an assert.
> + } else if (size < note_head_size || size > MAX_VMCOREINFO_SIZE) {
> + error_report("vmcoreinfo size is invalid: %u", size);
Consider using "%"PRIu32 for stylistic reasons, although in practice %u
is perfectly fine for uint32_t (since uint32_t *is* "unsigned int").
> + } 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);
So, to answer your earlier question, please check both "name_size" and
"desc_size" individually against MAX_VMCOREINFO_SIZE. (While preserving
the cumulative size check below, of course.) This is simple enough and
ensures neither the roundings nor the additions can overflow later.
If any of these checks fails, you'll have to free and null s->vmcoreinfo
here too.
The rest looks good!
Thanks!
Laszlo
> + s->vmcoreinfo_size = ELF_NOTE_SIZE(note_head_size, name_size,
> + desc_size);
> + if (s->vmcoreinfo_size > size) {
> + error_report("Invalid vmcoreinfo header, size mismatch");
> + 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);
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/7] kdump: add vmcoreinfo ELF note
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 5/7] kdump: " Marc-André Lureau
@ 2017-07-06 17:13 ` Laszlo Ersek
2017-07-06 17:21 ` Marc-André Lureau
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 17:13 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: ehabkost, anderson, imammedo
On 07/06/17 12:16, Marc-André Lureau wrote:
> kdump header provides offset and size of the vmcoreinfo ELF note,
> append it if available.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> dump.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/dump.c b/dump.c
> index f699198204..dd416ad271 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -839,6 +839,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);
>
> @@ -939,6 +949,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);
>
>
I continue to think that this patch is unnecessary, but if you insist,
it does look OK to me.
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 7/7] MAINTAINERS: add Dump maintainers
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 7/7] MAINTAINERS: add Dump maintainers Marc-André Lureau
@ 2017-07-06 17:14 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 17:14 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel; +Cc: ehabkost, anderson, imammedo
On 07/06/17 12:16, Marc-André Lureau wrote:
> Proposing myself, since I have some familiarity with the code now.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> MAINTAINERS | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 839f7ca063..ba17ce5b85 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1272,6 +1272,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
>
Acked-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/7] kdump: add vmcoreinfo ELF note
2017-07-06 17:13 ` Laszlo Ersek
@ 2017-07-06 17:21 ` Marc-André Lureau
2017-07-06 18:09 ` Dave Anderson
0 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-06 17:21 UTC (permalink / raw)
To: Laszlo Ersek, Dave Anderson; +Cc: QEMU, Igor Mammedov, Eduardo Habkost
Hi
On Thu, Jul 6, 2017 at 7:13 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> On 07/06/17 12:16, Marc-André Lureau wrote:
>> kdump header provides offset and size of the vmcoreinfo ELF note,
>> append it if available.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>> dump.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/dump.c b/dump.c
>> index f699198204..dd416ad271 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -839,6 +839,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);
>>
>> @@ -939,6 +949,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);
>>
>>
>
> I continue to think that this patch is unnecessary, but if you insist,
> it does look OK to me.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Without it, crash doesn't read the vmcoreinfo PT_NOTE. And for some
reason, the phys_base in the header wasn't enough (to be doubled
checked).
Any comment Dave about crash handling of vmcoreinfo in kdump files?
--
Marc-André Lureau
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
@ 2017-07-06 17:29 ` Laszlo Ersek
2017-07-11 10:04 ` Marc-André Lureau
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 17:29 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: ehabkost, Michael S. Tsirkin, anderson, imammedo
On 07/06/17 12:16, Marc-André Lureau wrote:
> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>
> To help the python script, add a little global vmcoreinfo_gdb
> structure, that is populated with vmcoreinfo_gdb_update().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> scripts/dump-guest-memory.py | 40 ++++++++++++++++++++++++++++++++++++++++
> hw/acpi/vmcoreinfo.c | 3 +++
> 2 files changed, 43 insertions(+)
>
> diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> index f7c6635f15..2dd2ed6983 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,20 @@ 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."""
> + chead = type(get_arch_note(self.endianness, 0, 0))
> + header = chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
Maybe it's obvious to others, but I would have been helped a lot if a
comment had explained that you are creating a fake note (with 0 desc
size and 0 name size) to figure out the size of the note header. And
then you copy that many bytes out of the vmcoreinfo ELF note.
> + note = get_arch_note(self.endianness,
> + header.n_namesz - 1, header.n_descsz)
Why the -1?
... I think I'm giving up here for this method. My python is weak and I
can't follow this too well. Please add some comments.
More comments below:
> + ctypes.memmove(ctypes.pointer(note), vmcoreinfo, ctypes.sizeof(note))
> + header_size = ctypes.sizeof(note) - header.n_descsz
> +
> + 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 +520,30 @@ 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 < block["target_end"]:
Although I don't expect a single read to straddle phys-blocks, I would
prefer if you checked (addr + size) -- and not just addr -- against
block["target_end"].
> + haddr = block["host_addr"] + (addr - block["target_start"])
> + return qemu_core.read_memory(haddr, size)
> +
> + def add_vmcoreinfo(self):
> + if not gdb.parse_and_eval("vmcoreinfo_gdb_helper"):
> + return
> +
> + addr = gdb.parse_and_eval("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)
> + (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 +557,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..b6bcb47506 100644
> --- a/hw/acpi/vmcoreinfo.c
> +++ b/hw/acpi/vmcoreinfo.c
> @@ -163,6 +163,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
> memset(vis->vmcoreinfo_addr_le, 0, ARRAY_SIZE(vis->vmcoreinfo_addr_le));
> }
>
> +static VMCoreInfoState *vmcoreinfo_gdb_helper;
> +
> static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
> {
> if (!bios_linker_loader_can_write_pointer()) {
> @@ -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);
> }
>
>
I guess we don't build QEMU with link-time optimization at the moment.
With link-time optimization, I think gcc might reasonably optimize away
the assignment to "vmcoreinfo_gdb_helper", and "vmcoreinfo_gdb_helper"
itself. This is why I suggested "volatile":
static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
Do you think volatile is only superfluous, or do you actively dislike it
for some reason?
Thanks,
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/7] kdump: add vmcoreinfo ELF note
2017-07-06 17:21 ` Marc-André Lureau
@ 2017-07-06 18:09 ` Dave Anderson
2017-07-06 18:51 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Dave Anderson @ 2017-07-06 18:09 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: Laszlo Ersek, QEMU, Igor Mammedov, Eduardo Habkost
----- Original Message -----
> Hi
>
> On Thu, Jul 6, 2017 at 7:13 PM, Laszlo Ersek <lersek@redhat.com> wrote:
> > On 07/06/17 12:16, Marc-André Lureau wrote:
> >> kdump header provides offset and size of the vmcoreinfo ELF note,
> >> append it if available.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> ---
> >> dump.c | 20 ++++++++++++++++++++
> >> 1 file changed, 20 insertions(+)
> >>
> >> diff --git a/dump.c b/dump.c
> >> index f699198204..dd416ad271 100644
> >> --- a/dump.c
> >> +++ b/dump.c
> >> @@ -839,6 +839,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);
> >>
> >> @@ -939,6 +949,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);
> >>
> >>
> >
> > I continue to think that this patch is unnecessary, but if you insist,
> > it does look OK to me.
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>
> Without it, crash doesn't read the vmcoreinfo PT_NOTE. And for some
> reason, the phys_base in the header wasn't enough (to be doubled
> checked).
>
> Any comment Dave about crash handling of vmcoreinfo in kdump files?
It just reads the kdump_sub_header's offset_vmcoreinfo and size_vmcoreinfo
fields to gather the vmcoreinfo data into a local buffer of memory, and
scans the strings for whatever it's looking for.
With respect to phys_base, the only thing that might be of consequence
is this fairly recent change that's currently only in the github repo,
queued for crash-7.2.0:
commit a4a538caca140a8e948bbdae2be311168db7a1eb
Author: Dave Anderson <anderson@redhat.com>
Date: Tue May 2 16:51:53 2017 -0400
Fix for Linux 4.10 and later kdump dumpfiles, or kernels that have
backported commit 401721ecd1dcb0a428aa5d6832ee05ffbdbffbbe, titled
"kexec: export the value of phys_base instead of symbol address".
Without the patch, if the x86_64 "phys_base" value in the VMCOREINFO
note is a negative decimal number, the crash session fails during
session intialization with a "page excluded" or "seek error" when
reading "page_offset_base".
(anderson@redhat.com)
Also, 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.
Dave
>
>
>
> --
> Marc-André Lureau
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/7] kdump: add vmcoreinfo ELF note
2017-07-06 18:09 ` Dave Anderson
@ 2017-07-06 18:51 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-06 18:51 UTC (permalink / raw)
To: Dave Anderson, Marc-André Lureau
Cc: QEMU, Igor Mammedov, Eduardo Habkost
On 07/06/17 20:09, Dave Anderson wrote:
>
>
> ----- Original Message -----
>> Hi
>>
>> On Thu, Jul 6, 2017 at 7:13 PM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 07/06/17 12:16, Marc-André Lureau wrote:
>>>> kdump header provides offset and size of the vmcoreinfo ELF note,
>>>> append it if available.
>>>>
>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>> ---
>>>> dump.c | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/dump.c b/dump.c
>>>> index f699198204..dd416ad271 100644
>>>> --- a/dump.c
>>>> +++ b/dump.c
>>>> @@ -839,6 +839,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);
>>>>
>>>> @@ -939,6 +949,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);
>>>>
>>>>
>>>
>>> I continue to think that this patch is unnecessary, but if you insist,
>
>>> it does look OK to me.
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Without it, crash doesn't read the vmcoreinfo PT_NOTE. And for some
>> reason, the phys_base in the header wasn't enough (to be doubled
>> checked).
>>
>> Any comment Dave about crash handling of vmcoreinfo in kdump files?
>
> It just reads the kdump_sub_header's offset_vmcoreinfo and size_vmcoreinfo
> fields to gather the vmcoreinfo data into a local buffer of memory, and
> scans the strings for whatever it's looking for.
>
> With respect to phys_base, the only thing that might be of consequence
> is this fairly recent change that's currently only in the github repo,
> queued for crash-7.2.0:
>
> commit a4a538caca140a8e948bbdae2be311168db7a1eb
> Author: Dave Anderson <anderson@redhat.com>
> Date: Tue May 2 16:51:53 2017 -0400
>
> Fix for Linux 4.10 and later kdump dumpfiles, or kernels that have
> backported commit 401721ecd1dcb0a428aa5d6832ee05ffbdbffbbe, titled
> "kexec: export the value of phys_base instead of symbol address".
> Without the patch, if the x86_64 "phys_base" value in the VMCOREINFO
> note is a negative decimal number, the crash session fails during
> session intialization with a "page excluded" or "seek error" when
> reading "page_offset_base".
> (anderson@redhat.com)
>
> Also, 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.
OK, if crash (or earlier versions of crash) need this QEMU patch, then
I'm fine with it -- my R-b stands.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device Marc-André Lureau
2017-07-06 16:08 ` Laszlo Ersek
@ 2017-07-07 13:13 ` Laszlo Ersek
2017-07-07 13:16 ` Laszlo Ersek
1 sibling, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-07 13:13 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, Michael S. Tsirkin,
Paolo Bonzini, Richard Henderson
Marc-André,
sorry about this, but I have another late comment (I shoudl have pointed
this out in v1). Regarding:
On 07/06/17 12:16, Marc-André Lureau wrote:
> +#define VMCOREINFO_OFFSET 40 /* allow space for
> + * OVMF SDT Header Probe Supressor
> + */
and
> + Method (ADDR, 0, NotSerialized)
> + {
> + Local0 = Package (0x02) {}
> + Local0 [Zero] = (VCIA + 0x28)
> + Local0 [One] = Zero
> + Return (Local0)
> + }
and
> +In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
> +the vmcoreinfo blob has 40 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 |
> ++-----------------------------------+ | 40: uint32 version field |
> + | 44: info contents |
> + | .... |
> + +---------------------------+
> + END OF PAGE
Please define the VMCOREINFO_OFFSET macro like this:
#define VMCOREINFO_OFFSET sizeof(AcpiTableHeader)
and then please refresh the documentation as well:
- replace decimal "40" with "36",
- replace 0x28 in the dumped SSDT with 0x24.
Namely, in order to suppress the OVMF ACP Table Header Probe -- "SDT"
simply means "System Description Table" --, it's enough to add
zero-padding that *precisely* covers such a header.
Given that we have a typedef for this header in QEMU, we should use it
in the definition of VMCOREINFO_OFFSET.
Now, the reason why VMGENID uses 40 instead comes from another
requirement: the VMGENID GUID has to be aligned at 8 bytes. (See
requirement "R1a" in "docs/specs/vmgenid.txt".) Therefore the directly
necessary 36 bytes of padding are rounded up to 40. See again
"docs/specs/vmgenid.txt":
----------------------------------+
| SSDT with OEM Table ID = VMGENID |
+----------------------------------+
| ... | TOP OF PAGE
| VGIA dword object ---------------|-----> +---------------------------+
| ... | | fw-allocated array for |
| _STA method referring to VGIA | | "etc/vmgenid_guid" |
| ... | +---------------------------+
| ADDR method referring to VGIA | | 0: OVMF SDT Header probe |
| ... | | suppressor |
+----------------------------------+ | 36: padding for 8-byte |
| alignment |
| 40: GUID |
| 56: padding to page size |
+---------------------------+
END OF PAGE
At offset 36 of "etc/vmgenid_guid", it says "padding for 8-byte alignment".
For VMCOREINFO, you don't need this extra alignment to 8 bytes, before
the "version" field is listed. So please make the VMCOREINFO_OFFSET
reflect what's actually necessary.
The rest of the code doesn't have to be modified (including your
experimental guest kernel driver); changing VMCOREINFO_OFFSET will
update all necessary locations automatically, including the generated
AML. Only the docs have to be synced manually.
... In fact, there *is* yet another location to update: the test case. I
suggest to include "hw/acpi/vmcoreinfo.h" in "tests/vmcoreinfo-test.c",
rather than open-code VMCOREINFO_OFFSET there. ... Oh wait: in the test
case you don't even use VMCOREINFO_OFFSET for anything. So just delete
the macro definition from that patch.
Thank you (and sorry about the churn),
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device
2017-07-07 13:13 ` Laszlo Ersek
@ 2017-07-07 13:16 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-07 13:16 UTC (permalink / raw)
To: Marc-André Lureau, qemu-devel
Cc: imammedo, anderson, berrange, ehabkost, Michael S. Tsirkin,
Paolo Bonzini, Richard Henderson
On 07/07/17 15:13, Laszlo Ersek wrote:
> Marc-André,
>
> sorry about this, but I have another late comment (I shoudl have pointed
> this out in v1). Regarding:
>
> On 07/06/17 12:16, Marc-André Lureau wrote:
>
>> +#define VMCOREINFO_OFFSET 40 /* allow space for
>> + * OVMF SDT Header Probe Supressor
>> + */
>
> and
>
>> + Method (ADDR, 0, NotSerialized)
>> + {
>> + Local0 = Package (0x02) {}
>> + Local0 [Zero] = (VCIA + 0x28)
>> + Local0 [One] = Zero
>> + Return (Local0)
>> + }
>
> and
>
>> +In order to implement an OVMF "SDT Header Probe Suppressor", the contents of
>> +the vmcoreinfo blob has 40 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 |
>> ++-----------------------------------+ | 40: uint32 version field |
>> + | 44: info contents |
>> + | .... |
>> + +---------------------------+
>> + END OF PAGE
>
> Please define the VMCOREINFO_OFFSET macro like this:
>
> #define VMCOREINFO_OFFSET sizeof(AcpiTableHeader)
>
> and then please refresh the documentation as well:
> - replace decimal "40" with "36",
also replace decimal "44", the offset of "info contents", with "40"...
you get the idea.
Thanks
Laszlo
> - replace 0x28 in the dumped SSDT with 0x24.
>
> Namely, in order to suppress the OVMF ACP Table Header Probe -- "SDT"
> simply means "System Description Table" --, it's enough to add
> zero-padding that *precisely* covers such a header.
>
> Given that we have a typedef for this header in QEMU, we should use it
> in the definition of VMCOREINFO_OFFSET.
>
> Now, the reason why VMGENID uses 40 instead comes from another
> requirement: the VMGENID GUID has to be aligned at 8 bytes. (See
> requirement "R1a" in "docs/specs/vmgenid.txt".) Therefore the directly
> necessary 36 bytes of padding are rounded up to 40. See again
> "docs/specs/vmgenid.txt":
>
> ----------------------------------+
> | SSDT with OEM Table ID = VMGENID |
> +----------------------------------+
> | ... | TOP OF PAGE
> | VGIA dword object ---------------|-----> +---------------------------+
> | ... | | fw-allocated array for |
> | _STA method referring to VGIA | | "etc/vmgenid_guid" |
> | ... | +---------------------------+
> | ADDR method referring to VGIA | | 0: OVMF SDT Header probe |
> | ... | | suppressor |
> +----------------------------------+ | 36: padding for 8-byte |
> | alignment |
> | 40: GUID |
> | 56: padding to page size |
> +---------------------------+
> END OF PAGE
>
> At offset 36 of "etc/vmgenid_guid", it says "padding for 8-byte alignment".
>
> For VMCOREINFO, you don't need this extra alignment to 8 bytes, before
> the "version" field is listed. So please make the VMCOREINFO_OFFSET
> reflect what's actually necessary.
>
> The rest of the code doesn't have to be modified (including your
> experimental guest kernel driver); changing VMCOREINFO_OFFSET will
> update all necessary locations automatically, including the generated
> AML. Only the docs have to be synced manually.
>
> ... In fact, there *is* yet another location to update: the test case. I
> suggest to include "hw/acpi/vmcoreinfo.h" in "tests/vmcoreinfo-test.c",
> rather than open-code VMCOREINFO_OFFSET there. ... Oh wait: in the test
> case you don't even use VMCOREINFO_OFFSET for anything. So just delete
> the macro definition from that patch.
>
> Thank you (and sorry about the churn),
> Laszlo
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-06 17:29 ` Laszlo Ersek
@ 2017-07-11 10:04 ` Marc-André Lureau
2017-07-11 13:25 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-11 10:04 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, ehabkost, Michael S. Tsirkin, anderson, imammedo
Hi
----- Original Message -----
> On 07/06/17 12:16, Marc-André Lureau wrote:
> > Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> >
> > To help the python script, add a little global vmcoreinfo_gdb
> > structure, that is populated with vmcoreinfo_gdb_update().
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> > scripts/dump-guest-memory.py | 40 ++++++++++++++++++++++++++++++++++++++++
> > hw/acpi/vmcoreinfo.c | 3 +++
> > 2 files changed, 43 insertions(+)
> >
> > diff --git a/scripts/dump-guest-memory.py b/scripts/dump-guest-memory.py
> > index f7c6635f15..2dd2ed6983 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,20 @@ 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."""
> > + chead = type(get_arch_note(self.endianness, 0, 0))
> > + header =
> > chead.from_buffer_copy(vmcoreinfo[0:ctypes.sizeof(chead)])
>
> Maybe it's obvious to others, but I would have been helped a lot if a
> comment had explained that you are creating a fake note (with 0 desc
> size and 0 name size) to figure out the size of the note header. And
> then you copy that many bytes out of the vmcoreinfo ELF note.
>
ok
> > + note = get_arch_note(self.endianness,
> > + header.n_namesz - 1, header.n_descsz)
>
> Why the -1?
because get_arch_note() adds + 1 (supposedly for the ending \0, see also add_note())
>
> ... I think I'm giving up here for this method. My python is weak and I
> can't follow this too well. Please add some comments.
I simplified a bit the code now, I was also discovering some ctype usage.
> More comments below:
>
> > + ctypes.memmove(ctypes.pointer(note), vmcoreinfo,
> > ctypes.sizeof(note))
> > + header_size = ctypes.sizeof(note) - header.n_descsz
> > +
> > + 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 +520,30 @@ 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 < block["target_end"]:
>
> Although I don't expect a single read to straddle phys-blocks, I would
> prefer if you checked (addr + size) -- and not just addr -- against
> block["target_end"].
done
>
> > + haddr = block["host_addr"] + (addr -
> > block["target_start"])
> > + return qemu_core.read_memory(haddr, size)
> > +
> > + def add_vmcoreinfo(self):
> > + if not gdb.parse_and_eval("vmcoreinfo_gdb_helper"):
> > + return
> > +
> > + addr =
> > gdb.parse_and_eval("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)
> > + (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 +557,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..b6bcb47506 100644
> > --- a/hw/acpi/vmcoreinfo.c
> > +++ b/hw/acpi/vmcoreinfo.c
> > @@ -163,6 +163,8 @@ static void vmcoreinfo_handle_reset(void *opaque)
> > memset(vis->vmcoreinfo_addr_le, 0,
> > ARRAY_SIZE(vis->vmcoreinfo_addr_le));
> > }
> >
> > +static VMCoreInfoState *vmcoreinfo_gdb_helper;
> > +
> > static void vmcoreinfo_realize(DeviceState *dev, Error **errp)
> > {
> > if (!bios_linker_loader_can_write_pointer()) {
> > @@ -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);
> > }
> >
> >
>
> I guess we don't build QEMU with link-time optimization at the moment.
>
> With link-time optimization, I think gcc might reasonably optimize away
> the assignment to "vmcoreinfo_gdb_helper", and "vmcoreinfo_gdb_helper"
> itself. This is why I suggested "volatile":
>
> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
>
> Do you think volatile is only superfluous, or do you actively dislike it
> for some reason?
Yeah, I am not convinced volatile is the best way, but nor is static.
Let's export it?
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-11 10:04 ` Marc-André Lureau
@ 2017-07-11 13:25 ` Laszlo Ersek
2017-07-11 13:35 ` Marc-André Lureau
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-11 13:25 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, ehabkost, Michael S. Tsirkin, anderson, imammedo
On 07/11/17 12:04, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 07/06/17 12:16, Marc-André Lureau wrote:
>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>>>
>>> To help the python script, add a little global vmcoreinfo_gdb
>>> structure, that is populated with vmcoreinfo_gdb_update().
>>>
>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>> @@ -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);
>>> }
>>>
>>>
>>
>> I guess we don't build QEMU with link-time optimization at the
>> moment.
>>
>> With link-time optimization, I think gcc might reasonably optimize
>> away the assignment to "vmcoreinfo_gdb_helper", and
>> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
>>
>> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
>>
>> Do you think volatile is only superfluous, or do you actively dislike
>> it for some reason?
>
> Yeah, I am not convinced volatile is the best way, but nor is static.
>
> Let's export it?
Volatile guarantees that the assignment will take place according to the
behavior of the "abstract C machine" described by ISO C. From ISO C99,
6.7.3 Type qualifiers
6 An object that has volatile-qualified type may be modified in ways
unknown to the implementation or have other unknown side effects.
Therefore any expression referring to such an object shall be
evaluated strictly according to the rules of the abstract machine,
as described in 5.1.2.3. Furthermore, at every sequence point the
value last stored in the object shall agree with that prescribed by
the abstract machine, except as modified by the unknown factors
mentioned previously. [116] What constitutes an access to an object
that has volatile-qualified type is implementation-defined.
Footnote 116: A volatile declaration may be used to describe an object
corresponding to a memory-mapped input/output port or an
object accessed by an asynchronously interrupting
function. Actions on objects so declared shall not be
"optimized out" by an implementation or reordered except
as permitted by the rules for evaluating expressions.
So basically if you can find the symbol somehow, it will always have the
right value, due to the volatile qualifier, regardless of the
optimizations the compiler did.
Internal vs. external linkage ("static" vs. "extern") is a different
question; it might affect whether you find the symbol at all. IME,
symbols for objects with internal linkage are preserved, if: (a) you
don't strip the final binary, and (b) gcc doesn't optimize the variable
away.
With link-time / full program optimization, gcc is entitled to optimize
away variables with external linkage too, so "extern" in itself is not
bulletproof. Volatile is more important.
Given volatile, I'm sort of neutral on extern vs. static. However, if
you make the variable extern, that makes abuse from other source files
easier. (The variable should only be looked at with gdb.) This is also
why I originally suggested to limit the scope of even the static
variable to function scope -- this way abuse would completely be
prevented (nothing else could access the variable even from the same
translation unit), and gdb would still find the variable (IME).
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-11 13:25 ` Laszlo Ersek
@ 2017-07-11 13:35 ` Marc-André Lureau
2017-07-11 13:49 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-11 13:35 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, ehabkost, Michael S. Tsirkin, anderson, imammedo
Hi
----- Original Message -----
> On 07/11/17 12:04, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> >> On 07/06/17 12:16, Marc-André Lureau wrote:
> >>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> >>>
> >>> To help the python script, add a little global vmcoreinfo_gdb
> >>> structure, that is populated with vmcoreinfo_gdb_update().
> >>>
> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> >>> @@ -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);
> >>> }
> >>>
> >>>
> >>
> >> I guess we don't build QEMU with link-time optimization at the
> >> moment.
> >>
> >> With link-time optimization, I think gcc might reasonably optimize
> >> away the assignment to "vmcoreinfo_gdb_helper", and
> >> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
> >>
> >> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
> >>
> >> Do you think volatile is only superfluous, or do you actively dislike
> >> it for some reason?
> >
> > Yeah, I am not convinced volatile is the best way, but nor is static.
> >
> > Let's export it?
>
> Volatile guarantees that the assignment will take place according to the
> behavior of the "abstract C machine" described by ISO C. From ISO C99,
>
> 6.7.3 Type qualifiers
>
> 6 An object that has volatile-qualified type may be modified in ways
> unknown to the implementation or have other unknown side effects.
> Therefore any expression referring to such an object shall be
> evaluated strictly according to the rules of the abstract machine,
> as described in 5.1.2.3. Furthermore, at every sequence point the
> value last stored in the object shall agree with that prescribed by
> the abstract machine, except as modified by the unknown factors
> mentioned previously. [116] What constitutes an access to an object
> that has volatile-qualified type is implementation-defined.
>
> Footnote 116: A volatile declaration may be used to describe an object
> corresponding to a memory-mapped input/output port or an
> object accessed by an asynchronously interrupting
> function. Actions on objects so declared shall not be
> "optimized out" by an implementation or reordered except
> as permitted by the rules for evaluating expressions.
>
> So basically if you can find the symbol somehow, it will always have the
> right value, due to the volatile qualifier, regardless of the
> optimizations the compiler did.
>
> Internal vs. external linkage ("static" vs. "extern") is a different
> question; it might affect whether you find the symbol at all. IME,
> symbols for objects with internal linkage are preserved, if: (a) you
> don't strip the final binary, and (b) gcc doesn't optimize the variable
> away.
>
> With link-time / full program optimization, gcc is entitled to optimize
> away variables with external linkage too, so "extern" in itself is not
> bulletproof. Volatile is more important.
Ok, you seem confident about this sort of things, I am not, I'll follow you :)
>
> Given volatile, I'm sort of neutral on extern vs. static. However, if
> you make the variable extern, that makes abuse from other source files
> easier. (The variable should only be looked at with gdb.) This is also
> why I originally suggested to limit the scope of even the static
> variable to function scope -- this way abuse would completely be
> prevented (nothing else could access the variable even from the same
> translation unit), and gdb would still find the variable (IME).
How do you access a static inside a function? Gdb can look it up but complains that it's not in current context.
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-11 13:35 ` Marc-André Lureau
@ 2017-07-11 13:49 ` Laszlo Ersek
2017-07-11 13:58 ` Marc-André Lureau
0 siblings, 1 reply; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-11 13:49 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, ehabkost, Michael S. Tsirkin, anderson, imammedo
On 07/11/17 15:35, Marc-André Lureau wrote:
> Hi
>
> ----- Original Message -----
>> On 07/11/17 12:04, Marc-André Lureau wrote:
>>> Hi
>>>
>>> ----- Original Message -----
>>>> On 07/06/17 12:16, Marc-André Lureau wrote:
>>>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
>>>>>
>>>>> To help the python script, add a little global vmcoreinfo_gdb
>>>>> structure, that is populated with vmcoreinfo_gdb_update().
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>>>>> @@ -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);
>>>>> }
>>>>>
>>>>>
>>>>
>>>> I guess we don't build QEMU with link-time optimization at the
>>>> moment.
>>>>
>>>> With link-time optimization, I think gcc might reasonably optimize
>>>> away the assignment to "vmcoreinfo_gdb_helper", and
>>>> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
>>>>
>>>> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
>>>>
>>>> Do you think volatile is only superfluous, or do you actively dislike
>>>> it for some reason?
>>>
>>> Yeah, I am not convinced volatile is the best way, but nor is static.
>>>
>>> Let's export it?
>>
>> Volatile guarantees that the assignment will take place according to the
>> behavior of the "abstract C machine" described by ISO C. From ISO C99,
>>
>> 6.7.3 Type qualifiers
>>
>> 6 An object that has volatile-qualified type may be modified in ways
>> unknown to the implementation or have other unknown side effects.
>> Therefore any expression referring to such an object shall be
>> evaluated strictly according to the rules of the abstract machine,
>> as described in 5.1.2.3. Furthermore, at every sequence point the
>> value last stored in the object shall agree with that prescribed by
>> the abstract machine, except as modified by the unknown factors
>> mentioned previously. [116] What constitutes an access to an object
>> that has volatile-qualified type is implementation-defined.
>>
>> Footnote 116: A volatile declaration may be used to describe an object
>> corresponding to a memory-mapped input/output port or an
>> object accessed by an asynchronously interrupting
>> function. Actions on objects so declared shall not be
>> "optimized out" by an implementation or reordered except
>> as permitted by the rules for evaluating expressions.
>>
>> So basically if you can find the symbol somehow, it will always have the
>> right value, due to the volatile qualifier, regardless of the
>> optimizations the compiler did.
>>
>> Internal vs. external linkage ("static" vs. "extern") is a different
>> question; it might affect whether you find the symbol at all. IME,
>> symbols for objects with internal linkage are preserved, if: (a) you
>> don't strip the final binary, and (b) gcc doesn't optimize the variable
>> away.
>>
>> With link-time / full program optimization, gcc is entitled to optimize
>> away variables with external linkage too, so "extern" in itself is not
>> bulletproof. Volatile is more important.
>
> Ok, you seem confident about this sort of things, I am not, I'll follow you :)
>>
>
>> Given volatile, I'm sort of neutral on extern vs. static. However, if
>> you make the variable extern, that makes abuse from other source files
>> easier. (The variable should only be looked at with gdb.) This is also
>> why I originally suggested to limit the scope of even the static
>> variable to function scope -- this way abuse would completely be
>> prevented (nothing else could access the variable even from the same
>> translation unit), and gdb would still find the variable (IME).
>
> How do you access a static inside a function? Gdb can look it up but complains that it's not in current context.
>
According to <https://sourceware.org/gdb/onlinedocs/gdb/Variables.html>,
the magic incantation is
function::variable
If this doesn't work (in case the realize function isn't on the stack at
all), then I agree the variable should be made file scope.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-11 13:49 ` Laszlo Ersek
@ 2017-07-11 13:58 ` Marc-André Lureau
2017-07-11 15:03 ` Laszlo Ersek
0 siblings, 1 reply; 26+ messages in thread
From: Marc-André Lureau @ 2017-07-11 13:58 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: qemu-devel, ehabkost, Michael S. Tsirkin, anderson, imammedo
Hi
----- Original Message -----
> On 07/11/17 15:35, Marc-André Lureau wrote:
> > Hi
> >
> > ----- Original Message -----
> >> On 07/11/17 12:04, Marc-André Lureau wrote:
> >>> Hi
> >>>
> >>> ----- Original Message -----
> >>>> On 07/06/17 12:16, Marc-André Lureau wrote:
> >>>>> Add vmcoreinfo ELF note if vmcoreinfo device is ready.
> >>>>>
> >>>>> To help the python script, add a little global vmcoreinfo_gdb
> >>>>> structure, that is populated with vmcoreinfo_gdb_update().
> >>>>>
> >>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >>
> >>>>> @@ -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);
> >>>>> }
> >>>>>
> >>>>>
> >>>>
> >>>> I guess we don't build QEMU with link-time optimization at the
> >>>> moment.
> >>>>
> >>>> With link-time optimization, I think gcc might reasonably optimize
> >>>> away the assignment to "vmcoreinfo_gdb_helper", and
> >>>> "vmcoreinfo_gdb_helper" itself. This is why I suggested "volatile":
> >>>>
> >>>> static VMCoreInfoState * volatile vmcoreinfo_gdb_helper;
> >>>>
> >>>> Do you think volatile is only superfluous, or do you actively dislike
> >>>> it for some reason?
> >>>
> >>> Yeah, I am not convinced volatile is the best way, but nor is static.
> >>>
> >>> Let's export it?
> >>
> >> Volatile guarantees that the assignment will take place according to the
> >> behavior of the "abstract C machine" described by ISO C. From ISO C99,
> >>
> >> 6.7.3 Type qualifiers
> >>
> >> 6 An object that has volatile-qualified type may be modified in ways
> >> unknown to the implementation or have other unknown side effects.
> >> Therefore any expression referring to such an object shall be
> >> evaluated strictly according to the rules of the abstract machine,
> >> as described in 5.1.2.3. Furthermore, at every sequence point the
> >> value last stored in the object shall agree with that prescribed by
> >> the abstract machine, except as modified by the unknown factors
> >> mentioned previously. [116] What constitutes an access to an object
> >> that has volatile-qualified type is implementation-defined.
> >>
> >> Footnote 116: A volatile declaration may be used to describe an object
> >> corresponding to a memory-mapped input/output port or an
> >> object accessed by an asynchronously interrupting
> >> function. Actions on objects so declared shall not be
> >> "optimized out" by an implementation or reordered except
> >> as permitted by the rules for evaluating expressions.
> >>
> >> So basically if you can find the symbol somehow, it will always have the
> >> right value, due to the volatile qualifier, regardless of the
> >> optimizations the compiler did.
> >>
> >> Internal vs. external linkage ("static" vs. "extern") is a different
> >> question; it might affect whether you find the symbol at all. IME,
> >> symbols for objects with internal linkage are preserved, if: (a) you
> >> don't strip the final binary, and (b) gcc doesn't optimize the variable
> >> away.
> >>
> >> With link-time / full program optimization, gcc is entitled to optimize
> >> away variables with external linkage too, so "extern" in itself is not
> >> bulletproof. Volatile is more important.
> >
> > Ok, you seem confident about this sort of things, I am not, I'll follow you
> > :)
> >>
> >
> >> Given volatile, I'm sort of neutral on extern vs. static. However, if
> >> you make the variable extern, that makes abuse from other source files
> >> easier. (The variable should only be looked at with gdb.) This is also
> >> why I originally suggested to limit the scope of even the static
> >> variable to function scope -- this way abuse would completely be
> >> prevented (nothing else could access the variable even from the same
> >> translation unit), and gdb would still find the variable (IME).
> >
> > How do you access a static inside a function? Gdb can look it up but
> > complains that it's not in current context.
> >
>
> According to <https://sourceware.org/gdb/onlinedocs/gdb/Variables.html>,
> the magic incantation is
>
> function::variable
>
Thanks that works! I'll update the patch
> If this doesn't work (in case the realize function isn't on the stack at
> all), then I agree the variable should be made file scope.
>
Why would it matter if realize is on the stack? It's likely not :)
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo
2017-07-11 13:58 ` Marc-André Lureau
@ 2017-07-11 15:03 ` Laszlo Ersek
0 siblings, 0 replies; 26+ messages in thread
From: Laszlo Ersek @ 2017-07-11 15:03 UTC (permalink / raw)
To: Marc-André Lureau
Cc: qemu-devel, ehabkost, Michael S. Tsirkin, anderson, imammedo
On 07/11/17 15:58, Marc-André Lureau wrote:
>>> How do you access a static inside a function? Gdb can look it up but
>>> complains that it's not in current context.
>>>
>>
>> According to <https://sourceware.org/gdb/onlinedocs/gdb/Variables.html>,
>> the magic incantation is
>>
>> function::variable
>>
>
> Thanks that works! I'll update the patch
Great!
>
>> If this doesn't work (in case the realize function isn't on the stack at
>> all), then I agree the variable should be made file scope.
>>
>
> Why would it matter if realize is on the stack? It's likely not :)
I agree the realize function will likely not be on the stack, and I also
agree that it should not matter. However, the gdb docs seemed a bit
unclear to me regarding this, so I wasn't 100% sure without trying.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-07-11 15:03 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-06 10:16 [Qemu-devel] [PATCH v2 0/7] KASLR kernel dump support Marc-André Lureau
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 1/7] vmgenid: replace x-write-pointer-available hack Marc-André Lureau
2017-07-06 15:55 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 2/7] acpi: add vmcoreinfo device Marc-André Lureau
2017-07-06 16:08 ` Laszlo Ersek
2017-07-07 13:13 ` Laszlo Ersek
2017-07-07 13:16 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 3/7] tests: add simple vmcoreinfo test Marc-André Lureau
2017-07-06 16:18 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 4/7] dump: add vmcoreinfo ELF note Marc-André Lureau
2017-07-06 17:01 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 5/7] kdump: " Marc-André Lureau
2017-07-06 17:13 ` Laszlo Ersek
2017-07-06 17:21 ` Marc-André Lureau
2017-07-06 18:09 ` Dave Anderson
2017-07-06 18:51 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 6/7] scripts/dump-guest-memory.py: add vmcoreinfo Marc-André Lureau
2017-07-06 17:29 ` Laszlo Ersek
2017-07-11 10:04 ` Marc-André Lureau
2017-07-11 13:25 ` Laszlo Ersek
2017-07-11 13:35 ` Marc-André Lureau
2017-07-11 13:49 ` Laszlo Ersek
2017-07-11 13:58 ` Marc-André Lureau
2017-07-11 15:03 ` Laszlo Ersek
2017-07-06 10:16 ` [Qemu-devel] [PATCH v2 7/7] MAINTAINERS: add Dump maintainers Marc-André Lureau
2017-07-06 17:14 ` Laszlo Ersek
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.