[Qemu-devel] [PATCH V15 0/5] Virtual Machine Generation ID

[Qemu-devel] [PATCH V15 0/5] Virtual Machine Generation ID

[Gal Hammer]

Hi,

Resending patches after going back to the "fixed address" solution and
updating the code to work with the new aml apis.

Thanks,

    Gal.

V15 - Version is based on V7 ("fixed address") with new aml apis.

V13, V14 - Igor Mammedov's patches.

V12 - Fixed bios_linker_loader_add_pointer call parameters. Offset
      should be relative to the table.

V11 - Add required missing files.

V10 - Fixed typos in docs and a few clarification.

V9 - Add a unit test.
   - Rebased to version 2.2.
   - Removed hex.generated the binary files from patch.

V8 - Add a device's description file.
   - GUID is stored in fw cfg file and the guest writes the
     physical address to the device (reduces vmexits).

V7 - Move the device's description back to the static SSDT table.
   - The GUID is store in a "hard coded" physical address and not
     in the ACPI table itself.
   - ACPI notification is triggered when the GUID is changed.

V6 - include the pre-compiled ASL file
   - remove an empty line at end of files.

V5 - Move device's description to SSDT table (dynamic).

V4 - Fix a typo in error message string.
   - Move device's description from DSDT back to SSDT table.

V3 - Remove "-uuid" command line parameter.
   - Move device's description from SSDT to DSDT table.
   - Add new "vmgenid" sysbus device.

Gal Hammer (5):
  docs: vm generation id device's description
  acpi: add a vm_generation_id_changed method
  aml: implement a 32-bit fixed memory range descriptor
  i386: add a Virtual Machine Generation ID device
  tests: add a unit test for the vmgenid device.

 default-configs/i386-softmmu.mak     |   1 +
 default-configs/x86_64-softmmu.mak   |   1 +
 docs/specs/vmgenid.txt               |  35 +++++++++++
 hw/acpi/aml-build.c                  |  18 ++++++
 hw/acpi/core.c                       |   8 +++
 hw/acpi/ich9.c                       |   8 +++
 hw/acpi/piix4.c                      |   8 +++
 hw/i386/acpi-build.c                 |  41 +++++++++++++
 hw/i386/pc.c                         |   8 +++
 hw/isa/lpc_ich9.c                    |   1 +
 hw/misc/Makefile.objs                |   1 +
 hw/misc/vmgenid.c                    | 116 +++++++++++++++++++++++++++++++++++
 include/hw/acpi/acpi.h               |   2 +
 include/hw/acpi/acpi_dev_interface.h |   4 ++
 include/hw/acpi/aml-build.h          |   3 +
 include/hw/acpi/ich9.h               |   2 +
 include/hw/i386/pc.h                 |   3 +
 include/hw/misc/vmgenid.h            |  21 +++++++
 tests/Makefile                       |   2 +
 tests/vmgenid-test.c                 |  44 +++++++++++++
 20 files changed, 327 insertions(+)
 create mode 100644 docs/specs/vmgenid.txt
 create mode 100644 hw/misc/vmgenid.c
 create mode 100644 include/hw/misc/vmgenid.h
 create mode 100644 tests/vmgenid-test.c

-- 
2.1.0

[Qemu-devel] [PATCH V15 1/5] docs: vm generation id device's description

[Gal Hammer]

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 docs/specs/vmgenid.txt | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 docs/specs/vmgenid.txt

diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
new file mode 100644
index 0000000..86ce6ab
--- /dev/null
+++ b/docs/specs/vmgenid.txt
@@ -0,0 +1,35 @@
+VIRTUAL MACHINE GENERATION ID
+=============================
+
+Copyright (C) 2014 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 generation ID (vmgenid) device is an emulated device which
+exposes a 128-bit, cryptographically random, integer value identifier.
+This allows management applications (e.g. libvirt) to notify the guest
+operating system when the virtual machine is executed with a different
+configuration (e.g. snapshot execution or creation from a template).
+
+This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
+
+---
+
+The vmgenid device is a sysbus device with the following ACPI ID:
+"QEMU0002".
+
+The device adds a "vmgenid.uuid" property, which can be modified using
+the -global command line argument or the QMP interface.
+
+The device uses a fixed memory resource: 0xfedf0000-0xfedf000f to store
+the GUID's buffer.
+
+According to the specification, any change to the GUID executes an
+ACPI notification. The vmgenid device triggers the GPE._E00 which
+executes the ACPI Notify operation.
+
+Although not specified in Microsoft's document, it is assumed that the
+device is expected to use the little-endian system.
-- 
2.1.0

[Qemu-devel] [PATCH V15 2/5] acpi: add a vm_generation_id_changed method

[Gal Hammer]

Add a new method to the AcpiDeviceIfClass interface. The new
method send an ACPI notfication when the VM generation id is
changed.

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 hw/acpi/core.c                       | 8 ++++++++
 hw/acpi/ich9.c                       | 8 ++++++++
 hw/acpi/piix4.c                      | 8 ++++++++
 hw/isa/lpc_ich9.c                    | 1 +
 include/hw/acpi/acpi.h               | 2 ++
 include/hw/acpi/acpi_dev_interface.h | 4 ++++
 include/hw/acpi/ich9.h               | 2 ++
 7 files changed, 33 insertions(+)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 51913d6..d4597c6 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -28,6 +28,8 @@
 #include "qapi-visit.h"
 #include "qapi-event.h"
 
+#define ACPI_VM_GENERATION_ID_CHANGED_STATUS 1
+
 struct acpi_table_header {
     uint16_t _length;         /* our length, not actual part of the hdr */
                               /* allows easier parsing for fw_cfg clients */
@@ -683,3 +685,9 @@ void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
                        (regs->pm1.evt.en & ACPI_BITMASK_TIMER_ENABLE) &&
                        !(pm1a_sts & ACPI_BITMASK_TIMER_STATUS));
 }
+
+void acpi_vm_generation_id_changed(ACPIREGS *acpi_regs, qemu_irq irq)
+{
+    acpi_regs->gpe.sts[0] |= ACPI_VM_GENERATION_ID_CHANGED_STATUS;
+    acpi_update_sci(acpi_regs, irq);
+}
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 5352e19..613b82e 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -417,3 +417,11 @@ void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
 
     acpi_memory_ospm_status(&s->pm.acpi_memory_hotplug, list);
 }
+
+void ich9_vm_generation_id_changed(AcpiDeviceIf *adev)
+{
+    ICH9LPCState *s = ICH9_LPC_DEVICE(adev);
+    ICH9LPCPMRegs *pm = &s->pm;
+
+    acpi_vm_generation_id_changed(&pm->acpi_regs, pm->irq);
+}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index d1f1179..4708cfc 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -580,6 +580,13 @@ static void piix4_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list)
     acpi_memory_ospm_status(&s->acpi_memory_hotplug, list);
 }
 
+static void piix4_vm_generation_id_changed(AcpiDeviceIf *adev)
+{
+    PIIX4PMState *s = PIIX4_PM(adev);
+
+    acpi_vm_generation_id_changed(&s->ar, s->irq);
+}
+
 static Property piix4_pm_properties[] = {
     DEFINE_PROP_UINT32("smb_io_base", PIIX4PMState, smb_io_base, 0),
     DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
@@ -618,6 +625,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data)
     hc->unplug_request = piix4_device_unplug_request_cb;
     hc->unplug = piix4_device_unplug_cb;
     adevc->ospm_status = piix4_ospm_status;
+    adevc->vm_generation_id_changed = piix4_vm_generation_id_changed;
 }
 
 static const TypeInfo piix4_pm_info = {
diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
index dba7585..2626fd5 100644
--- a/hw/isa/lpc_ich9.c
+++ b/hw/isa/lpc_ich9.c
@@ -688,6 +688,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void *data)
     hc->unplug_request = ich9_device_unplug_request_cb;
     hc->unplug = ich9_device_unplug_cb;
     adevc->ospm_status = ich9_pm_ospm_status;
+    adevc->vm_generation_id_changed = ich9_vm_generation_id_changed;
 }
 
 static const TypeInfo ich9_lpc_info = {
diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
index 1f678b4..9373b4d 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -185,4 +185,6 @@ unsigned acpi_table_len(void *current);
 void acpi_table_add(const QemuOpts *opts, Error **errp);
 void acpi_table_add_builtin(const QemuOpts *opts, Error **errp);
 
+void acpi_vm_generation_id_changed(ACPIREGS *acpi_regs, qemu_irq irq);
+
 #endif /* !QEMU_HW_ACPI_H */
diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h
index f245f8d..757ce60 100644
--- a/include/hw/acpi/acpi_dev_interface.h
+++ b/include/hw/acpi/acpi_dev_interface.h
@@ -28,6 +28,9 @@ typedef struct AcpiDeviceIf {
  * ospm_status: returns status of ACPI device objects, reported
  *              via _OST method if device supports it.
  *
+ * vm_generation_id_changed: notify the guest that it generation
+ *                           id was changed.
+ *
  * Interface is designed for providing unified interface
  * to generic ACPI functionality that could be used without
  * knowledge about internals of actual device that implements
@@ -39,5 +42,6 @@ typedef struct AcpiDeviceIfClass {
 
     /* <public> */
     void (*ospm_status)(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
+    void (*vm_generation_id_changed)(AcpiDeviceIf *adev);
 } AcpiDeviceIfClass;
 #endif
diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
index c2d3dba..255a113 100644
--- a/include/hw/acpi/ich9.h
+++ b/include/hw/acpi/ich9.h
@@ -69,4 +69,6 @@ void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev,
                               Error **errp);
 
 void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list);
+
+void ich9_vm_generation_id_changed(AcpiDeviceIf *adev);
 #endif /* HW_ACPI_ICH9_H */
-- 
2.1.0

[Qemu-devel] [PATCH V15 3/5] aml: implement a 32-bit fixed memory range descriptor

[Gal Hammer]

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 hw/acpi/aml-build.c         | 18 ++++++++++++++++++
 include/hw/acpi/aml-build.h |  3 +++
 2 files changed, 21 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index d7945f6..038384f 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -891,3 +891,21 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
                              dec, addr_gran, addr_min, addr_max,
                              addr_trans, len, flags);
 }
