On 01/25/17 02:43, b...@skyportsystems.com wrote:
> From: Ben Warren
>
> This implements the VM Generation ID feature by passing a 128-bit
> GUID to the guest via a fw_cfg blob.
> Any time the GUID changes, and ACPI notify event is sent to the guest
(1) typo: "and" -> "an"
>
> The user interface is a simple device with one parameter:
> - guid (string, must be "auto" or in UUID format
>----)
>
> Signed-off-by: Ben Warren
> ---
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> hw/acpi/Makefile.objs| 1 +
> hw/acpi/vmgenid.c| 244
> +++
> hw/i386/acpi-build.c | 9 ++
> include/hw/acpi/acpi_dev_interface.h | 1 +
> include/hw/acpi/vmgenid.h| 32 +
> 7 files changed, 289 insertions(+)
> create mode 100644 hw/acpi/vmgenid.c
> create mode 100644 include/hw/acpi/vmgenid.h
>
> diff --git a/default-configs/i386-softmmu.mak
> b/default-configs/i386-softmmu.mak
> index 0b51360..b2bccf6 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -56,3 +56,4 @@ CONFIG_IOH3420=y
> CONFIG_I82801B11=y
> CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> +CONFIG_ACPI_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak
> b/default-configs/x86_64-softmmu.mak
> index 7f89503..c6bd310 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -56,3 +56,4 @@ CONFIG_IOH3420=y
> CONFIG_I82801B11=y
> CONFIG_SMBIOS=y
> CONFIG_HYPERV_TESTDEV=$(CONFIG_KVM)
> +CONFIG_ACPI_VMGENID=y
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index 6acf798..11c35bc 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -5,6 +5,7 @@ common-obj-$(CONFIG_ACPI_CPU_HOTPLUG) += cpu_hotplug.o
> 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-$(call lnot,$(CONFIG_ACPI_X86)) += acpi-stub.o
>
> common-obj-y += acpi_interface.o
> diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c
> new file mode 100644
> index 000..63cb039
> --- /dev/null
> +++ b/hw/acpi/vmgenid.c
> @@ -0,0 +1,244 @@
> +/*
> + * Virtual Machine Generation ID Device
> + *
> + * Copyright (C) 2017 Skyport Systems.
> + *
> + * Author: Ben Warren
> + *
> + * 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 "qmp-commands.h"
> +#include "hw/acpi/acpi.h"
> +#include "hw/acpi/aml-build.h"
> +#include "hw/acpi/vmgenid.h"
> +#include "hw/nvram/fw_cfg.h"
> +#include "sysemu/sysemu.h"
> +
> +Object *find_vmgenid_dev(Error **errp)
> +{
> +Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> +if (!obj) {
> +error_setg(errp, VMGENID_DEVICE " is not found");
> +}
(2) For general cleanliness, we should use "%s" in the format string,
and pass VMGENID_DEVICE as an argument.
> +return obj;
> +}
> +
> +void vmgenid_build_acpi(GArray *table_offsets, GArray *table_data,
> + BIOSLinker *linker)
(3) I think the "table_offsets" parameter should be dropped.
In this function, that param is used only for calling acpi_add_table().
Looking at other similar call sites in acpi_build(), the pattern is always:
if (check_feature_or_device()) {
acpi_add_table(table_offsets, table_data);
build_stuff_for_feature_or_device();
}
That is, the parameter should be dropped, and the acpi_add_table() call
should be moved out to acpi_build().
> +{
> +Object *obj;
> +VmGenIdState *s;
> +GArray *guid, *vgia;
> +Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx;
> +uint32_t vgia_offset;
> +
> +obj = find_vmgenid_dev(NULL);
> +if (!obj) {
> +return;
> +}
> +s = VMGENID(obj);
(4) The call to this function, from acpi_build(), is already protected
by has_vmgenid(). (And, see my suggestion near the end of this email
about rebasing has_vmgenid() to find_vmgenid_dev().) Therefore we should
simply assert that find_vmgenid_dev() succeeds.
> +
> +acpi_add_table(table_offsets, table_data);
(So this should be moved to acpi_build(), see (3) above.)
> +
> +guid = g_array_new(false, true, sizeof(s->guid.data));
> +g_array_append_val(guid, s->guid.data);
(5) We're not considering host vs. guest endianness.
Let me quote the VmGenIdState structure:
typedef struct VmGenIdState {
SysBusDevice parent_obj;
QemuUUID guid;
uint64_t vgia;
} VmGenIdState;
According to the documentation of QemuUUID, and the qemu_uuid_parse()
and qemu_uuid_unparse() functions -- used later in this patch --, the
representation of the UUID fields is big-endian in the QemuUUID structure.
However, the Windows gue