+
+Aml *aml_memory32_fixed(AmlReadAndWrite read_and_write,
+                        uint32_t addr_base, uint32_t addr_len)
+{
+    Aml *var = aml_alloc();
+
+    /* 32-bit Fixed Memory Range Descriptor */
+    build_append_byte(var->buf, 0x86);
+    /* minimum length since we do not encode optional fields */
+    build_append_byte(var->buf, 0x9);
+    build_append_byte(var->buf, 0x0);
+
+    build_append_byte(var->buf, !!read_and_write & 1);
+    build_append_int_noprefix(var->buf, addr_base, sizeof(addr_base));
+    build_append_int_noprefix(var->buf, addr_len, sizeof(addr_len));
+
+    return var;
+}
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 17d3beb..e3cdd70 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -177,6 +177,9 @@ Aml *aml_qword_memory(AmlDecode dec, AmlMinFixed min_fixed,
                       uint64_t addr_max, uint64_t addr_trans,
                       uint64_t len);
 
+Aml *aml_memory32_fixed(AmlReadAndWrite read_and_write,
+                        uint32_t addr_base, uint32_t addr_len);
+
 /* Block AML object primitives */
 Aml *aml_scope(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
 Aml *aml_device(const char *name_format, ...) GCC_FMT_ATTR(1, 2);
-- 
2.1.0

[Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

Based on Microsoft's sepecifications (paper can be dowloaded from
http://go.microsoft.com/fwlink/?LinkId=260709), add a device
description to the SSDT ACPI table and its implementation.

The GUID is set using a global "vmgenid.uuid" parameter.

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 default-configs/i386-softmmu.mak   |   1 +
 default-configs/x86_64-softmmu.mak |   1 +
 hw/i386/acpi-build.c               |  41 +++++++++++++
 hw/i386/pc.c                       |   8 +++
 hw/misc/Makefile.objs              |   1 +
 hw/misc/vmgenid.c                  | 116 +++++++++++++++++++++++++++++++++++++
 include/hw/i386/pc.h               |   3 +
 include/hw/misc/vmgenid.h          |  21 +++++++
 8 files changed, 192 insertions(+)
 create mode 100644 hw/misc/vmgenid.c
 create mode 100644 include/hw/misc/vmgenid.h

diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 6a74e00..9fc0a3c 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
 CONFIG_XIO3130=y
 CONFIG_IOH3420=y
 CONFIG_I82801B11=y
+CONFIG_VMGENID=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index 46b87dd..f66fd3c 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
 CONFIG_XIO3130=y
 CONFIG_IOH3420=y
 CONFIG_I82801B11=y
+CONFIG_VMGENID=y
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e761005..8d1a761 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -42,6 +42,7 @@
 #include "hw/acpi/memory_hotplug.h"
 #include "sysemu/tpm.h"
 #include "hw/acpi/tpm.h"
+#include "hw/misc/vmgenid.h"
 
 /* Supported chipsets: */
 #include "hw/acpi/piix4.h"
@@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
     unsigned dsdt_size;
     uint16_t pvpanic_port;
     uint16_t applesmc_io_base;
+    bool vm_generation_id_set;
 } AcpiMiscInfo;
 
 typedef struct AcpiBuildPciBusHotplugState {
@@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
     info->has_tpm = tpm_find();
     info->pvpanic_port = pvpanic_port();
     info->applesmc_io_base = applesmc_port();
+    info->vm_generation_id_set = vm_generation_id_set();
 }
 
 static void acpi_get_pci_info(PcPciInfo *info)
@@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
         aml_append(ssdt, scope);
     }
 
+    if (misc->vm_generation_id_set) {
+        scope = aml_scope("\\_SB");
+
+        dev = aml_device("VMGI");
+        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
+        aml_append(dev,
+            aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
+        aml_append(dev,
+            aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
+
+        crs = aml_resource_template();
+        aml_append(crs, aml_memory32_fixed(aml_ReadOnly,
+            VMGENID_BASE_ADDRESS, VMGENID_BASE_ADDR_LEN));
+        aml_append(dev, aml_name_decl("_CRS", crs));
+
+        aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
+
+        method = aml_method("ADDR", 0);
+        pkg = aml_package(2);
+        aml_append(pkg, aml_int(VMGENID_BASE_ADDRESS));
+        aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr */
+        aml_append(method, aml_store(pkg, aml_local(0)));
+        aml_append(method, aml_return(aml_local(0)));
+        aml_append(dev, method);
+
+        aml_append(scope, dev);
+        aml_append(ssdt, scope);
+
+        scope = aml_scope("\\_GPE");
+
+        method = aml_method("_E00", 0);
+        aml_append(method,
+            aml_notify(aml_name("\\_SB.VMGI"), aml_int(0x80)));
+
+        aml_append(scope, method);
+        aml_append(ssdt, scope);
+    }
+
     sb_scope = aml_scope("_SB");
     {
         /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a8e6be1..266c5f3 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -63,6 +63,7 @@
 #include "hw/pci/pci_host.h"
 #include "acpi-build.h"
 #include "hw/mem/pc-dimm.h"
+#include "hw/misc/vmgenid.h"
 #include "trace.h"
 #include "qapi/visitor.h"
 #include "qapi-visit.h"
@@ -1402,6 +1403,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
     int i;
     DriveInfo *fd[MAX_FD];
     DeviceState *hpet = NULL;
+    DeviceState *vmgenid;
     int pit_isa_irq = 0;
     qemu_irq pit_alt_irq = NULL;
     qemu_irq rtc_irq = NULL;
@@ -1491,6 +1493,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
         fd[i] = drive_get(IF_FLOPPY, 0, i);
     }
     *floppy = fdctrl_init_isa(isa_bus, fd);
+
+    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
+    if (vmgenid) {
+        qdev_init_nofail(vmgenid);
+        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
+    }
 }
 
 void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 4aa76ff..3db11de 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -40,3 +40,4 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
 
 obj-$(CONFIG_PVPANIC) += pvpanic.o
 obj-$(CONFIG_EDU) += edu.o
+obj-$(CONFIG_VMGENID) += vmgenid.o
diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
new file mode 100644
index 0000000..01aacd4
--- /dev/null
+++ b/hw/misc/vmgenid.c
@@ -0,0 +1,116 @@
+/*
+ *  Virtual Machine Generation ID Device
+ *
+ *  Copyright (C) 2014 Red Hat Inc.
+ *
+ *  Authors: Gal Hammer <ghammer@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/i386/pc.h"
+#include "hw/sysbus.h"
+#include "hw/misc/vmgenid.h"
+#include "hw/acpi/acpi_dev_interface.h"
+
+#define PROPERTY_UUID "uuid"
+
+#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
+
+typedef struct VmGenIdState {
+    SysBusDevice parent_obj;
+    MemoryRegion iomem;
+    uint8_t guid[16];
+    bool guid_set;
+} VmGenIdState;
+
+bool vm_generation_id_set(void)
+{
+    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
+    VmGenIdState *s = VMGENID(obj);
+
+    if (!obj) {
+        return false;
+    }
+    return s->guid_set;
+}
+
+static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr,
+                                 unsigned size)
+{
+    VmGenIdState *s = VMGENID(opaque);
+    uint64_t value;
+
+    memcpy(&value, s->guid + addr, size);
+    return value;
+}
+
+static const MemoryRegionOps vmgenid_ram_ops = {
+    .read = vmgenid_ram_read,
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
+    .endianness = DEVICE_NATIVE_ENDIAN,
+};
+
+static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
+{
+    VmGenIdState *s = VMGENID(obj);
+    Object *acpi_obj;
+    bool first_set = !s->guid_set;
+
+    if (qemu_uuid_parse(value, s->guid) < 0) {
+        error_setg(errp, "Fail to parse UUID string.");
+        return;
+    }
+    s->guid_set = true;
+
+    /* Skip the acpi notification when setting the vm generation id for the
+     * first time. This is done because in a q35 machine the gpe register is
+     * allocated after the device is initialized. */
+    if (!first_set) {
+        acpi_obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
+        if (acpi_obj) {
+            AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(acpi_obj);
+            AcpiDeviceIf *adev = ACPI_DEVICE_IF(acpi_obj);
+
+            adevc->vm_generation_id_changed(adev);
+        }
+    }
+}
+
+static void vmgenid_init(Object *obj)
+{
+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+    VmGenIdState *s = VMGENID(obj);
+
+    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
+    sysbus_init_mmio(sbd, &s->iomem);
+
+    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
+}
+
+static void vmgenid_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo vmgenid_device_info = {
+    .name          = VMGENID_DEVICE,
+    .parent        = TYPE_SYS_BUS_DEVICE,
+    .instance_size = sizeof(VmGenIdState),
+    .instance_init = vmgenid_init,
+    .class_init    = vmgenid_class_init,
+};
+
+static void vmgenid_register_types(void)
+{
+    type_register_static(&vmgenid_device_info);
+}
+
+type_init(vmgenid_register_types)
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1b35168..9df3a9f 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -284,6 +284,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory,
 /* pvpanic.c */
 uint16_t pvpanic_port(void);
 
+/* vmgenid.c */
+bool vm_generation_id_set(void);
+
 /* e820 types */
 #define E820_RAM        1
 #define E820_RESERVED   2
diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
new file mode 100644
index 0000000..3d44421
--- /dev/null
+++ b/include/hw/misc/vmgenid.h
@@ -0,0 +1,21 @@
+/*
+ *  Virtual Machine Generation ID Device
+ *
+ *  Copyright (C) 2014 Red Hat Inc.
+ *
+ *  Authors: Gal Hammer <ghammer@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef HW_MISC_VMGENID_H
+#define HW_MISC_VMGENID_H
+
+#define VMGENID_DEVICE "vmgenid"
+
+#define VMGENID_BASE_ADDRESS    0xfedf0000
+#define VMGENID_BASE_ADDR_LEN   16
+
+#endif
-- 
2.1.0

[Qemu-devel] [PATCH V15 5/5] tests: add a unit test for the vmgenid device.

[Gal Hammer]

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 tests/Makefile       |  2 ++
 tests/vmgenid-test.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)
 create mode 100644 tests/vmgenid-test.c

diff --git a/tests/Makefile b/tests/Makefile
index 55aa745..6e4905c 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -175,6 +175,7 @@ check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-xhci.c
 check-qtest-i386-y += tests/pc-cpu-test$(EXESUF)
 check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
 gcov-files-x86_64-y = $(subst i386-softmmu/,x86_64-softmmu/,$(gcov-files-i386-y))
@@ -369,6 +370,7 @@ tests/vhost-user-test$(EXESUF): tests/vhost-user-test.o qemu-char.o qemu-timer.o
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
 tests/test-write-threshold$(EXESUF): tests/test-write-threshold.o $(block-obj-y) libqemuutil.a libqemustub.a
+tests/vmgenid-test$(EXESUF): tests/vmgenid-test.o
 
 ifeq ($(CONFIG_POSIX),y)
 LIBS += -lutil
diff --git a/tests/vmgenid-test.c b/tests/vmgenid-test.c
new file mode 100644
index 0000000..abb7974
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,44 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2014 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.
+ */
+
+#include <string.h>
+#include "libqtest.h"
+
+static void vmgenid_test(void)
+{
+    static const uint8_t expected[16] = {
+        0x32, 0x4e, 0x6e, 0xaf, 0xd1, 0xd1, 0x4b, 0xf6,
+        0xbf, 0x41, 0xb9, 0xbb, 0x6c, 0x91, 0xfb, 0x87
+    };
+    uint8_t guid[16];
+    uint32_t i;
+
+    /* Skip the ACPI ADDR method and read the GUID directly from memory. */
+    for (i = 0; i < 16; i++) {
+        guid[i] = readb(0xfedf0000 + i);
+    }
+
+    g_assert_cmpuint(sizeof(guid), ==, sizeof(expected));
+    g_assert(memcmp(guid, expected, sizeof(guid)) == 0);
+}
+
+int main(int argc, char **argv)
+{
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+    qtest_add_func("/vmgenid/vmgenid", vmgenid_test);
+
+    qtest_start("-global vmgenid.uuid=324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87");
+    ret = g_test_run();
+
+    qtest_end();
+
+    return ret;
+}
-- 
2.1.0

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 02:19:50PM +0300, Gal Hammer wrote:
> Based on Microsoft's sepecifications (paper can be dowloaded from
> http://go.microsoft.com/fwlink/?LinkId=260709), add a device
> description to the SSDT ACPI table and its implementation.
> 
> The GUID is set using a global "vmgenid.uuid" parameter.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  default-configs/i386-softmmu.mak   |   1 +
>  default-configs/x86_64-softmmu.mak |   1 +
>  hw/i386/acpi-build.c               |  41 +++++++++++++
>  hw/i386/pc.c                       |   8 +++
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/vmgenid.c                  | 116 +++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h               |   3 +
>  include/hw/misc/vmgenid.h          |  21 +++++++
>  8 files changed, 192 insertions(+)
>  create mode 100644 hw/misc/vmgenid.c
>  create mode 100644 include/hw/misc/vmgenid.h
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 6a74e00..9fc0a3c 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 46b87dd..f66fd3c 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e761005..8d1a761 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> +#include "hw/misc/vmgenid.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
>      uint16_t applesmc_io_base;
> +    bool vm_generation_id_set;
>  } AcpiMiscInfo;
>  
>  typedef struct AcpiBuildPciBusHotplugState {
> @@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      info->has_tpm = tpm_find();
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
> +    info->vm_generation_id_set = vm_generation_id_set();
>  }
>  
>  static void acpi_get_pci_info(PcPciInfo *info)
> @@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(ssdt, scope);
>      }
>  
> +    if (misc->vm_generation_id_set) {
> +        scope = aml_scope("\\_SB");
> +
> +        dev = aml_device("VMGI");
> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +        aml_append(dev,
> +            aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
> +        aml_append(dev,
> +            aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
> +
> +        crs = aml_resource_template();
> +        aml_append(crs, aml_memory32_fixed(aml_ReadOnly,
> +            VMGENID_BASE_ADDRESS, VMGENID_BASE_ADDR_LEN));
> +        aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> +
> +        method = aml_method("ADDR", 0);
> +        pkg = aml_package(2);
> +        aml_append(pkg, aml_int(VMGENID_BASE_ADDRESS));
> +        aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr */

So VMGENID_BASE_ADDRESS >> 32 then, won't need a comment.

> +        aml_append(method, aml_store(pkg, aml_local(0)));
> +        aml_append(method, aml_return(aml_local(0)));
> +        aml_append(dev, method);
> +
> +        aml_append(scope, dev);
> +        aml_append(ssdt, scope);
> +
> +        scope = aml_scope("\\_GPE");
> +
> +        method = aml_method("_E00", 0);
> +        aml_append(method,
> +            aml_notify(aml_name("\\_SB.VMGI"), aml_int(0x80)));
> +
> +        aml_append(scope, method);
> +        aml_append(ssdt, scope);
> +    }
> +
>      sb_scope = aml_scope("_SB");
>      {
>          /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a8e6be1..266c5f3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/misc/vmgenid.h"
>  #include "trace.h"
>  #include "qapi/visitor.h"
>  #include "qapi-visit.h"
> @@ -1402,6 +1403,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>      int i;
>      DriveInfo *fd[MAX_FD];
>      DeviceState *hpet = NULL;
> +    DeviceState *vmgenid;
>      int pit_isa_irq = 0;
>      qemu_irq pit_alt_irq = NULL;
>      qemu_irq rtc_irq = NULL;
> @@ -1491,6 +1493,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>          fd[i] = drive_get(IF_FLOPPY, 0, i);
>      }
>      *floppy = fdctrl_init_isa(isa_bus, fd);
> +
> +    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
> +    if (vmgenid) {
> +        qdev_init_nofail(vmgenid);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
> +    }
>  }
>  
>  void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)

Please leave pc_basic_device_init alone.  This is legacy stuff, new
devices should be added with -device parameter.


> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 4aa76ff..3db11de 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -40,3 +40,4 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
>  
>  obj-$(CONFIG_PVPANIC) += pvpanic.o
>  obj-$(CONFIG_EDU) += edu.o
> +obj-$(CONFIG_VMGENID) += vmgenid.o
> diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
> new file mode 100644
> index 0000000..01aacd4
> --- /dev/null
> +++ b/hw/misc/vmgenid.c
> @@ -0,0 +1,116 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2014 Red Hat Inc.
> + *
> + *  Authors: Gal Hammer <ghammer@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hw/i386/pc.h"
> +#include "hw/sysbus.h"
> +#include "hw/misc/vmgenid.h"
> +#include "hw/acpi/acpi_dev_interface.h"
> +
> +#define PROPERTY_UUID "uuid"
> +
> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
> +
> +typedef struct VmGenIdState {
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +    uint8_t guid[16];
> +    bool guid_set;
> +} VmGenIdState;
> +
> +bool vm_generation_id_set(void)
> +{
> +    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    if (!obj) {
> +        return false;
> +    }
> +    return s->guid_set;
> +}
> +
> +static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr,
> +                                 unsigned size)
> +{
> +    VmGenIdState *s = VMGENID(opaque);
> +    uint64_t value;
> +
> +    memcpy(&value, s->guid + addr, size);
> +    return value;
> +}

I tried to think through what this does on BE platforms
and got a headache. It seems clear not all of value
is initialized here, though.

> +
> +static const MemoryRegionOps vmgenid_ram_ops = {
> +    .read = vmgenid_ram_read,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +

DEVICE_NATIVE_ENDIAN is generally evil, and best avoided.
I think you should make it a RAM region, not an IO region. Otherwise the
spec text that wants to enable caching for it seems useless as caching
has no effect on io regions.  Will also take care of migrating this,
which you currently don't.

> +static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
> +{
> +    VmGenIdState *s = VMGENID(obj);
> +    Object *acpi_obj;
> +    bool first_set = !s->guid_set;
> +
> +    if (qemu_uuid_parse(value, s->guid) < 0) {
> +        error_setg(errp, "Fail to parse UUID string.");
> +        return;
> +    }
> +    s->guid_set = true;
> +
> +    /* Skip the acpi notification when setting the vm generation id for the
> +     * first time. This is done because in a q35 machine the gpe register is
> +     * allocated after the device is initialized. */

/* Always
 * Like this
 */

/*
 * or
 * Like this
 */

/* never
 * Like this */


> +    if (!first_set) {
> +        acpi_obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
> +        if (acpi_obj) {
> +            AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(acpi_obj);
> +            AcpiDeviceIf *adev = ACPI_DEVICE_IF(acpi_obj);
> +
> +            adevc->vm_generation_id_changed(adev);
> +        }
> +    }
> +}
> +
> +static void vmgenid_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
> +}
> +
> +static void vmgenid_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo vmgenid_device_info = {
> +    .name          = VMGENID_DEVICE,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(VmGenIdState),
> +    .instance_init = vmgenid_init,
> +    .class_init    = vmgenid_class_init,
> +};
> +
> +static void vmgenid_register_types(void)
> +{
> +    type_register_static(&vmgenid_device_info);
> +}
> +
> +type_init(vmgenid_register_types)
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1b35168..9df3a9f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -284,6 +284,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory,
>  /* pvpanic.c */
>  uint16_t pvpanic_port(void);
>  
> +/* vmgenid.c */
> +bool vm_generation_id_set(void);
> +
>  /* e820 types */
>  #define E820_RAM        1
>  #define E820_RESERVED   2
> diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
> new file mode 100644
> index 0000000..3d44421
> --- /dev/null
> +++ b/include/hw/misc/vmgenid.h
> @@ -0,0 +1,21 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2014 Red Hat Inc.
> + *
> + *  Authors: Gal Hammer <ghammer@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef HW_MISC_VMGENID_H
> +#define HW_MISC_VMGENID_H
> +
> +#define VMGENID_DEVICE "vmgenid"
> +
> +#define VMGENID_BASE_ADDRESS    0xfedf0000

Please do not leave the reader to guess where
did this magic number come from.
This needs to be documented.

> +#define VMGENID_BASE_ADDR_LEN   16


Spec says this must not be in same 4K with uncacheable memory.
How do you guarantee this?

> +
> +#endif
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V15 1/5] docs: vm generation id device's description

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 02:19:47PM +0300, Gal Hammer wrote:
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  docs/specs/vmgenid.txt | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 docs/specs/vmgenid.txt
> 
> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
> new file mode 100644
> index 0000000..86ce6ab
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,35 @@
> +VIRTUAL MACHINE GENERATION ID
> +=============================
> +
> +Copyright (C) 2014 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 generation ID (vmgenid) device is an emulated device which
> +exposes a 128-bit, cryptographically random, integer value identifier.
> +This allows management applications (e.g. libvirt) to notify the guest
> +operating system when the virtual machine is executed with a different
> +configuration (e.g. snapshot execution or creation from a template).
> +
> +This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
> +
> +---
> +
> +The vmgenid device is a sysbus device with the following ACPI ID:
> +"QEMU0002".
> +
> +The device adds a "vmgenid.uuid" property, which can be modified using
> +the -global command line argument or the QMP interface.
> +
> +The device uses a fixed memory resource: 0xfedf0000-0xfedf000f to store
> +the GUID's buffer.
> +
> +According to the specification, any change to the GUID executes an
> +ACPI notification. The vmgenid device triggers the GPE._E00 which
> +executes the ACPI Notify operation.
> +
> +Although not specified in Microsoft's document, it is assumed that the
> +device is expected to use the little-endian system.

host? guest?
Please fix that. UUID is just a byte array, no need to
assume endian-ness anywhere.

> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V15 1/5] docs: vm generation id device's description

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 02:19:47PM +0300, Gal Hammer wrote:
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  docs/specs/vmgenid.txt | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 docs/specs/vmgenid.txt
> 
> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
> new file mode 100644
> index 0000000..86ce6ab
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,35 @@
> +VIRTUAL MACHINE GENERATION ID
> +=============================
> +
> +Copyright (C) 2014 Red Hat, Inc.

It's 2015, isn't it?

> +
> +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 generation ID (vmgenid) device is an emulated device which
> +exposes a 128-bit, cryptographically random, integer value identifier.

The value is up to user, let's not claim it's cryptographically random.

> +This allows management applications (e.g. libvirt) to notify the guest
> +operating system when the virtual machine is executed with a different
> +configuration (e.g. snapshot execution or creation from a template).
> +
> +This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
> +
> +---
> +
> +The vmgenid device is a sysbus device with the following ACPI ID:
> +"QEMU0002".

No one is expected to locate it using that ID, right?

> +
> +The device adds a "vmgenid.uuid" property, which can be modified using
> +the -global command line argument or the QMP interface.
> +
> +The device uses a fixed memory resource: 0xfedf0000-0xfedf000f to store
> +the GUID's buffer.
> +
> +According to the specification, any change to the GUID executes an
> +ACPI notification. The vmgenid device triggers the GPE._E00 which
> +executes the ACPI Notify operation.
> +
> +Although not specified in Microsoft's document, it is assumed that the
> +device is expected to use the little-endian system.
> -- 
> 2.1.0

Re: [Qemu-devel] [PATCH V15 1/5] docs: vm generation id device's description

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On 04/27/2015 05:19 AM, Gal Hammer wrote:
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  docs/specs/vmgenid.txt | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 docs/specs/vmgenid.txt
> 
> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
> new file mode 100644
> index 0000000..86ce6ab
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,35 @@
> +VIRTUAL MACHINE GENERATION ID
> +=============================
> +
> +Copyright (C) 2014 Red Hat, Inc.

Do you want to include 2015?

What you have is good.  But it might also be worth documenting how one
would set a new value via QMP (I assume it is a matter of finding the
right QOM path to the device, then using 'qom-set' on the property).
Can be done as a separate patch on top, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V15 2/5] acpi: add a vm_generation_id_changed method

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 841 bytes --]

On 04/27/2015 05:19 AM, Gal Hammer wrote:
> Add a new method to the AcpiDeviceIfClass interface. The new
> method send an ACPI notfication when the VM generation id is

s/send/sends/

> changed.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  hw/acpi/core.c                       | 8 ++++++++
>  hw/acpi/ich9.c                       | 8 ++++++++
>  hw/acpi/piix4.c                      | 8 ++++++++
>  hw/isa/lpc_ich9.c                    | 1 +
>  include/hw/acpi/acpi.h               | 2 ++
>  include/hw/acpi/acpi_dev_interface.h | 4 ++++
>  include/hw/acpi/ich9.h               | 2 ++
>  7 files changed, 33 insertions(+)

I'll leave the technical review to others more familiar with the code.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On 04/27/2015 05:19 AM, Gal Hammer wrote:
> Based on Microsoft's sepecifications (paper can be dowloaded from

s/sepecifications/specifications/
s/dowloaded/downloaded/

> http://go.microsoft.com/fwlink/?LinkId=260709), add a device
> description to the SSDT ACPI table and its implementation.
> 
> The GUID is set using a global "vmgenid.uuid" parameter.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---

> +++ b/hw/misc/vmgenid.c
> @@ -0,0 +1,116 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2014 Red Hat Inc.

Worth including 2015?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V15 5/5] tests: add a unit test for the vmgenid device.

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 781 bytes --]

On 04/27/2015 05:19 AM, Gal Hammer wrote:
> Signed-off-by: Gal Hammer <ghammer@redhat.com>

Subject line: Most commits do NOT end in a trailing '.'. It's less
obvious if there is a preference for starting commits with a capital
after the subject, but that seems to be the current prevailing trend.
So I might have done:

tests: Add a unit test for vmgenid device

or even:

vmgenid: Add a unit test

(the latter approach would mean grouping all of the series under a
single topic of vmgenid, instead of your approach of a different topic
per patch according to which part was being modified in support of
adding vmgenid. Either approach is fine by me)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH V15 5/5] tests: add a unit test for the vmgenid device.

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 09:01:36AM -0600, Eric Blake wrote:
> On 04/27/2015 05:19 AM, Gal Hammer wrote:
> > Signed-off-by: Gal Hammer <ghammer@redhat.com>
> 
> Subject line: Most commits do NOT end in a trailing '.'. It's less
> obvious if there is a preference for starting commits with a capital
> after the subject, but that seems to be the current prevailing trend.

I personally prefer all-lower-case subjects.
In particular upper case after : is just weird.

> So I might have done:
> 
> tests: Add a unit test for vmgenid device
> 
> or even:
> 
> vmgenid: Add a unit test
> 
> (the latter approach would mean grouping all of the series under a
> single topic of vmgenid, instead of your approach of a different topic
> per patch according to which part was being modified in support of
> adding vmgenid. Either approach is fine by me)
> 
> -- 
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
> 

Re: [Qemu-devel] [PATCH V15 1/5] docs: vm generation id device's description

[Gal Hammer]

On 27/04/2015 16:55, Michael S. Tsirkin wrote:
> On Mon, Apr 27, 2015 at 02:19:47PM +0300, Gal Hammer wrote:
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>   docs/specs/vmgenid.txt | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>   create mode 100644 docs/specs/vmgenid.txt
>>
>> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
>> new file mode 100644
>> index 0000000..86ce6ab
>> --- /dev/null
>> +++ b/docs/specs/vmgenid.txt
>> @@ -0,0 +1,35 @@
>> +VIRTUAL MACHINE GENERATION ID
>> +=============================
>> +
>> +Copyright (C) 2014 Red Hat, Inc.
>
> It's 2015, isn't it?
>
>> +
>> +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 generation ID (vmgenid) device is an emulated device which
>> +exposes a 128-bit, cryptographically random, integer value identifier.
>
> The value is up to user, let's not claim it's cryptographically random.

I'll add a sentence that clear this issue.

>
>> +This allows management applications (e.g. libvirt) to notify the guest
>> +operating system when the virtual machine is executed with a different
>> +configuration (e.g. snapshot execution or creation from a template).
>> +
>> +This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
>> +
>> +---
>> +
>> +The vmgenid device is a sysbus device with the following ACPI ID:
>> +"QEMU0002".
>
> No one is expected to locate it using that ID, right?

Nope. According to the specs the device should be identified through its 
_CID and/or _DDN names.

>> +
>> +The device adds a "vmgenid.uuid" property, which can be modified using
>> +the -global command line argument or the QMP interface.
>> +
>> +The device uses a fixed memory resource: 0xfedf0000-0xfedf000f to store
>> +the GUID's buffer.
>> +
>> +According to the specification, any change to the GUID executes an
>> +ACPI notification. The vmgenid device triggers the GPE._E00 which
>> +executes the ACPI Notify operation.
>> +
>> +Although not specified in Microsoft's document, it is assumed that the
>> +device is expected to use the little-endian system.
>> --
>> 2.1.0

Re: [Qemu-devel] [PATCH V15 1/5] docs: vm generation id device's description

[Gal Hammer]

On 27/04/2015 17:56, Eric Blake wrote:
> On 04/27/2015 05:19 AM, Gal Hammer wrote:
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>   docs/specs/vmgenid.txt | 35 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>   create mode 100644 docs/specs/vmgenid.txt
>>
>> diff --git a/docs/specs/vmgenid.txt b/docs/specs/vmgenid.txt
>> new file mode 100644
>> index 0000000..86ce6ab
>> --- /dev/null
>> +++ b/docs/specs/vmgenid.txt
>> @@ -0,0 +1,35 @@
>> +VIRTUAL MACHINE GENERATION ID
>> +=============================
>> +
>> +Copyright (C) 2014 Red Hat, Inc.
>
> Do you want to include 2015?

Done.

> What you have is good.  But it might also be worth documenting how one
> would set a new value via QMP (I assume it is a matter of finding the
> right QOM path to the device, then using 'qom-set' on the property).
> Can be done as a separate patch on top, so:

Good idea. I'll add it.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks! :-)

     Gal.

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 27/04/2015 16:38, Michael S. Tsirkin wrote:
> On Mon, Apr 27, 2015 at 02:19:50PM +0300, Gal Hammer wrote:
>> Based on Microsoft's sepecifications (paper can be dowloaded from
>> http://go.microsoft.com/fwlink/?LinkId=260709), add a device
>> description to the SSDT ACPI table and its implementation.
>>
>> The GUID is set using a global "vmgenid.uuid" parameter.
>>
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>   default-configs/i386-softmmu.mak   |   1 +
>>   default-configs/x86_64-softmmu.mak |   1 +
>>   hw/i386/acpi-build.c               |  41 +++++++++++++
>>   hw/i386/pc.c                       |   8 +++
>>   hw/misc/Makefile.objs              |   1 +
>>   hw/misc/vmgenid.c                  | 116 +++++++++++++++++++++++++++++++++++++
>>   include/hw/i386/pc.h               |   3 +
>>   include/hw/misc/vmgenid.h          |  21 +++++++
>>   8 files changed, 192 insertions(+)
>>   create mode 100644 hw/misc/vmgenid.c
>>   create mode 100644 include/hw/misc/vmgenid.h
>>
>> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
>> index 6a74e00..9fc0a3c 100644
>> --- a/default-configs/i386-softmmu.mak
>> +++ b/default-configs/i386-softmmu.mak
>> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>>   CONFIG_XIO3130=y
>>   CONFIG_IOH3420=y
>>   CONFIG_I82801B11=y
>> +CONFIG_VMGENID=y
>> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
>> index 46b87dd..f66fd3c 100644
>> --- a/default-configs/x86_64-softmmu.mak
>> +++ b/default-configs/x86_64-softmmu.mak
>> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>>   CONFIG_XIO3130=y
>>   CONFIG_IOH3420=y
>>   CONFIG_I82801B11=y
>> +CONFIG_VMGENID=y
>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
>> index e761005..8d1a761 100644
>> --- a/hw/i386/acpi-build.c
>> +++ b/hw/i386/acpi-build.c
>> @@ -42,6 +42,7 @@
>>   #include "hw/acpi/memory_hotplug.h"
>>   #include "sysemu/tpm.h"
>>   #include "hw/acpi/tpm.h"
>> +#include "hw/misc/vmgenid.h"
>>
>>   /* Supported chipsets: */
>>   #include "hw/acpi/piix4.h"
>> @@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
>>       unsigned dsdt_size;
>>       uint16_t pvpanic_port;
>>       uint16_t applesmc_io_base;
>> +    bool vm_generation_id_set;
>>   } AcpiMiscInfo;
>>
>>   typedef struct AcpiBuildPciBusHotplugState {
>> @@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>>       info->has_tpm = tpm_find();
>>       info->pvpanic_port = pvpanic_port();
>>       info->applesmc_io_base = applesmc_port();
>> +    info->vm_generation_id_set = vm_generation_id_set();
>>   }
>>
>>   static void acpi_get_pci_info(PcPciInfo *info)
>> @@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
>>           aml_append(ssdt, scope);
>>       }
>>
>> +    if (misc->vm_generation_id_set) {
>> +        scope = aml_scope("\\_SB");
>> +
>> +        dev = aml_device("VMGI");
>> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
>> +        aml_append(dev,
>> +            aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
>> +        aml_append(dev,
>> +            aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
>> +
>> +        crs = aml_resource_template();
>> +        aml_append(crs, aml_memory32_fixed(aml_ReadOnly,
>> +            VMGENID_BASE_ADDRESS, VMGENID_BASE_ADDR_LEN));
>> +        aml_append(dev, aml_name_decl("_CRS", crs));
>> +
>> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
>> +
>> +        method = aml_method("ADDR", 0);
>> +        pkg = aml_package(2);
>> +        aml_append(pkg, aml_int(VMGENID_BASE_ADDRESS));
>> +        aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr */
>
> So VMGENID_BASE_ADDRESS >> 32 then, won't need a comment.

Comments are not evil, you know. But it should be fixed because the 
addresses can be 64-bit in this solution.

>
>> +        aml_append(method, aml_store(pkg, aml_local(0)));
>> +        aml_append(method, aml_return(aml_local(0)));
>> +        aml_append(dev, method);
>> +
>> +        aml_append(scope, dev);
>> +        aml_append(ssdt, scope);
>> +
>> +        scope = aml_scope("\\_GPE");
>> +
>> +        method = aml_method("_E00", 0);
>> +        aml_append(method,
>> +            aml_notify(aml_name("\\_SB.VMGI"), aml_int(0x80)));
>> +
>> +        aml_append(scope, method);
>> +        aml_append(ssdt, scope);
>> +    }
>> +
>>       sb_scope = aml_scope("_SB");
>>       {
>>           /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO */
>> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
>> index a8e6be1..266c5f3 100644
>> --- a/hw/i386/pc.c
>> +++ b/hw/i386/pc.c
>> @@ -63,6 +63,7 @@
>>   #include "hw/pci/pci_host.h"
>>   #include "acpi-build.h"
>>   #include "hw/mem/pc-dimm.h"
>> +#include "hw/misc/vmgenid.h"
>>   #include "trace.h"
>>   #include "qapi/visitor.h"
>>   #include "qapi-visit.h"
>> @@ -1402,6 +1403,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>       int i;
>>       DriveInfo *fd[MAX_FD];
>>       DeviceState *hpet = NULL;
>> +    DeviceState *vmgenid;
>>       int pit_isa_irq = 0;
>>       qemu_irq pit_alt_irq = NULL;
>>       qemu_irq rtc_irq = NULL;
>> @@ -1491,6 +1493,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq *gsi,
>>           fd[i] = drive_get(IF_FLOPPY, 0, i);
>>       }
>>       *floppy = fdctrl_init_isa(isa_bus, fd);
>> +
>> +    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
>> +    if (vmgenid) {
>> +        qdev_init_nofail(vmgenid);
>> +        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
>> +    }
>>   }
>>
>>   void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)
>
> Please leave pc_basic_device_init alone.  This is legacy stuff, new
> devices should be added with -device parameter.

I think I did it because it is not possible to create a sysbus device 
when using the -device parameter.

>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index 4aa76ff..3db11de 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -40,3 +40,4 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.o
>>
>>   obj-$(CONFIG_PVPANIC) += pvpanic.o
>>   obj-$(CONFIG_EDU) += edu.o
>> +obj-$(CONFIG_VMGENID) += vmgenid.o
>> diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
>> new file mode 100644
>> index 0000000..01aacd4
>> --- /dev/null
>> +++ b/hw/misc/vmgenid.c
>> @@ -0,0 +1,116 @@
>> +/*
>> + *  Virtual Machine Generation ID Device
>> + *
>> + *  Copyright (C) 2014 Red Hat Inc.
>> + *
>> + *  Authors: Gal Hammer <ghammer@redhat.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "hw/i386/pc.h"
>> +#include "hw/sysbus.h"
>> +#include "hw/misc/vmgenid.h"
>> +#include "hw/acpi/acpi_dev_interface.h"
>> +
>> +#define PROPERTY_UUID "uuid"
>> +
>> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
>> +
>> +typedef struct VmGenIdState {
>> +    SysBusDevice parent_obj;
>> +    MemoryRegion iomem;
>> +    uint8_t guid[16];
>> +    bool guid_set;
>> +} VmGenIdState;
>> +
>> +bool vm_generation_id_set(void)
>> +{
>> +    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
>> +    VmGenIdState *s = VMGENID(obj);
>> +
>> +    if (!obj) {
>> +        return false;
>> +    }
>> +    return s->guid_set;
>> +}
>> +
>> +static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr,
>> +                                 unsigned size)
>> +{
>> +    VmGenIdState *s = VMGENID(opaque);
>> +    uint64_t value;
>> +
>> +    memcpy(&value, s->guid + addr, size);
>> +    return value;
>> +}
>
> I tried to think through what this does on BE platforms
> and got a headache. It seems clear not all of value
> is initialized here, though.

Your comment on the doc patch said that I should remove the reference to 
LE because UUID is just a buffer, so I don't understand this comment.

The code works in my tests so I assume there is no problem with the 
initialization of the value.

>> +
>> +static const MemoryRegionOps vmgenid_ram_ops = {
>> +    .read = vmgenid_ram_read,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>> +};
>> +
>
> DEVICE_NATIVE_ENDIAN is generally evil, and best avoided.

And your alternative is...?

> I think you should make it a RAM region, not an IO region. Otherwise the
> spec text that wants to enable caching for it seems useless as caching
> has no effect on io regions.  Will also take care of migrating this,
> which you currently don't.

Do you mean that I should use memory_region_init_ram?

>> +static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
>> +{
>> +    VmGenIdState *s = VMGENID(obj);
>> +    Object *acpi_obj;
>> +    bool first_set = !s->guid_set;
>> +
>> +    if (qemu_uuid_parse(value, s->guid) < 0) {
>> +        error_setg(errp, "Fail to parse UUID string.");
>> +        return;
>> +    }
>> +    s->guid_set = true;
>> +
>> +    /* Skip the acpi notification when setting the vm generation id for the
>> +     * first time. This is done because in a q35 machine the gpe register is
>> +     * allocated after the device is initialized. */
>
> /* Always
>   * Like this
>   */
>
> /*
>   * or
>   * Like this
>   */
>
> /* never
>   * Like this */
>
>
>> +    if (!first_set) {
>> +        acpi_obj = object_resolve_path_type("", TYPE_ACPI_DEVICE_IF, NULL);
>> +        if (acpi_obj) {
>> +            AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(acpi_obj);
>> +            AcpiDeviceIf *adev = ACPI_DEVICE_IF(acpi_obj);
>> +
>> +            adevc->vm_generation_id_changed(adev);
>> +        }
>> +    }
>> +}
>> +
>> +static void vmgenid_init(Object *obj)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    VmGenIdState *s = VMGENID(obj);
>> +
>> +    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
>> +}
>> +
>> +static void vmgenid_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
>> +}
>> +
>> +static const TypeInfo vmgenid_device_info = {
>> +    .name          = VMGENID_DEVICE,
>> +    .parent        = TYPE_SYS_BUS_DEVICE,
>> +    .instance_size = sizeof(VmGenIdState),
>> +    .instance_init = vmgenid_init,
>> +    .class_init    = vmgenid_class_init,
>> +};
>> +
>> +static void vmgenid_register_types(void)
>> +{
>> +    type_register_static(&vmgenid_device_info);
>> +}
>> +
>> +type_init(vmgenid_register_types)
>> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
>> index 1b35168..9df3a9f 100644
>> --- a/include/hw/i386/pc.h
>> +++ b/include/hw/i386/pc.h
>> @@ -284,6 +284,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory,
>>   /* pvpanic.c */
>>   uint16_t pvpanic_port(void);
>>
>> +/* vmgenid.c */
>> +bool vm_generation_id_set(void);
>> +
>>   /* e820 types */
>>   #define E820_RAM        1
>>   #define E820_RESERVED   2
>> diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
>> new file mode 100644
>> index 0000000..3d44421
>> --- /dev/null
>> +++ b/include/hw/misc/vmgenid.h
>> @@ -0,0 +1,21 @@
>> +/*
>> + *  Virtual Machine Generation ID Device
>> + *
>> + *  Copyright (C) 2014 Red Hat Inc.
>> + *
>> + *  Authors: Gal Hammer <ghammer@redhat.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef HW_MISC_VMGENID_H
>> +#define HW_MISC_VMGENID_H
>> +
>> +#define VMGENID_DEVICE "vmgenid"
>> +
>> +#define VMGENID_BASE_ADDRESS    0xfedf0000
>
> Please do not leave the reader to guess where
> did this magic number come from.
> This needs to be documented.

OK.

>
>> +#define VMGENID_BASE_ADDR_LEN   16
>
>
> Spec says this must not be in same 4K with uncacheable memory.
> How do you guarantee this?

I assume that if the device is using part of a page (and the current 
device's address is page-aligned) then the OS won't use that page. And 
an unused page is uncacheable.

>> +
>> +#endif
>> --
>> 2.1.0

     Gal.

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 27/04/2015 13:19, Gal Hammer wrote:
> +static void vmgenid_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);

This is a small problem.  The spec says the memory of the VMGENID should
not be uncacheable, but MMIO _should_ be uncacheable.

This is running on a VM so we maybe not care, but it's worth pointing it
out.

Paolo

> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
> +}
> +

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Thu, May 28, 2015 at 12:25:40PM +0200, Paolo Bonzini wrote:
> 
> 
> On 27/04/2015 13:19, Gal Hammer wrote:
> > +static void vmgenid_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    VmGenIdState *s = VMGENID(obj);
> > +
> > +    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
> 
> This is a small problem.  The spec says the memory of the VMGENID should
> not be uncacheable, but MMIO _should_ be uncacheable.
> 
> This is running on a VM so we maybe not care, but it's worth pointing it
> out.
> 
> Paolo

Exactly.

> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +
> > +    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
> > +}
> > +

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 28/05/2015 13:25, Paolo Bonzini wrote:
>
>
> On 27/04/2015 13:19, Gal Hammer wrote:
>> +static void vmgenid_init(Object *obj)
>> +{
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
>> +    VmGenIdState *s = VMGENID(obj);
>> +
>> +    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
>
> This is a small problem.  The spec says the memory of the VMGENID should
> not be uncacheable, but MMIO _should_ be uncacheable.
>
> This is running on a VM so we maybe not care, but it's worth pointing it
> out.

And the way I can fix it would be...? Is there another memory region I 
can use?

Thanks,

     Gal.

> Paolo
>
>> +    sysbus_init_mmio(sbd, &s->iomem);
>> +
>> +    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
>> +}
>> +

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Thu, May 28, 2015 at 02:59:09PM +0300, Gal Hammer wrote:
> On 28/05/2015 13:25, Paolo Bonzini wrote:
> >
> >
> >On 27/04/2015 13:19, Gal Hammer wrote:
> >>+static void vmgenid_init(Object *obj)
> >>+{
> >>+    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> >>+    VmGenIdState *s = VMGENID(obj);
> >>+
> >>+    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
> >
> >This is a small problem.  The spec says the memory of the VMGENID should
> >not be uncacheable, but MMIO _should_ be uncacheable.
> >
> >This is running on a VM so we maybe not care, but it's worth pointing it
> >out.
> 
> And the way I can fix it would be...? Is there another memory region I can
> use?
> 
> Thanks,
> 
>     Gal.

I think we hammered out an interesting alternative off-list: get hold of
some AddressRegionNVS memory and copy data from MMIO or port io over
there when you get an interrupt.

> >Paolo
> >
> >>+    sysbus_init_mmio(sbd, &s->iomem);
> >>+
> >>+    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, NULL);
> >>+}
> >>+

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 28/05/2015 14:21, Michael S. Tsirkin wrote:
> > And the way I can fix it would be...? Is there another memory region I can
> > use?
> > 
> > Thanks,
> > 
> >     Gal.
> 
> I think we hammered out an interesting alternative off-list: get hold of
> some AddressRegionNVS memory and copy data from MMIO or port io over
> there when you get an interrupt.

That doesn't work because there's no way to get the base address of a
DataTableRegion.

You could use a linker ADD_POINTER command, but it wouldn't work for
UEFI.  So I think it's best to just stay with this one.  If we do find
something better we can always change it later.

Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Thu, May 28, 2015 at 03:00:52PM +0200, Paolo Bonzini wrote:
> 
> 
> On 28/05/2015 14:21, Michael S. Tsirkin wrote:
> > > And the way I can fix it would be...? Is there another memory region I can
> > > use?
> > > 
> > > Thanks,
> > > 
> > >     Gal.
> > 
> > I think we hammered out an interesting alternative off-list: get hold of
> > some AddressRegionNVS memory and copy data from MMIO or port io over
> > there when you get an interrupt.
> 
> That doesn't work because there's no way to get the base address of a
> DataTableRegion.
> 
> You could use a linker ADD_POINTER command, but it wouldn't work for
> UEFI.

Hmm why not? UEFI runs these as well.

>  So I think it's best to just stay with this one.  If we do find
> something better we can always change it later.
> 
> Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 28/05/2015 15:24, Michael S. Tsirkin wrote:
> On Thu, May 28, 2015 at 03:00:52PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 28/05/2015 14:21, Michael S. Tsirkin wrote:
>>>> And the way I can fix it would be...? Is there another memory region I can
>>>> use?
>>>>
>>>> Thanks,
>>>>
>>>>     Gal.
>>>
>>> I think we hammered out an interesting alternative off-list: get hold of
>>> some AddressRegionNVS memory and copy data from MMIO or port io over
>>> there when you get an interrupt.
>>
>> That doesn't work because there's no way to get the base address of a
>> DataTableRegion.
>>
>> You could use a linker ADD_POINTER command, but it wouldn't work for
>> UEFI.
> 
> Hmm why not? UEFI runs these as well.

But it only uses them to find a pointer to the base of the tables.  The
actual relocation is done by the platform-independent UEFI libraries.

So you would have the following:

1) OVMF loads the loader script and resolves the pointers.  ADDR now
correctly points inside the table that has the VMGENID.

2) OVMF calls the AcpiProtocol->InstallAcpiTable, which _copies_ the
tables to memory that it allocates.  The type of memory is correct (NVS)
but the ADDR is now wrong because it points inside the old copy.

3) OVMF notices that the loader script has a pointer to inside the UEFI
table.  So it does not free the table, but this doesn't help: the
DataTableRegion points to the copy of the table, but ADDR points to the
original.  Whenever the GPE event runs, it writes the VMGENID into the
copy of the table, not into ADDR.

It's a pity, yes, but it's very hard to fix.

Paolo

>>  So I think it's best to just stay with this one.  If we do find
>> something better we can always change it later.
>>
>> Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Igor Mammedov]

On Mon, 27 Apr 2015 14:19:50 +0300
Gal Hammer <ghammer@redhat.com> wrote:

> Based on Microsoft's sepecifications (paper can be dowloaded from
> http://go.microsoft.com/fwlink/?LinkId=260709), add a device
> description to the SSDT ACPI table and its implementation.
> 
> The GUID is set using a global "vmgenid.uuid" parameter.
> 
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  default-configs/i386-softmmu.mak   |   1 +
>  default-configs/x86_64-softmmu.mak |   1 +
>  hw/i386/acpi-build.c               |  41 +++++++++++++
>  hw/i386/pc.c                       |   8 +++
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/vmgenid.c                  | 116 +++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h               |   3 +
>  include/hw/misc/vmgenid.h          |  21 +++++++
>  8 files changed, 192 insertions(+)
>  create mode 100644 hw/misc/vmgenid.c
>  create mode 100644 include/hw/misc/vmgenid.h
> 
> diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> index 6a74e00..9fc0a3c 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index 46b87dd..f66fd3c 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e761005..8d1a761 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> +#include "hw/misc/vmgenid.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
>      uint16_t applesmc_io_base;
> +    bool vm_generation_id_set;
>  } AcpiMiscInfo;
>  
>  typedef struct AcpiBuildPciBusHotplugState {
> @@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      info->has_tpm = tpm_find();
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
> +    info->vm_generation_id_set = vm_generation_id_set();
>  }
>  
>  static void acpi_get_pci_info(PcPciInfo *info)
> @@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(ssdt, scope);
>      }
>  
> +    if (misc->vm_generation_id_set) {
would be better to make it conditional depending on if vmgenid device is present

> +        scope = aml_scope("\\_SB");
> +
> +        dev = aml_device("VMGI");

[...]

> +
> +    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
> +    if (vmgenid) {
> +        qdev_init_nofail(vmgenid);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
> +    }
it should be possible not to create device if user haven't asked for it,
my guess is that you are using sysbus device as a convenience for mapping.

I'd suggest to use plain Device type instead, with it
vmgenid would be created when management asks for it using CLI:
  -device vmgenid,uuid=XXXX
and from mgmt side it would treat device as any other device using the same APIs.

Mapping device to board is also simple, for example see:

pc_machine_device_plug_cb()->pc_dimm_plug()->memory_region_add_subregion()

for PC machine pc_machine_device_plug_cb() callback is called for
every bus-less device during device_realize() time
   hw/core/qdev.c:hotplug_handler_plug(hotplug_ctrl, dev, &local_err);

you just need to add something like:
  pc_vmgenid_plug()
	addr = object_property_get_int(dev, VMGENID_BASE_ADDR_PROP, &local_err);
        VMGenIdDeviceClass vmgidc = CAST(dev)
	memory_region_add_subregion(&system_memory, addr, vmgidc->get_memory_region());

[...]


> +
> +#define VMGENID_BASE_ADDRESS    0xfedf0000
I'd make it a property, so that every target would be able to set
its own value when times come and ACPI part would automatically pick it up,
it also helps with test cases.

> +#define VMGENID_BASE_ADDR_LEN   16
> +
> +#endif

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Fri, May 29, 2015 at 01:46:44PM +0200, Igor Mammedov wrote:
> On Mon, 27 Apr 2015 14:19:50 +0300
> Gal Hammer <ghammer@redhat.com> wrote:
> 
> > Based on Microsoft's sepecifications (paper can be dowloaded from
> > http://go.microsoft.com/fwlink/?LinkId=260709), add a device
> > description to the SSDT ACPI table and its implementation.
> > 
> > The GUID is set using a global "vmgenid.uuid" parameter.
> > 
> > Signed-off-by: Gal Hammer <ghammer@redhat.com>
> > ---
> >  default-configs/i386-softmmu.mak   |   1 +
> >  default-configs/x86_64-softmmu.mak |   1 +
> >  hw/i386/acpi-build.c               |  41 +++++++++++++
> >  hw/i386/pc.c                       |   8 +++
> >  hw/misc/Makefile.objs              |   1 +
> >  hw/misc/vmgenid.c                  | 116 +++++++++++++++++++++++++++++++++++++
> >  include/hw/i386/pc.h               |   3 +
> >  include/hw/misc/vmgenid.h          |  21 +++++++
> >  8 files changed, 192 insertions(+)
> >  create mode 100644 hw/misc/vmgenid.c
> >  create mode 100644 include/hw/misc/vmgenid.h
> > 
> > diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
> > index 6a74e00..9fc0a3c 100644
> > --- a/default-configs/i386-softmmu.mak
> > +++ b/default-configs/i386-softmmu.mak
> > @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
> >  CONFIG_XIO3130=y
> >  CONFIG_IOH3420=y
> >  CONFIG_I82801B11=y
> > +CONFIG_VMGENID=y
> > diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> > index 46b87dd..f66fd3c 100644
> > --- a/default-configs/x86_64-softmmu.mak
> > +++ b/default-configs/x86_64-softmmu.mak
> > @@ -45,3 +45,4 @@ CONFIG_MEM_HOTPLUG=y
> >  CONFIG_XIO3130=y
> >  CONFIG_IOH3420=y
> >  CONFIG_I82801B11=y
> > +CONFIG_VMGENID=y
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index e761005..8d1a761 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -42,6 +42,7 @@
> >  #include "hw/acpi/memory_hotplug.h"
> >  #include "sysemu/tpm.h"
> >  #include "hw/acpi/tpm.h"
> > +#include "hw/misc/vmgenid.h"
> >  
> >  /* Supported chipsets: */
> >  #include "hw/acpi/piix4.h"
> > @@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
> >      unsigned dsdt_size;
> >      uint16_t pvpanic_port;
> >      uint16_t applesmc_io_base;
> > +    bool vm_generation_id_set;
> >  } AcpiMiscInfo;
> >  
> >  typedef struct AcpiBuildPciBusHotplugState {
> > @@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
> >      info->has_tpm = tpm_find();
> >      info->pvpanic_port = pvpanic_port();
> >      info->applesmc_io_base = applesmc_port();
> > +    info->vm_generation_id_set = vm_generation_id_set();
> >  }
> >  
> >  static void acpi_get_pci_info(PcPciInfo *info)
> > @@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
> >          aml_append(ssdt, scope);
> >      }
> >  
> > +    if (misc->vm_generation_id_set) {
> would be better to make it conditional depending on if vmgenid device is present
> 
> > +        scope = aml_scope("\\_SB");
> > +
> > +        dev = aml_device("VMGI");
> 
> [...]
> 
> > +
> > +    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
> > +    if (vmgenid) {
> > +        qdev_init_nofail(vmgenid);
> > +        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
> > +    }
> it should be possible not to create device if user haven't asked for it,
> my guess is that you are using sysbus device as a convenience for mapping.
> 
> I'd suggest to use plain Device type instead, with it
> vmgenid would be created when management asks for it using CLI:
>   -device vmgenid,uuid=XXXX
> and from mgmt side it would treat device as any other device using the same APIs.
> 
> Mapping device to board is also simple, for example see:
> 
> pc_machine_device_plug_cb()->pc_dimm_plug()->memory_region_add_subregion()
> 
> for PC machine pc_machine_device_plug_cb() callback is called for
> every bus-less device during device_realize() time
>    hw/core/qdev.c:hotplug_handler_plug(hotplug_ctrl, dev, &local_err);
> 
> you just need to add something like:
>   pc_vmgenid_plug()
> 	addr = object_property_get_int(dev, VMGENID_BASE_ADDR_PROP, &local_err);
>         VMGenIdDeviceClass vmgidc = CAST(dev)
> 	memory_region_add_subregion(&system_memory, addr, vmgidc->get_memory_region());
> 
> [...]
> 

Yes, I think it is best not to add devices by default, -device is safer.

> > +
> > +#define VMGENID_BASE_ADDRESS    0xfedf0000
> I'd make it a property, so that every target would be able to set
> its own value when times come and ACPI part would automatically pick it up,
> it also helps with test cases.

I think there's no need for this change as we plan
to modify this code down the road anyway.

> > +#define VMGENID_BASE_ADDR_LEN   16
> > +
> > +#endif

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

Since Michael said he'd merge this version, there's just one thing I
would change.  You can access the region only one byte at a time, and
let the memory core fix endianness here:

On 27/04/2015 13:19, Gal Hammer wrote:
> +static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr,
> +                                 unsigned size)
> +{
> +    VmGenIdState *s = VMGENID(opaque);
> +    uint64_t value;
> +
> +    memcpy(&value, s->guid + addr, size);

value = s->guid[addr];

> +    return value;
> +}
> +
> +static const MemoryRegionOps vmgenid_ram_ops = {
> +    .read = vmgenid_ram_read,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },

And instead of this .valid declaration, use this:

    .valid.min_access_size = 1,
    .valid.max_access_size = 4,
    .impl.min_access_size = 1,
    .impl.max_access_size = 1,


> +    .endianness = DEVICE_NATIVE_ENDIAN,

    .endianness = DEVICE_LITTLE_ENDIAN


> +};

Thanks,

Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 03/06/2015 19:37, Paolo Bonzini wrote:
>
> Since Michael said he'd merge this version, there's just one thing I
> would change.  You can access the region only one byte at a time, and
> let the memory core fix endianness here:

I've applied the change offered below. Thanks.

There are two unanswered questions:

1. You wrote that I should not use the memory_region_init_io() function. 
Should I use memory_region_init_ram() instead?

2. Is it possible to create a sysbus device using the "-device" command 
line argument? I vaguely recall that it is not possible to do it and 
that's the reason that I specifically add the device in the pc init.

Thanks,

     Gal.

> On 27/04/2015 13:19, Gal Hammer wrote:
>> +static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr,
>> +                                 unsigned size)
>> +{
>> +    VmGenIdState *s = VMGENID(opaque);
>> +    uint64_t value;
>> +
>> +    memcpy(&value, s->guid + addr, size);
>
> value = s->guid[addr];
>
>> +    return value;
>> +}
>> +
>> +static const MemoryRegionOps vmgenid_ram_ops = {
>> +    .read = vmgenid_ram_read,
>> +    .valid = {
>> +        .min_access_size = 4,
>> +        .max_access_size = 4,
>> +    },
>
> And instead of this .valid declaration, use this:
>
>      .valid.min_access_size = 1,
>      .valid.max_access_size = 4,
>      .impl.min_access_size = 1,
>      .impl.max_access_size = 1,
>
>
>> +    .endianness = DEVICE_NATIVE_ENDIAN,
>
>      .endianness = DEVICE_LITTLE_ENDIAN
>
>
>> +};
>
> Thanks,
>
> Paolo
>

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 08/06/2015 15:42, Gal Hammer wrote:
> On 03/06/2015 19:37, Paolo Bonzini wrote:
>>
>> Since Michael said he'd merge this version, there's just one thing I
>> would change.  You can access the region only one byte at a time, and
>> let the memory core fix endianness here:
> 
> I've applied the change offered below. Thanks.
> 
> There are two unanswered questions:
> 
> 1. You wrote that I should not use the memory_region_init_io() function.
> Should I use memory_region_init_ram() instead?

This is related to the uncached vs. cached memory discussion.  Michael
agreed that it's okay to do it this way.

> 2. Is it possible to create a sysbus device using the "-device" command
> line argument? I vaguely recall that it is not possible to do it and
> that's the reason that I specifically add the device in the pc init.

It's now possible, but it is somewhat complicated.  I think it's simpler
to initialize this unconditionally and hide it (via ACPI _STA) if the
vmgenid is all zeros.

Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 08/06/2015 16:43, Paolo Bonzini wrote:
>
>
> On 08/06/2015 15:42, Gal Hammer wrote:
>> On 03/06/2015 19:37, Paolo Bonzini wrote:
>>>
>>> Since Michael said he'd merge this version, there's just one thing I
>>> would change.  You can access the region only one byte at a time, and
>>> let the memory core fix endianness here:
>>
>> I've applied the change offered below. Thanks.
>>
>> There are two unanswered questions:
>>
>> 1. You wrote that I should not use the memory_region_init_io() function.
>> Should I use memory_region_init_ram() instead?
>
> This is related to the uncached vs. cached memory discussion.  Michael
> agreed that it's okay to do it this way.

OK. Nothing is changed for now.

>> 2. Is it possible to create a sysbus device using the "-device" command
>> line argument? I vaguely recall that it is not possible to do it and
>> that's the reason that I specifically add the device in the pc init.
>
> It's now possible, but it is somewhat complicated.  I think it's simpler
> to initialize this unconditionally and hide it (via ACPI _STA) if the
> vmgenid is all zeros.

I didn't understand. I need the device to be a sysbus device so it won't 
be found as an ISA or a PCI device by Windows. So I need to know what 
ever or not it is possible to create a sysbus device using "-device". In 
either way it won't be created if vmgenid is not given so no need to 
hide it using _STA.

     Gal.

> Paolo
>

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 08/06/2015 15:52, Gal Hammer wrote:
>>> 2. Is it possible to create a sysbus device using the "-device" command
>>> line argument? I vaguely recall that it is not possible to do it and
>>> that's the reason that I specifically add the device in the pc init.
>>
>> It's now possible, but it is somewhat complicated.  I think it's simpler
>> to initialize this unconditionally and hide it (via ACPI _STA) if the
>> vmgenid is all zeros.
> 
> I didn't understand. I need the device to be a sysbus device so it won't
> be found as an ISA or a PCI device by Windows. So I need to know what
> ever or not it is possible to create a sysbus device using "-device". In
> either way it won't be created if vmgenid is not given so no need to
> hide it using _STA.

Windows doesn't enumerate ISA devices when you create them with -device.
 It just enumerates devices from the ACPI DSDT/SSDT.  So it's okay to
make it an ISADevice, or to make it a part of another device (e.g. the
ISA bridge or the power management device).  It's still ugly though.

If you make it a sysbus device, you can just add it unconditionally, and
define _STA so that Windows only sees it under the appropriate
circumstances: for example, return 0 from _STA if the vmgenid (from the
command line) is all zeroes.

What is the command line option like?  Is it "-global vmgenid.uuid=foo"?

Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Daniel P. Berrange]

On Mon, Jun 08, 2015 at 03:55:14PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/06/2015 15:52, Gal Hammer wrote:
> >>> 2. Is it possible to create a sysbus device using the "-device" command
> >>> line argument? I vaguely recall that it is not possible to do it and
> >>> that's the reason that I specifically add the device in the pc init.
> >>
> >> It's now possible, but it is somewhat complicated.  I think it's simpler
> >> to initialize this unconditionally and hide it (via ACPI _STA) if the
> >> vmgenid is all zeros.
> > 
> > I didn't understand. I need the device to be a sysbus device so it won't
> > be found as an ISA or a PCI device by Windows. So I need to know what
> > ever or not it is possible to create a sysbus device using "-device". In
> > either way it won't be created if vmgenid is not given so no need to
> > hide it using _STA.
> 
> Windows doesn't enumerate ISA devices when you create them with -device.
>  It just enumerates devices from the ACPI DSDT/SSDT.  So it's okay to
> make it an ISADevice, or to make it a part of another device (e.g. the
> ISA bridge or the power management device).  It's still ugly though.
> 
> If you make it a sysbus device, you can just add it unconditionally, and
> define _STA so that Windows only sees it under the appropriate
> circumstances: for example, return 0 from _STA if the vmgenid (from the
> command line) is all zeroes.
> 
> What is the command line option like?  Is it "-global vmgenid.uuid=foo"?

FWIW, although the spec for this feature comes from Windows/Microsoft,
I'd expect that when we enable it in libvirt, we'll want to make it
unconditionally available to all VMs, since its a generically useful
information source for guest OS'.

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 08/06/2015 16:00, Gal Hammer wrote:
>> What is the command line option like?  Is it "-global vmgenid.uuid=foo"?
> 
> Yes. Right now it is a "-global" and I've been asked to change it to
> -"device", which I remember couldn't be done back then.

I don't think it's worth doing it now, either.

Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 08/06/2015 16:55, Paolo Bonzini wrote:
>
>
> On 08/06/2015 15:52, Gal Hammer wrote:
>>>> 2. Is it possible to create a sysbus device using the "-device" command
>>>> line argument? I vaguely recall that it is not possible to do it and
>>>> that's the reason that I specifically add the device in the pc init.
>>>
>>> It's now possible, but it is somewhat complicated.  I think it's simpler
>>> to initialize this unconditionally and hide it (via ACPI _STA) if the
>>> vmgenid is all zeros.
>>
>> I didn't understand. I need the device to be a sysbus device so it won't
>> be found as an ISA or a PCI device by Windows. So I need to know what
>> ever or not it is possible to create a sysbus device using "-device". In
>> either way it won't be created if vmgenid is not given so no need to
>> hide it using _STA.
>
> Windows doesn't enumerate ISA devices when you create them with -device.
>   It just enumerates devices from the ACPI DSDT/SSDT.  So it's okay to
> make it an ISADevice, or to make it a part of another device (e.g. the
> ISA bridge or the power management device).  It's still ugly though.
>
> If you make it a sysbus device, you can just add it unconditionally, and
> define _STA so that Windows only sees it under the appropriate
> circumstances: for example, return 0 from _STA if the vmgenid (from the
> command line) is all zeroes.
>
> What is the command line option like?  Is it "-global vmgenid.uuid=foo"?

Yes. Right now it is a "-global" and I've been asked to change it to 
-"device", which I remember couldn't be done back then.

     Gal.

> Paolo
>

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 08/06/2015 16:58, Daniel P. Berrange wrote:
> On Mon, Jun 08, 2015 at 03:55:14PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 08/06/2015 15:52, Gal Hammer wrote:
>>>>> 2. Is it possible to create a sysbus device using the "-device" command
>>>>> line argument? I vaguely recall that it is not possible to do it and
>>>>> that's the reason that I specifically add the device in the pc init.
>>>>
>>>> It's now possible, but it is somewhat complicated.  I think it's simpler
>>>> to initialize this unconditionally and hide it (via ACPI _STA) if the
>>>> vmgenid is all zeros.
>>>
>>> I didn't understand. I need the device to be a sysbus device so it won't
>>> be found as an ISA or a PCI device by Windows. So I need to know what
>>> ever or not it is possible to create a sysbus device using "-device". In
>>> either way it won't be created if vmgenid is not given so no need to
>>> hide it using _STA.
>>
>> Windows doesn't enumerate ISA devices when you create them with -device.
>>   It just enumerates devices from the ACPI DSDT/SSDT.  So it's okay to
>> make it an ISADevice, or to make it a part of another device (e.g. the
>> ISA bridge or the power management device).  It's still ugly though.
>>
>> If you make it a sysbus device, you can just add it unconditionally, and
>> define _STA so that Windows only sees it under the appropriate
>> circumstances: for example, return 0 from _STA if the vmgenid (from the
>> command line) is all zeroes.
>>
>> What is the command line option like?  Is it "-global vmgenid.uuid=foo"?
>
> FWIW, although the spec for this feature comes from Windows/Microsoft,
> I'd expect that when we enable it in libvirt, we'll want to make it
> unconditionally available to all VMs, since its a generically useful
> information source for guest OS'.

Once qemu part will be completed it will be possible to add a module to 
Linux as well (and maybe use it as a RNG generator source too).

     Gal.

> Regards,
> Daniel
>

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 02:58:54PM +0100, Daniel P. Berrange wrote:
> On Mon, Jun 08, 2015 at 03:55:14PM +0200, Paolo Bonzini wrote:
> > 
> > 
> > On 08/06/2015 15:52, Gal Hammer wrote:
> > >>> 2. Is it possible to create a sysbus device using the "-device" command
> > >>> line argument? I vaguely recall that it is not possible to do it and
> > >>> that's the reason that I specifically add the device in the pc init.
> > >>
> > >> It's now possible, but it is somewhat complicated.  I think it's simpler
> > >> to initialize this unconditionally and hide it (via ACPI _STA) if the
> > >> vmgenid is all zeros.
> > > 
> > > I didn't understand. I need the device to be a sysbus device so it won't
> > > be found as an ISA or a PCI device by Windows. So I need to know what
> > > ever or not it is possible to create a sysbus device using "-device". In
> > > either way it won't be created if vmgenid is not given so no need to
> > > hide it using _STA.
> > 
> > Windows doesn't enumerate ISA devices when you create them with -device.
> >  It just enumerates devices from the ACPI DSDT/SSDT.  So it's okay to
> > make it an ISADevice, or to make it a part of another device (e.g. the
> > ISA bridge or the power management device).  It's still ugly though.
> > 
> > If you make it a sysbus device, you can just add it unconditionally, and
> > define _STA so that Windows only sees it under the appropriate
> > circumstances: for example, return 0 from _STA if the vmgenid (from the
> > command line) is all zeroes.
> > 
> > What is the command line option like?  Is it "-global vmgenid.uuid=foo"?
> 
> FWIW, although the spec for this feature comes from Windows/Microsoft,
> I'd expect that when we enable it in libvirt, we'll want to make it
> unconditionally available to all VMs, since its a generically useful
> information source for guest OS'.
> 
> Regards,
> Daniel

Are there applications that would actually use this?  For microsoft this
seems to be mostly driven by ActiveDirectory needs.
It seems quite possible that applications solve the problem
differently on Linux.


> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 08/06/2015 17:01, Michael S. Tsirkin wrote:
> Are there applications that would actually use this?  For microsoft this
> seems to be mostly driven by ActiveDirectory needs.
> It seems quite possible that applications solve the problem
> differently on Linux.

I'm not aware of a different solution to the problem on Linux.

Paolo

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 05:17:31PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/06/2015 17:01, Michael S. Tsirkin wrote:
> > Are there applications that would actually use this?  For microsoft this
> > seems to be mostly driven by ActiveDirectory needs.
> > It seems quite possible that applications solve the problem
> > differently on Linux.
> 
> I'm not aware of a different solution to the problem on Linux.
> 
> Paolo

Adding it by default for everyone still seems too aggressive.
We had a ton of pain with pvpanic exactly because of a
similar "can't hurt" approach.  

I think a better approach would be to merge the code in qemu,
merge a linux driver. Once that's available, and some apps use it,
enabling it by default will seem more reasonable.

-- 
MST

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Gal Hammer]

On 08/06/2015 18:22, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2015 at 05:17:31PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 08/06/2015 17:01, Michael S. Tsirkin wrote:
>>> Are there applications that would actually use this?  For microsoft this
>>> seems to be mostly driven by ActiveDirectory needs.
>>> It seems quite possible that applications solve the problem
>>> differently on Linux.
>>
>> I'm not aware of a different solution to the problem on Linux.
>>
>> Paolo
>
> Adding it by default for everyone still seems too aggressive.
> We had a ton of pain with pvpanic exactly because of a
> similar "can't hurt" approach.
>
> I think a better approach would be to merge the code in qemu,
> merge a linux driver. Once that's available, and some apps use it,
> enabling it by default will seem more reasonable.
>

You should not enable it by default. It should be left for the 
management system to decide. QEmu by itself can't follow rules/spec on 
when to change the UUID.

     Gal.

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 06:28:23PM +0300, Gal Hammer wrote:
> On 08/06/2015 18:22, Michael S. Tsirkin wrote:
> >On Mon, Jun 08, 2015 at 05:17:31PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >>On 08/06/2015 17:01, Michael S. Tsirkin wrote:
> >>>Are there applications that would actually use this?  For microsoft this
> >>>seems to be mostly driven by ActiveDirectory needs.
> >>>It seems quite possible that applications solve the problem
> >>>differently on Linux.
> >>
> >>I'm not aware of a different solution to the problem on Linux.
> >>
> >>Paolo
> >
> >Adding it by default for everyone still seems too aggressive.
> >We had a ton of pain with pvpanic exactly because of a
> >similar "can't hurt" approach.
> >
> >I think a better approach would be to merge the code in qemu,
> >merge a linux driver. Once that's available, and some apps use it,
> >enabling it by default will seem more reasonable.
> >
> 
> You should not enable it by default. It should be left for the management
> system to decide. QEmu by itself can't follow rules/spec on when to change
> the UUID.
> 
>     Gal.

I agree. And IMHO at this early stage, neither should libvirt without
an explicit request.

-- 
MST

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 08/06/2015 17:22, Michael S. Tsirkin wrote:
> On Mon, Jun 08, 2015 at 05:17:31PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 08/06/2015 17:01, Michael S. Tsirkin wrote:
>>> Are there applications that would actually use this?  For microsoft this
>>> seems to be mostly driven by ActiveDirectory needs.
>>> It seems quite possible that applications solve the problem
>>> differently on Linux.
>>
>> I'm not aware of a different solution to the problem on Linux.
> 
> Adding it by default for everyone still seems too aggressive.
> We had a ton of pain with pvpanic exactly because of a
> similar "can't hurt" approach.  

The ton of pain was due to enabling it in ACPI unconditionally.  We can
leave it disabled in the UUID is all zeroes, for example.

Paolo

> I think a better approach would be to merge the code in qemu,
> merge a linux driver. Once that's available, and some apps use it,
> enabling it by default will seem more reasonable.
> 

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Mon, Jun 08, 2015 at 05:33:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/06/2015 17:22, Michael S. Tsirkin wrote:
> > On Mon, Jun 08, 2015 at 05:17:31PM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 08/06/2015 17:01, Michael S. Tsirkin wrote:
> >>> Are there applications that would actually use this?  For microsoft this
> >>> seems to be mostly driven by ActiveDirectory needs.
> >>> It seems quite possible that applications solve the problem
> >>> differently on Linux.
> >>
> >> I'm not aware of a different solution to the problem on Linux.
> > 
> > Adding it by default for everyone still seems too aggressive.
> > We had a ton of pain with pvpanic exactly because of a
> > similar "can't hurt" approach.  
> 
> The ton of pain was due to enabling it in ACPI unconditionally.  We can
> leave it disabled in the UUID is all zeroes, for example.
> 
> Paolo

Not if libvirt insists on adding a non zero uuid to all VMs
unconditionally. This is what I was arguing against really.

> > I think a better approach would be to merge the code in qemu,
> > merge a linux driver. Once that's available, and some apps use it,
> > enabling it by default will seem more reasonable.
> > 

Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device

[Paolo Bonzini]

On 08/06/2015 17:41, Michael S. Tsirkin wrote:
> > > Adding it by default for everyone still seems too aggressive.
> > > We had a ton of pain with pvpanic exactly because of a
> > > similar "can't hurt" approach.  
> > 
> > The ton of pain was due to enabling it in ACPI unconditionally.  We can
> > leave it disabled in the UUID is all zeroes, for example.
> 
> Not if libvirt insists on adding a non zero uuid to all VMs
> unconditionally. This is what I was arguing against really.

That would be a libvirt bug.  The VM UUID in SMBIOS and is different
from the vmgenid UUID.

Paolo