* [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID
@ 2015-03-03 16:18 Igor Mammedov
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description Igor Mammedov
` (3 more replies)
0 siblings, 4 replies; 24+ messages in thread
From: Igor Mammedov @ 2015-03-03 16:18 UTC (permalink / raw)
To: qemu-devel; +Cc: ghammer, mst
Changes since v13:
* fix comment style to /*... */ in testcase
* make BAR TARGET_PAGE_SIZE as required by spec
* make BAR prefetchable, spec also says that it shouldn't be
marked as non cached
* ACPI part
* merge separate VGID device with PCI device description
* mark device as not shown in UI,
it hides "VM Generation ID" device from device manager
and leaves only "PCI Standard RAM Controller" device there
Note to maintainer:
this patch set doesn't include the *.hex.generated files and
updated test ACPI tables blobs. Hex files for [q35-]acpi-dsdt
should be updated.
Tested with WS2012R2DCx64, thanks Gal for providing test utilities.
Based on top of mst's PCI tree.
Git branch:
https://github.com/imammedo/qemu.git vmgenid_v14
Gal Hammer (1):
docs: vm generation id device's description
Igor Mammedov (2):
pc: 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/pci-ids.txt | 1 +
docs/specs/vmgenid.txt | 36 ++++++++++
hw/i386/acpi-build.c | 69 +++++++++++++++++--
hw/i386/acpi-dsdt.dsl | 2 -
hw/i386/q35-acpi-dsdt.dsl | 2 -
hw/misc/Makefile.objs | 1 +
hw/misc/vmgenid.c | 134 +++++++++++++++++++++++++++++++++++++
include/hw/acpi/acpi.h | 1 +
include/hw/misc/vmgenid.h | 21 ++++++
include/hw/pci/pci.h | 1 +
tests/Makefile | 2 +
tests/vmgenid-test.c | 92 +++++++++++++++++++++++++
14 files changed, 356 insertions(+), 8 deletions(-)
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
^ permalink raw reply [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description
2015-03-03 16:18 [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Igor Mammedov
@ 2015-03-03 16:18 ` Igor Mammedov
2015-03-04 19:57 ` Eric Blake
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device Igor Mammedov
` (2 subsequent siblings)
3 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-03-03 16:18 UTC (permalink / raw)
To: qemu-devel; +Cc: ghammer, mst
From: Gal Hammer <ghammer@redhat.com>
Signed-off-by: Gal Hammer <ghammer@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
- ammend doc since no more MMIO occupied by vmgenid
device and device became PCIDevice
---
docs/specs/vmgenid.txt | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 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..718850a
--- /dev/null
+++ b/docs/specs/vmgenid.txt
@@ -0,0 +1,36 @@
+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 PCI device with the following ACPI ID: "QEMU0003".
+
+The device has a "vmgenid.uuid" property, which can be set using
+the command line argument or the QMP interface.
+For example:
+QEMU -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+
+Or to change uuid in runtime use:
+ qom-set "/machine/peripheral/FOO.uuid" "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+
+According to the specification, any change to the GUID executes an
+ACPI notification. The vmgenid device triggers the \_GPE._E00 handler
+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
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-03 16:18 [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Igor Mammedov
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description Igor Mammedov
@ 2015-03-03 16:18 ` Igor Mammedov
2015-03-03 17:35 ` Michael S. Tsirkin
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 3/3] tests: add a unit test for the vmgenid device Igor Mammedov
2015-04-15 10:38 ` [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Michael S. Tsirkin
3 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-03-03 16:18 UTC (permalink / raw)
To: qemu-devel; +Cc: ghammer, mst
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 "vmgenid.uuid" property.
Example of using vmgenid device:
-device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
To change uuid in runtime use:
qom-set "/machine/peripheral/FOO.uuid" "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
'vmgenid' device initialization flow is as following:
1. vmgenid has RAM BAR resistered with size of UUID buffer
2. BIOS initializes PCI devices and it maps BAR in PCI hole
3. BIOS reads ACPI tables from QEMU, at that moment tables
are generated with \_SB.VMGI.ADDR constant pointing to
HPA where BIOS's mapped vmgenid's BAR earlier
Signed-off-by: Gal Hammer <ghammer@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
* make BAR TARGET_PAGE_SIZE as required by spec
* make BAR prefetchable, spec also says that it shouldn't be
marked as non cached
* ACPI
* merge separate VGID device with PCI device description
* mark device as not shown in UI,
it hides "VM Generation ID" device from device manager
and leaves only "PCI Standard RAM Controller" device there
v2:
* rewrite to use PCIDevice so that we don't have to mess
with complex fwcfg/linker and patch ACPI tables then
read VMGENID buffer adddress in guest OSPM and communicate
it to QEMU via reserved MMIO region.
Which also allows us to write a more complete unit test
that wouldn't require to run OSPM so that it could update
HPA in QEMU.
* make 'vmgenid' optional, users who want to use it
should add -device vmgenid,.... to QEMU CLI
it also saves us some space in SSDT if device is not used
* mark UUID buffer as dirty when it's updated via QMP in runtime
* make 'uiid' property mandatory at -device
---
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
docs/specs/pci-ids.txt | 1 +
hw/i386/acpi-build.c | 69 +++++++++++++++++--
hw/i386/acpi-dsdt.dsl | 2 -
hw/i386/q35-acpi-dsdt.dsl | 2 -
hw/misc/Makefile.objs | 1 +
hw/misc/vmgenid.c | 134 +++++++++++++++++++++++++++++++++++++
include/hw/acpi/acpi.h | 1 +
include/hw/misc/vmgenid.h | 21 ++++++
include/hw/pci/pci.h | 1 +
11 files changed, 226 insertions(+), 8 deletions(-)
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 bd99af9..0b913a8 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -43,3 +43,4 @@ CONFIG_IOAPIC=y
CONFIG_ICC_BUS=y
CONFIG_PVPANIC=y
CONFIG_MEM_HOTPLUG=y
+CONFIG_VMGENID=y
diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
index e7c2734..de5e6af 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -43,3 +43,4 @@ CONFIG_IOAPIC=y
CONFIG_ICC_BUS=y
CONFIG_PVPANIC=y
CONFIG_MEM_HOTPLUG=y
+CONFIG_VMGENID=y
diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
index c6732fe..b398c5d 100644
--- a/docs/specs/pci-ids.txt
+++ b/docs/specs/pci-ids.txt
@@ -46,6 +46,7 @@ PCI devices (other than virtio):
1b36:0004 PCI Quad-port 16550A adapter (docs/specs/pci-serial.txt)
1b36:0005 PCI test device (docs/specs/pci-testdev.txt)
1b36:0007 PCI SD Card Host Controller Interface (SDHCI)
+1b36:0009 PCI VM-Generation device
All these devices are documented in docs/specs.
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b94e47e..9a82c7a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -50,6 +50,7 @@
#include "hw/pci/pci_bus.h"
#include "hw/pci-host/q35.h"
#include "hw/i386/intel_iommu.h"
+#include "hw/misc/vmgenid.h"
#include "hw/i386/q35-acpi-dsdt.hex"
#include "hw/i386/acpi-dsdt.hex"
@@ -110,6 +111,7 @@ typedef struct AcpiPmInfo {
} AcpiPmInfo;
typedef struct AcpiMiscInfo {
+ uint32_t vmgen_buf_paddr;
bool has_hpet;
bool has_tpm;
const unsigned char *dsdt_code;
@@ -238,6 +240,14 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
static void acpi_get_misc_info(AcpiMiscInfo *info)
{
+ Object *obj;
+
+ obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
+ info->vmgen_buf_paddr = 0;
+ if (obj) {
+ info->vmgen_buf_paddr =
+ object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
+ }
info->has_hpet = hpet_find();
info->has_tpm = tpm_find();
info->pvpanic_port = pvpanic_port();
@@ -521,8 +531,27 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot)
aml_append(method, if_ctx);
}
+static char *acpi_get_pci_dev_scope_name(PCIDevice *pdev)
+{
+ char *name = NULL;
+ char *last = name;
+ PCIBus *bus = pdev->bus;
+
+ while (bus->parent_dev) {
+ last = name;
+ name = g_strdup_printf("%s.S%.02X_", name ? name : "",
+ bus->parent_dev->devfn);
+ g_free(last);
+ bus = bus->parent_dev->bus;
+ }
+ last = name;
+ name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "", pdev->devfn);
+ g_free(last);
+ return name;
+}
static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
- bool pcihp_bridge_en)
+ bool pcihp_bridge_en,
+ AcpiMiscInfo *misc)
{
Aml *dev, *notify_method, *method;
QObject *bsel;
@@ -583,7 +612,21 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
- if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
+ if (pc->device_id == PCI_DEVICE_ID_REDHAT_VMGENID) {
+ Aml *pkg;
+
+ aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0003")));
+ aml_append(dev,
+ aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
+ aml_append(dev,
+ aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
+ aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
+
+ pkg = aml_package(2);
+ aml_append(pkg, aml_int(misc->vmgen_buf_paddr));
+ aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr */
+ aml_append(dev, aml_name_decl("ADDR", pkg));
+ } else if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
/* add VGA specific AML methods */
int s3d;
@@ -624,7 +667,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
*/
PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
- build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
+ build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en, misc);
}
/* slot descriptor has been composed, add it into parent context */
aml_append(parent_scope, dev);
@@ -1013,11 +1056,29 @@ build_ssdt(GArray *table_data, GArray *linker,
if (bus) {
Aml *scope = aml_scope("PCI0");
/* Scan all PCI buses. Generate tables to support hotplug. */
- build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
+ build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en,
+ misc);
aml_append(sb_scope, scope);
}
}
aml_append(ssdt, sb_scope);
+
+ if (misc->vmgen_buf_paddr) {
+ Object *obj;
+ char *vgid_path;
+
+ scope = aml_scope("\\_GPE");
+ method = aml_method("_E00", 0);
+
+ obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
+ vgid_path = acpi_get_pci_dev_scope_name(PCI_DEVICE(obj));
+ aml_append(method,
+ aml_notify(aml_name("\\_SB.%s", vgid_path), aml_int(0x80)));
+ g_free(vgid_path);
+
+ aml_append(scope, method);
+ aml_append(ssdt, scope);
+ }
}
/* copy AML table into ACPI tables blob and patch header there */
diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
index a2d84ec..e2e089f 100644
--- a/hw/i386/acpi-dsdt.dsl
+++ b/hw/i386/acpi-dsdt.dsl
@@ -260,8 +260,6 @@ DefinitionBlock (
Scope(\_GPE) {
Name(_HID, "ACPI0006")
- Method(_L00) {
- }
Method(_E01) {
// PCI hotplug event
Acquire(\_SB.PCI0.BLCK, 0xFFFF)
diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
index 16eaca3..8d031c0 100644
--- a/hw/i386/q35-acpi-dsdt.dsl
+++ b/hw/i386/q35-acpi-dsdt.dsl
@@ -395,8 +395,6 @@ DefinitionBlock (
Scope(\_GPE) {
Name(_HID, "ACPI0006")
- Method(_L00) {
- }
Method(_L01) {
}
Method(_E02) {
diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
index 029a56f..e047aea 100644
--- a/hw/misc/Makefile.objs
+++ b/hw/misc/Makefile.objs
@@ -41,3 +41,4 @@ obj-$(CONFIG_ZYNQ) += zynq_slcr.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..c47fb06
--- /dev/null
+++ b/hw/misc/vmgenid.c
@@ -0,0 +1,134 @@
+/*
+ * Virtual Machine Generation ID Device
+ *
+ * Copyright (C) 2014 Red Hat Inc.
+ *
+ * Authors: Gal Hammer <ghammer@redhat.com>
+ * Igor Mammedov <imammedo@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/pci/pci.h"
+#include "hw/misc/vmgenid.h"
+#include "hw/acpi/acpi.h"
+#include "qapi/visitor.h"
+
+#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
+
+typedef struct VmGenIdState {
+ PCIDevice parent_obj;
+ MemoryRegion iomem;
+ union {
+ uint8_t guid[16];
+ uint8_t guid_page[TARGET_PAGE_SIZE];
+ };
+ bool guid_set;
+} VmGenIdState;
+
+static void vmgenid_update_guest(VmGenIdState *s)
+{
+ Object *acpi_obj;
+ void *ptr = memory_region_get_ram_ptr(&s->iomem);
+
+ memcpy(ptr, &s->guid, sizeof(s->guid));
+ memory_region_set_dirty(&s->iomem, 0, sizeof(s->guid));
+
+ 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);
+ ACPIREGS *acpi_regs = adevc->regs(adev);
+
+ acpi_regs->gpe.sts[0] |= 1; /* _GPE.E00 handler */
+ acpi_update_sci(acpi_regs, adevc->sci(adev));
+ }
+}
+
+static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
+{
+ VmGenIdState *s = VMGENID(obj);
+
+ if (qemu_uuid_parse(value, s->guid) < 0) {
+ error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse UUID string.",
+ object_get_typename(OBJECT(s)));
+ return;
+ }
+
+ s->guid_set = true;
+ vmgenid_update_guest(s);
+}
+
+static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void *opaque,
+ const char *name, Error **errp)
+{
+ VmGenIdState *s = VMGENID(obj);
+ int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
+
+ if (value == PCI_BAR_UNMAPPED) {
+ error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not initialized",
+ object_get_typename(OBJECT(s)));
+ return;
+ }
+ visit_type_int(v, &value, name, errp);
+}
+
+static void vmgenid_initfn(Object *obj)
+{
+ VmGenIdState *s = VMGENID(obj);
+
+ memory_region_init_ram(&s->iomem, obj, "vgid.bar", sizeof(s->guid_page),
+ &error_abort);
+
+ object_property_add_str(obj, VMGENID_UUID, NULL, vmgenid_set_uuid, NULL);
+ object_property_add(obj, VMGENID_VMGID_ADDR, "int", vmgenid_get_vmgid_addr,
+ NULL, NULL, NULL, NULL);
+}
+
+
+static void vmgenid_realize(PCIDevice *dev, Error **errp)
+{
+ VmGenIdState *s = VMGENID(dev);
+
+ if (!s->guid_set) {
+ error_setg(errp, "'%s." VMGENID_UUID "' property is not set",
+ object_get_typename(OBJECT(s)));
+ return;
+ }
+
+ vmstate_register_ram(&s->iomem, DEVICE(s));
+ pci_register_bar(PCI_DEVICE(s), 0, PCI_BASE_ADDRESS_MEM_PREFETCH,
+ &s->iomem);
+ return;
+}
+
+static void vmgenid_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+ PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+ dc->hotpluggable = false;
+ k->realize = vmgenid_realize;
+ k->vendor_id = PCI_VENDOR_ID_REDHAT;
+ k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
+ k->class_id = PCI_CLASS_MEMORY_RAM;
+}
+
+static const TypeInfo vmgenid_device_info = {
+ .name = VMGENID_DEVICE,
+ .parent = TYPE_PCI_DEVICE,
+ .instance_size = sizeof(VmGenIdState),
+ .instance_init = vmgenid_initfn,
+ .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/acpi/acpi.h b/include/hw/acpi/acpi.h
index 1f678b4..a09cb3f 100644
--- a/include/hw/acpi/acpi.h
+++ b/include/hw/acpi/acpi.h
@@ -25,6 +25,7 @@
#include "qemu/option.h"
#include "exec/memory.h"
#include "hw/irq.h"
+#include "hw/acpi/acpi_dev_interface.h"
/*
* current device naming scheme supports up to 256 memory devices
diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
new file mode 100644
index 0000000..325f095
--- /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>
+ * Igor Mammedov <imammedo@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_UUID "uuid"
+#define VMGENID_VMGID_ADDR "vmgid-addr"
+
+#endif
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 3164fc3..245171b 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -90,6 +90,7 @@
#define PCI_DEVICE_ID_REDHAT_TEST 0x0005
#define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007
#define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008
+#define PCI_DEVICE_ID_REDHAT_VMGENID 0x0009
#define PCI_DEVICE_ID_REDHAT_QXL 0x0100
#define FMT_PCIBUS PRIx64
--
2.1.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [Qemu-devel] [PATCH V14 3/3] tests: add a unit test for the vmgenid device.
2015-03-03 16:18 [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Igor Mammedov
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description Igor Mammedov
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device Igor Mammedov
@ 2015-03-03 16:18 ` Igor Mammedov
2015-04-15 10:38 ` [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Michael S. Tsirkin
3 siblings, 0 replies; 24+ messages in thread
From: Igor Mammedov @ 2015-03-03 16:18 UTC (permalink / raw)
To: qemu-devel; +Cc: ghammer, mst
* test that guest can read UUID provided on CLI from buffer
accessing it at HPA which is available via 'vmgid-addr'
property when device is inintialized.
* test setting UUID at runtime and check that it's updated
at expected HPA.
Signed-off-by: Gal Hammer <ghammer@redhat.com>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
* fix comment style to /*... */
v2:
* fix: make sure that test actually runs
* use property to get UUID buffer HPA
* add setting UUID via QMP at runtime test case
---
tests/Makefile | 2 ++
tests/vmgenid-test.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 94 insertions(+)
create mode 100644 tests/vmgenid-test.c
diff --git a/tests/Makefile b/tests/Makefile
index d5df168..ccd0eb5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -171,6 +171,7 @@ gcov-files-i386-y += hw/usb/dev-hid.c
gcov-files-i386-y += hw/usb/dev-storage.c
check-qtest-i386-y += tests/usb-hcd-xhci-test$(EXESUF)
gcov-files-i386-y += hw/usb/hcd-xhci.c
+check-qtest-i386-y += tests/vmgenid-test$(EXESUF)
check-qtest-i386-$(CONFIG_LINUX) += tests/vhost-user-test$(EXESUF)
check-qtest-x86_64-y = $(check-qtest-i386-y)
gcov-files-i386-y += i386-softmmu/hw/timer/mc146818rtc.c
@@ -363,6 +364,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..628e1ec
--- /dev/null
+++ b/tests/vmgenid-test.c
@@ -0,0 +1,92 @@
+/*
+ * QTest testcase for VM Generation ID
+ *
+ * Copyright (c) 2015 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 <glib.h>
+#include <string.h>
+#include <unistd.h>
+#include "libqtest.h"
+
+/* Wait at most 1 minute */
+#define TEST_DELAY (1 * G_USEC_PER_SEC / 10)
+#define TEST_CYCLES MAX((60 * G_USEC_PER_SEC / TEST_DELAY), 1)
+
+#define VGID_UUID "324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
+
+static void vmgenid_check_uuid(const uint8_t *expected)
+{
+ uint8_t guid[16];
+ int i;
+ uint32_t addr = 0;
+ QDict *response;
+
+ for (i = 0; i < TEST_CYCLES; ++i) {
+ response = qmp("{ 'execute': 'qom-get', 'arguments': { "
+ "'path': '/machine/peripheral/testvgid', "
+ "'property': 'vmgid-addr' } }");
+ if (qdict_haskey(response, "return")) {
+ addr = qdict_get_int(response, "return");
+ }
+ QDECREF(response);
+ if (addr) {
+ break;
+ }
+ g_usleep(TEST_DELAY);
+ }
+
+ /* Skip the ACPI ADDR method and read the GUID directly from memory */
+ for (i = 0; i < 16; i++) {
+ guid[i] = readb(addr + i);
+ }
+
+ g_assert(memcmp(guid, expected, sizeof(guid)) == 0);
+}
+
+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
+ };
+ vmgenid_check_uuid(expected);
+}
+
+static void vmgenid_set_uuid_test(void)
+{
+ QDict *response;
+ static const uint8_t expected[16] = {
+ 0x12, 0x4e, 0x6e, 0xaf, 0xd1, 0xd1, 0x4b, 0xf6,
+ 0xbf, 0x41, 0xb9, 0xbb, 0x6c, 0x91, 0xfb, 0x87
+ };
+
+ response = qmp("{ 'execute': 'qom-set', 'arguments': { "
+ "'path': '/machine/peripheral/testvgid', "
+ "'property': 'uuid', 'value': '"
+ "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87' } }");
+ g_assert(qdict_haskey(response, "return"));
+ QDECREF(response);
+
+ vmgenid_check_uuid(expected);
+}
+
+int main(int argc, char **argv)
+{
+ int ret;
+
+ g_test_init(&argc, &argv, NULL);
+
+ qtest_start("-machine accel=tcg -device vmgenid,id=testvgid,"
+ "uuid=" VGID_UUID);
+ qtest_add_func("/vmgenid/vmgenid", vmgenid_test);
+ qtest_add_func("/vmgenid/vmgenid/set-uuid", vmgenid_set_uuid_test);
+ ret = g_test_run();
+
+ qtest_end();
+
+ return ret;
+}
--
2.1.0
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device Igor Mammedov
@ 2015-03-03 17:35 ` Michael S. Tsirkin
2015-03-03 20:33 ` Igor Mammedov
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-03-03 17:35 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, qemu-devel
On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
>
> Example of using vmgenid device:
> -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
So this is not a great example since we are burning up
a pci slot. Management can work around this by making this
a multifunction device and making this part of chipset,
but how about we make this a default, by specifying
appropriate addr and multifunction properties
as part of machine type.
Also, how are we going to extend this device?
looks like we've burned it all just for vmid?
How about we have a slightly more generic container
where we'll be able to stick all kind of stuff
in the future, and make vmgenid a child of
this device?
> To change uuid in runtime use:
> qom-set "/machine/peripheral/FOO.uuid" "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
Looking just at this, how does user discover this functionality?
>
> 'vmgenid' device initialization flow is as following:
> 1. vmgenid has RAM BAR resistered with size of UUID buffer
> 2. BIOS initializes PCI devices and it maps BAR in PCI hole
> 3. BIOS reads ACPI tables from QEMU, at that moment tables
> are generated with \_SB.VMGI.ADDR constant pointing to
> HPA where BIOS's mapped vmgenid's BAR earlier
>
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
> * make BAR TARGET_PAGE_SIZE as required by spec
> * make BAR prefetchable, spec also says that it shouldn't be
> marked as non cached
> * ACPI
> * merge separate VGID device with PCI device description
> * mark device as not shown in UI,
> it hides "VM Generation ID" device from device manager
> and leaves only "PCI Standard RAM Controller" device there
>
> v2:
> * rewrite to use PCIDevice so that we don't have to mess
> with complex fwcfg/linker and patch ACPI tables then
> read VMGENID buffer adddress in guest OSPM and communicate
> it to QEMU via reserved MMIO region.
> Which also allows us to write a more complete unit test
> that wouldn't require to run OSPM so that it could update
> HPA in QEMU.
> * make 'vmgenid' optional, users who want to use it
> should add -device vmgenid,.... to QEMU CLI
> it also saves us some space in SSDT if device is not used
> * mark UUID buffer as dirty when it's updated via QMP in runtime
> * make 'uiid' property mandatory at -device
> ---
> default-configs/i386-softmmu.mak | 1 +
> default-configs/x86_64-softmmu.mak | 1 +
> docs/specs/pci-ids.txt | 1 +
> hw/i386/acpi-build.c | 69 +++++++++++++++++--
> hw/i386/acpi-dsdt.dsl | 2 -
> hw/i386/q35-acpi-dsdt.dsl | 2 -
> hw/misc/Makefile.objs | 1 +
> hw/misc/vmgenid.c | 134 +++++++++++++++++++++++++++++++++++++
> include/hw/acpi/acpi.h | 1 +
> include/hw/misc/vmgenid.h | 21 ++++++
> include/hw/pci/pci.h | 1 +
> 11 files changed, 226 insertions(+), 8 deletions(-)
> 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 bd99af9..0b913a8 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -43,3 +43,4 @@ CONFIG_IOAPIC=y
> CONFIG_ICC_BUS=y
> CONFIG_PVPANIC=y
> CONFIG_MEM_HOTPLUG=y
> +CONFIG_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak b/default-configs/x86_64-softmmu.mak
> index e7c2734..de5e6af 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -43,3 +43,4 @@ CONFIG_IOAPIC=y
> CONFIG_ICC_BUS=y
> CONFIG_PVPANIC=y
> CONFIG_MEM_HOTPLUG=y
> +CONFIG_VMGENID=y
> diff --git a/docs/specs/pci-ids.txt b/docs/specs/pci-ids.txt
> index c6732fe..b398c5d 100644
> --- a/docs/specs/pci-ids.txt
> +++ b/docs/specs/pci-ids.txt
> @@ -46,6 +46,7 @@ PCI devices (other than virtio):
> 1b36:0004 PCI Quad-port 16550A adapter (docs/specs/pci-serial.txt)
> 1b36:0005 PCI test device (docs/specs/pci-testdev.txt)
> 1b36:0007 PCI SD Card Host Controller Interface (SDHCI)
> +1b36:0009 PCI VM-Generation device
>
> All these devices are documented in docs/specs.
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b94e47e..9a82c7a 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -50,6 +50,7 @@
> #include "hw/pci/pci_bus.h"
> #include "hw/pci-host/q35.h"
> #include "hw/i386/intel_iommu.h"
> +#include "hw/misc/vmgenid.h"
>
> #include "hw/i386/q35-acpi-dsdt.hex"
> #include "hw/i386/acpi-dsdt.hex"
> @@ -110,6 +111,7 @@ typedef struct AcpiPmInfo {
> } AcpiPmInfo;
>
> typedef struct AcpiMiscInfo {
> + uint32_t vmgen_buf_paddr;
> bool has_hpet;
> bool has_tpm;
> const unsigned char *dsdt_code;
> @@ -238,6 +240,14 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>
> static void acpi_get_misc_info(AcpiMiscInfo *info)
> {
> + Object *obj;
> +
> + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> + info->vmgen_buf_paddr = 0;
> + if (obj) {
> + info->vmgen_buf_paddr =
> + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
confused. So what happens if BAR is not mapped by guest?
> + }
> info->has_hpet = hpet_find();
> info->has_tpm = tpm_find();
> info->pvpanic_port = pvpanic_port();
> @@ -521,8 +531,27 @@ static void build_append_pcihp_notify_entry(Aml *method, int slot)
> aml_append(method, if_ctx);
> }
>
> +static char *acpi_get_pci_dev_scope_name(PCIDevice *pdev)
> +{
> + char *name = NULL;
> + char *last = name;
> + PCIBus *bus = pdev->bus;
> +
> + while (bus->parent_dev) {
> + last = name;
> + name = g_strdup_printf("%s.S%.02X_", name ? name : "",
> + bus->parent_dev->devfn);
> + g_free(last);
> + bus = bus->parent_dev->bus;
> + }
> + last = name;
> + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "", pdev->devfn);
> + g_free(last);
> + return name;
> +}
Looks tricky, and duplicates logic for device naming.
All this won't be necessary if you just add this as child
of the correct device, without playing with scope.
Why not do it?
> static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> - bool pcihp_bridge_en)
> + bool pcihp_bridge_en,
> + AcpiMiscInfo *misc)
> {
> Aml *dev, *notify_method, *method;
> QObject *bsel;
> @@ -583,7 +612,21 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> dev = aml_device("S%.02X", PCI_DEVFN(slot, 0));
> aml_append(dev, aml_name_decl("_ADR", aml_int(slot << 16)));
>
> - if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> + if (pc->device_id == PCI_DEVICE_ID_REDHAT_VMGENID) {
clearly not enough, one must also check the vendor.
> + Aml *pkg;
> +
> + aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0003")));
> + aml_append(dev,
> + aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
> + aml_append(dev,
> + aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
> + aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> +
> + pkg = aml_package(2);
> + aml_append(pkg, aml_int(misc->vmgen_buf_paddr));
> + aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr */
> + aml_append(dev, aml_name_decl("ADDR", pkg));
> + } else if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> /* add VGA specific AML methods */
> int s3d;
>
> @@ -624,7 +667,7 @@ static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus *bus,
> */
> PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>
> - build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en);
> + build_append_pci_bus_devices(dev, sec_bus, pcihp_bridge_en, misc);
> }
> /* slot descriptor has been composed, add it into parent context */
> aml_append(parent_scope, dev);
> @@ -1013,11 +1056,29 @@ build_ssdt(GArray *table_data, GArray *linker,
> if (bus) {
> Aml *scope = aml_scope("PCI0");
> /* Scan all PCI buses. Generate tables to support hotplug. */
> - build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en);
> + build_append_pci_bus_devices(scope, bus, pm->pcihp_bridge_en,
> + misc);
> aml_append(sb_scope, scope);
> }
> }
> aml_append(ssdt, sb_scope);
> +
> + if (misc->vmgen_buf_paddr) {
> + Object *obj;
> + char *vgid_path;
> +
> + scope = aml_scope("\\_GPE");
> + method = aml_method("_E00", 0);
> +
> + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> + vgid_path = acpi_get_pci_dev_scope_name(PCI_DEVICE(obj));
> + aml_append(method,
> + aml_notify(aml_name("\\_SB.%s", vgid_path), aml_int(0x80)));
> + g_free(vgid_path);
> +
> + aml_append(scope, method);
> + aml_append(ssdt, scope);
> + }
> }
>
> /* copy AML table into ACPI tables blob and patch header there */
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a2d84ec..e2e089f 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -260,8 +260,6 @@ DefinitionBlock (
> Scope(\_GPE) {
> Name(_HID, "ACPI0006")
>
> - Method(_L00) {
> - }
> Method(_E01) {
> // PCI hotplug event
> Acquire(\_SB.PCI0.BLCK, 0xFFFF)
> diff --git a/hw/i386/q35-acpi-dsdt.dsl b/hw/i386/q35-acpi-dsdt.dsl
> index 16eaca3..8d031c0 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -395,8 +395,6 @@ DefinitionBlock (
> Scope(\_GPE) {
> Name(_HID, "ACPI0006")
>
> - Method(_L00) {
> - }
> Method(_L01) {
> }
> Method(_E02) {
> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> index 029a56f..e047aea 100644
> --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -41,3 +41,4 @@ obj-$(CONFIG_ZYNQ) += zynq_slcr.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..c47fb06
> --- /dev/null
> +++ b/hw/misc/vmgenid.c
> @@ -0,0 +1,134 @@
> +/*
> + * Virtual Machine Generation ID Device
> + *
> + * Copyright (C) 2014 Red Hat Inc.
It's 2015 isn't it?
> + *
> + * Authors: Gal Hammer <ghammer@redhat.com>
> + * Igor Mammedov <imammedo@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/pci/pci.h"
> +#include "hw/misc/vmgenid.h"
> +#include "hw/acpi/acpi.h"
> +#include "qapi/visitor.h"
> +
> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
> +
> +typedef struct VmGenIdState {
> + PCIDevice parent_obj;
> + MemoryRegion iomem;
> + union {
> + uint8_t guid[16];
> + uint8_t guid_page[TARGET_PAGE_SIZE];
Please just make it 4K.
We don't want more target-specific code if we can help it.
> + };
> + bool guid_set;
> +} VmGenIdState;
> +
> +static void vmgenid_update_guest(VmGenIdState *s)
> +{
> + Object *acpi_obj;
> + void *ptr = memory_region_get_ram_ptr(&s->iomem);
> +
> + memcpy(ptr, &s->guid, sizeof(s->guid));
> + memory_region_set_dirty(&s->iomem, 0, sizeof(s->guid));
> +
> + 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);
> + ACPIREGS *acpi_regs = adevc->regs(adev);
> +
> + acpi_regs->gpe.sts[0] |= 1; /* _GPE.E00 handler */
> + acpi_update_sci(acpi_regs, adevc->sci(adev));
> + }
> +}
> +
> +static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
> +{
> + VmGenIdState *s = VMGENID(obj);
> +
> + if (qemu_uuid_parse(value, s->guid) < 0) {
> + error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse UUID string.",
s/Fail/Failed/
Also - print the string itself?
> + object_get_typename(OBJECT(s)));
> + return;
> + }
> +
> + s->guid_set = true;
> + vmgenid_update_guest(s);
> +}
> +
> +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void *opaque,
> + const char *name, Error **errp)
> +{
> + VmGenIdState *s = VMGENID(obj);
Why cast to VMGENID here?
> + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> +
> + if (value == PCI_BAR_UNMAPPED) {
> + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not initialized",
> + object_get_typename(OBJECT(s)));
This is guest error. Pls don't print these to monitor by default.
> + return;
> + }
> + visit_type_int(v, &value, name, errp);
> +}
> +
Useful for testing, but looks like of generic.
Add methods to access BAR from QOM for all pci
devices?
> +static void vmgenid_initfn(Object *obj)
> +{
> + VmGenIdState *s = VMGENID(obj);
> +
> + memory_region_init_ram(&s->iomem, obj, "vgid.bar", sizeof(s->guid_page),
> + &error_abort);
> +
> + object_property_add_str(obj, VMGENID_UUID, NULL, vmgenid_set_uuid, NULL);
> + object_property_add(obj, VMGENID_VMGID_ADDR, "int", vmgenid_get_vmgid_addr,
> + NULL, NULL, NULL, NULL);
> +}
> +
> +
> +static void vmgenid_realize(PCIDevice *dev, Error **errp)
> +{
> + VmGenIdState *s = VMGENID(dev);
> +
> + if (!s->guid_set) {
> + error_setg(errp, "'%s." VMGENID_UUID "' property is not set",
> + object_get_typename(OBJECT(s)));
> + return;
> + }
> +
> + vmstate_register_ram(&s->iomem, DEVICE(s));
> + pci_register_bar(PCI_DEVICE(s), 0, PCI_BASE_ADDRESS_MEM_PREFETCH,
> + &s->iomem);
> + return;
> +}
> +
> +static void vmgenid_class_init(ObjectClass *klass, void *data)
> +{
> + DeviceClass *dc = DEVICE_CLASS(klass);
> + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +
> + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> + dc->hotpluggable = false;
> + k->realize = vmgenid_realize;
> + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> + k->class_id = PCI_CLASS_MEMORY_RAM;
Still looks scary.
> +}
> +
> +static const TypeInfo vmgenid_device_info = {
> + .name = VMGENID_DEVICE,
> + .parent = TYPE_PCI_DEVICE,
> + .instance_size = sizeof(VmGenIdState),
> + .instance_init = vmgenid_initfn,
> + .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/acpi/acpi.h b/include/hw/acpi/acpi.h
> index 1f678b4..a09cb3f 100644
> --- a/include/hw/acpi/acpi.h
> +++ b/include/hw/acpi/acpi.h
> @@ -25,6 +25,7 @@
> #include "qemu/option.h"
> #include "exec/memory.h"
> #include "hw/irq.h"
> +#include "hw/acpi/acpi_dev_interface.h"
>
> /*
> * current device naming scheme supports up to 256 memory devices
I asked about this already I think - why is it here?
> diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
> new file mode 100644
> index 0000000..325f095
> --- /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>
> + * Igor Mammedov <imammedo@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_UUID "uuid"
uuid looks kind of too generic. vmgenid-uuid?
> +#define VMGENID_VMGID_ADDR "vmgid-addr"
> +
> +#endif
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 3164fc3..245171b 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -90,6 +90,7 @@
> #define PCI_DEVICE_ID_REDHAT_TEST 0x0005
> #define PCI_DEVICE_ID_REDHAT_SDHCI 0x0007
> #define PCI_DEVICE_ID_REDHAT_PCIE_HOST 0x0008
> +#define PCI_DEVICE_ID_REDHAT_VMGENID 0x0009
> #define PCI_DEVICE_ID_REDHAT_QXL 0x0100
>
> #define FMT_PCIBUS PRIx64
> --
> 2.1.0
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-03 17:35 ` Michael S. Tsirkin
@ 2015-03-03 20:33 ` Igor Mammedov
2015-03-04 12:11 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-03-03 20:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, qemu-devel
On Tue, 3 Mar 2015 18:35:39 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> >
> > Example of using vmgenid device:
> > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>
> So this is not a great example since we are burning up
> a pci slot. Management can work around this by making this
> a multifunction device and making this part of chipset,
> but how about we make this a default, by specifying
> appropriate addr and multifunction properties
> as part of machine type.
Why make it default? It's some Windows specific thing that
should be used when guest is Windows to be of any use
and not present when it's not needed.
>
> Also, how are we going to extend this device?
> looks like we've burned it all just for vmid?
I don't like the way MS uses yet another side-channel
to communicate something (UUID) instead of using ACPI
method for getting it.
I'd rather avoid extending it beyond of what it's now
and use channels that we already have.
> How about we have a slightly more generic container
> where we'll be able to stick all kind of stuff
> in the future, and make vmgenid a child of
> this device?
What other possible uses do you have in mind?
> > To change uuid in runtime use:
> > qom-set "/machine/peripheral/FOO.uuid"
> > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>
> Looking just at this, how does user discover this functionality?
what do you mean?
[...]
> >
> > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > {
> > + Object *obj;
> > +
> > + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> > + info->vmgen_buf_paddr = 0;
> > + if (obj) {
> > + info->vmgen_buf_paddr =
> > + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
>
> confused. So what happens if BAR is not mapped by guest?
it will get 0 address on acpi_setup() stage but later
when ACPI tables are read by BIOS (which happens after PCI is
initialized) it will be updated and get mapped address.
>
> > + }
> > info->has_hpet = hpet_find();
> > info->has_tpm = tpm_find();
> > info->pvpanic_port = pvpanic_port();
> > @@ -521,8 +531,27 @@ static void
> > build_append_pcihp_notify_entry(Aml *method, int slot)
> > aml_append(method, if_ctx); }
> >
> > +static char *acpi_get_pci_dev_scope_name(PCIDevice *pdev)
> > +{
> > + char *name = NULL;
> > + char *last = name;
> > + PCIBus *bus = pdev->bus;
> > +
> > + while (bus->parent_dev) {
> > + last = name;
> > + name = g_strdup_printf("%s.S%.02X_", name ? name : "",
> > + bus->parent_dev->devfn);
> > + g_free(last);
> > + bus = bus->parent_dev->bus;
> > + }
> > + last = name;
> > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > pdev->devfn);
> > + g_free(last);
> > + return name;
> > +}
>
> Looks tricky, and duplicates logic for device naming.
> All this won't be necessary if you just add this as child
> of the correct device, without playing with scope.
> Why not do it?
since vmgenid PCI device is located somewhere on PCI bus we don't have
fixed PATH to it and we need full path to it to send Notivy from
"\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
>
>
> > static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus
> > *bus,
> > - bool pcihp_bridge_en)
> > + bool pcihp_bridge_en,
> > + AcpiMiscInfo *misc)
> > {
> > Aml *dev, *notify_method, *method;
> > QObject *bsel;
> > @@ -583,7 +612,21 @@ static void build_append_pci_bus_devices(Aml
> > *parent_scope, PCIBus *bus, dev = aml_device("S%.02X",
> > PCI_DEVFN(slot, 0)); aml_append(dev, aml_name_decl("_ADR",
> > aml_int(slot << 16)));
> > - if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > + if (pc->device_id == PCI_DEVICE_ID_REDHAT_VMGENID) {
>
> clearly not enough, one must also check the vendor.
Yep, I've noticed that after sending, will fix.
[...]
> > diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
> > new file mode 100644
> > index 0000000..c47fb06
> > --- /dev/null
> > +++ b/hw/misc/vmgenid.c
> > @@ -0,0 +1,134 @@
> > +/*
> > + * Virtual Machine Generation ID Device
> > + *
> > + * Copyright (C) 2014 Red Hat Inc.
>
> It's 2015 isn't it?
sure, I'll fix it.
[...]
> > +typedef struct VmGenIdState {
> > + PCIDevice parent_obj;
> > + MemoryRegion iomem;
> > + union {
> > + uint8_t guid[16];
> > + uint8_t guid_page[TARGET_PAGE_SIZE];
>
> Please just make it 4K.
> We don't want more target-specific code if we can help it.
ok
[...]
> > +static void vmgenid_set_uuid(Object *obj, const char *value, Error
> > **errp) +{
> > + VmGenIdState *s = VMGENID(obj);
> > +
> > + if (qemu_uuid_parse(value, s->guid) < 0) {
> > + error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse
> > UUID string.",
>
> s/Fail/Failed/
sure
> Also - print the string itself?
What do you mean?
[...]
> > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > *opaque,
> > + const char *name, Error **errp)
> > +{
> > + VmGenIdState *s = VMGENID(obj);
>
> Why cast to VMGENID here?
Yep, there is no need to do it, I'll clean it up.
>
> > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > +
> > + if (value == PCI_BAR_UNMAPPED) {
> > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > initialized",
> > + object_get_typename(OBJECT(s)));
>
> This is guest error. Pls don't print these to monitor by default.
Then how test case querying this property via QOM could get to know
that property is in wrong state yet?
>
> > + return;
> > + }
> > + visit_type_int(v, &value, name, errp);
> > +}
> > +
>
> Useful for testing, but looks like of generic.
> Add methods to access BAR from QOM for all pci
> devices?
That's what it's used for in following test case.
But I'm not sure how to make it generic provided it
would be sort of array property (which Peter introduced)
but later they got some critique, I haven't kept on that issue though.
I'd leave this as it is for now.
> > +static void vmgenid_class_init(ObjectClass *klass, void *data)
> > +{
> > + DeviceClass *dc = DEVICE_CLASS(klass);
> > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > +
> > + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > + dc->hotpluggable = false;
> > + k->realize = vmgenid_realize;
> > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > + k->class_id = PCI_CLASS_MEMORY_RAM;
>
> Still looks scary.
Can do nothing about it,
it's closest class id to what this device is
(i.e. it exposes page of RAM) that works with Windows
without asking for drivers.
If that class id is not acceptable then let's drop PCI
approach altogether.
More over it's limited to target-i386 only and possibly
could apply to ARM in the future when Windows comes there,
so in this case I'm not very concerned about pseries guests
especially with buggy kernel as it was reported in
http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
[...]
> > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > index 1f678b4..a09cb3f 100644
> > --- a/include/hw/acpi/acpi.h
> > +++ b/include/hw/acpi/acpi.h
> > @@ -25,6 +25,7 @@
> > #include "qemu/option.h"
> > #include "exec/memory.h"
> > #include "hw/irq.h"
> > +#include "hw/acpi/acpi_dev_interface.h"
> >
> > /*
> > * current device naming scheme supports up to 256 memory devices
>
> I asked about this already I think - why is it here?
do you mean comment "current device naming scheme ..."
[...]
> > +
> > +#ifndef HW_MISC_VMGENID_H
> > +#define HW_MISC_VMGENID_H
> > +
> > +#define VMGENID_DEVICE "vmgenid"
> > +#define VMGENID_UUID "uuid"
>
> uuid looks kind of too generic. vmgenid-uuid?
sure
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-03 20:33 ` Igor Mammedov
@ 2015-03-04 12:11 ` Michael S. Tsirkin
2015-03-04 13:12 ` Igor Mammedov
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-03-04 12:11 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, qemu-devel
On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> On Tue, 3 Mar 2015 18:35:39 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > >
> > > Example of using vmgenid device:
> > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> >
> > So this is not a great example since we are burning up
> > a pci slot. Management can work around this by making this
> > a multifunction device and making this part of chipset,
> > but how about we make this a default, by specifying
> > appropriate addr and multifunction properties
> > as part of machine type.
> Why make it default? It's some Windows specific thing that
> should be used when guest is Windows to be of any use
> and not present when it's not needed.
yes, but when it's used, I'd like to avoid using up a whole slot.
> >
> > Also, how are we going to extend this device?
> > looks like we've burned it all just for vmid?
> I don't like the way MS uses yet another side-channel
> to communicate something (UUID) instead of using ACPI
> method for getting it.
> I'd rather avoid extending it beyond of what it's now
> and use channels that we already have.
Famous last words :)
> > How about we have a slightly more generic container
> > where we'll be able to stick all kind of stuff
> > in the future, and make vmgenid a child of
> > this device?
> What other possible uses do you have in mind?
I don't know for sure - some other value that applications want to map.
> > > To change uuid in runtime use:
> > > qom-set "/machine/peripheral/FOO.uuid"
> > > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> >
> > Looking just at this, how does user discover this functionality?
> what do you mean?
>
> [...]
Just this. You are a user. You want to change the vm gen id.
Adding devices is partially documented
in -help and -device help. commands are documented in hmp help.
But how do you find out that qom-set should be used to
update vm gen id, and how do you find out how to do this?
> > >
> > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > {
> > > + Object *obj;
> > > +
> > > + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> > > + info->vmgen_buf_paddr = 0;
> > > + if (obj) {
> > > + info->vmgen_buf_paddr =
> > > + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
> >
> > confused. So what happens if BAR is not mapped by guest?
> it will get 0 address on acpi_setup() stage but later
> when ACPI tables are read by BIOS (which happens after PCI is
> initialized) it will be updated and get mapped address.
Yes but it's up to guest. What if guest does not map BAR
later, either?
BTW, why do we need to stick vmgen_buf_paddr in the info?
> >
> > > + }
> > > info->has_hpet = hpet_find();
> > > info->has_tpm = tpm_find();
> > > info->pvpanic_port = pvpanic_port();
> > > @@ -521,8 +531,27 @@ static void
> > > build_append_pcihp_notify_entry(Aml *method, int slot)
> > > aml_append(method, if_ctx); }
> > >
> > > +static char *acpi_get_pci_dev_scope_name(PCIDevice *pdev)
> > > +{
> > > + char *name = NULL;
> > > + char *last = name;
> > > + PCIBus *bus = pdev->bus;
> > > +
> > > + while (bus->parent_dev) {
> > > + last = name;
> > > + name = g_strdup_printf("%s.S%.02X_", name ? name : "",
> > > + bus->parent_dev->devfn);
> > > + g_free(last);
> > > + bus = bus->parent_dev->bus;
> > > + }
> > > + last = name;
> > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > pdev->devfn);
> > > + g_free(last);
> > > + return name;
> > > +}
> >
> > Looks tricky, and duplicates logic for device naming.
> > All this won't be necessary if you just add this as child
> > of the correct device, without playing with scope.
> > Why not do it?
> since vmgenid PCI device is located somewhere on PCI bus we don't have
> fixed PATH to it and we need full path to it to send Notivy from
> "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
I see. Still - can't this function return the full aml_name?
> >
> >
> > > static void build_append_pci_bus_devices(Aml *parent_scope, PCIBus
> > > *bus,
> > > - bool pcihp_bridge_en)
> > > + bool pcihp_bridge_en,
> > > + AcpiMiscInfo *misc)
> > > {
> > > Aml *dev, *notify_method, *method;
> > > QObject *bsel;
> > > @@ -583,7 +612,21 @@ static void build_append_pci_bus_devices(Aml
> > > *parent_scope, PCIBus *bus, dev = aml_device("S%.02X",
> > > PCI_DEVFN(slot, 0)); aml_append(dev, aml_name_decl("_ADR",
> > > aml_int(slot << 16)));
> > > - if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> > > + if (pc->device_id == PCI_DEVICE_ID_REDHAT_VMGENID) {
> >
> > clearly not enough, one must also check the vendor.
> Yep, I've noticed that after sending, will fix.
>
> [...]
> > > diff --git a/hw/misc/vmgenid.c b/hw/misc/vmgenid.c
> > > new file mode 100644
> > > index 0000000..c47fb06
> > > --- /dev/null
> > > +++ b/hw/misc/vmgenid.c
> > > @@ -0,0 +1,134 @@
> > > +/*
> > > + * Virtual Machine Generation ID Device
> > > + *
> > > + * Copyright (C) 2014 Red Hat Inc.
> >
> > It's 2015 isn't it?
> sure, I'll fix it.
>
> [...]
> > > +typedef struct VmGenIdState {
> > > + PCIDevice parent_obj;
> > > + MemoryRegion iomem;
> > > + union {
> > > + uint8_t guid[16];
> > > + uint8_t guid_page[TARGET_PAGE_SIZE];
> >
> > Please just make it 4K.
> > We don't want more target-specific code if we can help it.
> ok
>
> [...]
> > > +static void vmgenid_set_uuid(Object *obj, const char *value, Error
> > > **errp) +{
> > > + VmGenIdState *s = VMGENID(obj);
> > > +
> > > + if (qemu_uuid_parse(value, s->guid) < 0) {
> > > + error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse
> > > UUID string.",
> >
> > s/Fail/Failed/
> sure
>
> > Also - print the string itself?
> What do you mean?
the uuid string which we failed to parse?
> [...]
> > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > *opaque,
> > > + const char *name, Error **errp)
> > > +{
> > > + VmGenIdState *s = VMGENID(obj);
> >
> > Why cast to VMGENID here?
> Yep, there is no need to do it, I'll clean it up.
>
> >
> > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > +
> > > + if (value == PCI_BAR_UNMAPPED) {
> > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > initialized",
> > > + object_get_typename(OBJECT(s)));
> >
> > This is guest error. Pls don't print these to monitor by default.
> Then how test case querying this property via QOM could get to know
> that property is in wrong state yet?
Maybe leave this around for tests (with a comment)
but use plain pci_get_bar_addr internally?
> >
> > > + return;
> > > + }
> > > + visit_type_int(v, &value, name, errp);
> > > +}
> > > +
> >
> > Useful for testing, but looks like of generic.
> > Add methods to access BAR from QOM for all pci
> > devices?
> That's what it's used for in following test case.
>
> But I'm not sure how to make it generic provided it
> would be sort of array property (which Peter introduced)
> but later they got some critique, I haven't kept on that issue though.
> I'd leave this as it is for now.
>
> > > +static void vmgenid_class_init(ObjectClass *klass, void *data)
> > > +{
> > > + DeviceClass *dc = DEVICE_CLASS(klass);
> > > + PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> > > +
> > > + set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> > > + dc->hotpluggable = false;
> > > + k->realize = vmgenid_realize;
> > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> >
> > Still looks scary.
> Can do nothing about it,
> it's closest class id to what this device is
> (i.e. it exposes page of RAM) that works with Windows
> without asking for drivers.
> If that class id is not acceptable then let's drop PCI
> approach altogether.
>
> More over it's limited to target-i386 only and possibly
> could apply to ARM in the future when Windows comes there,
> so in this case I'm not very concerned about pseries guests
I don't think we should treat this as a windows only device,
the function seems generally useful.
> especially with buggy kernel as it was reported in
> http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
I think it's firmware that's confused, not the guest kernel.
>
> [...]
Some options to think about/try
1. PCI_CLASS_MEMORY_OTHER (or some other class?)
2. Name(_HID, "PNP0A06") (or some other id)
> > > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > > index 1f678b4..a09cb3f 100644
> > > --- a/include/hw/acpi/acpi.h
> > > +++ b/include/hw/acpi/acpi.h
> > > @@ -25,6 +25,7 @@
> > > #include "qemu/option.h"
> > > #include "exec/memory.h"
> > > #include "hw/irq.h"
> > > +#include "hw/acpi/acpi_dev_interface.h"
> > >
> > > /*
> > > * current device naming scheme supports up to 256 memory devices
> >
> > I asked about this already I think - why is it here?
> do you mean comment "current device naming scheme ..."
>
> [...]
no - the extra include.
> > > +
> > > +#ifndef HW_MISC_VMGENID_H
> > > +#define HW_MISC_VMGENID_H
> > > +
> > > +#define VMGENID_DEVICE "vmgenid"
> > > +#define VMGENID_UUID "uuid"
> >
> > uuid looks kind of too generic. vmgenid-uuid?
> sure
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 12:11 ` Michael S. Tsirkin
@ 2015-03-04 13:12 ` Igor Mammedov
2015-03-04 13:49 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-03-04 13:12 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, qemu-devel
On Wed, 4 Mar 2015 13:11:48 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > On Tue, 3 Mar 2015 18:35:39 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > >
> > > > Example of using vmgenid device:
> > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > >
> > > So this is not a great example since we are burning up
> > > a pci slot. Management can work around this by making this
> > > a multifunction device and making this part of chipset,
> > > but how about we make this a default, by specifying
> > > appropriate addr and multifunction properties
> > > as part of machine type.
> > Why make it default? It's some Windows specific thing that
> > should be used when guest is Windows to be of any use
> > and not present when it's not needed.
>
> yes, but when it's used, I'd like to avoid using up a whole slot.
As I understand mutifunction devices are usually composit
ones and created along with parent device.
I'm not sure what to do here,
an example or some hints how to implement it are welcome.
>
>
> > >
> > > Also, how are we going to extend this device?
> > > looks like we've burned it all just for vmid?
> > I don't like the way MS uses yet another side-channel
> > to communicate something (UUID) instead of using ACPI
> > method for getting it.
> > I'd rather avoid extending it beyond of what it's now
> > and use channels that we already have.
>
> Famous last words :)
>
>
> > > How about we have a slightly more generic container
> > > where we'll be able to stick all kind of stuff
> > > in the future, and make vmgenid a child of
> > > this device?
> > What other possible uses do you have in mind?
>
> I don't know for sure - some other value that applications want to map.
Well then suggest something more concrete here I don't
quietly have an idea what
> > > How about we have a slightly more generic container
> > > where we'll be able to stick all kind of stuff
> > > in the future, and make vmgenid a child of
> > > this device?
means, maybe we need a canfcall with Gal to discuss idea?
>
>
> > > > To change uuid in runtime use:
> > > > qom-set "/machine/peripheral/FOO.uuid"
> > > > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > >
> > > Looking just at this, how does user discover this functionality?
> > what do you mean?
> >
> > [...]
>
> Just this. You are a user. You want to change the vm gen id.
> Adding devices is partially documented
> in -help and -device help. commands are documented in hmp help.
> But how do you find out that qom-set should be used to
> update vm gen id, and how do you find out how to do this?
I don't really know, it's approach used in original patches.
Any suggestions?
Do QOM properties have some 'help' connected to them? If yes we
could stick explanation there so that -device vmgenid,help
would show it at least.
>
>
> > > >
> > > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > > {
> > > > + Object *obj;
> > > > +
> > > > + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> > > > + info->vmgen_buf_paddr = 0;
> > > > + if (obj) {
> > > > + info->vmgen_buf_paddr =
> > > > + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
> > >
> > > confused. So what happens if BAR is not mapped by guest?
> > it will get 0 address on acpi_setup() stage but later
> > when ACPI tables are read by BIOS (which happens after PCI is
> > initialized) it will be updated and get mapped address.
>
> Yes but it's up to guest. What if guest does not map BAR
> later, either?
I'd prefer to abort machine since otherwise guest OS wouldn't
get what its users expected to and silently would continue running.
Considering that MS intends to use this value for cryptography
purposes () it would be security risk.
>
>
> BTW, why do we need to stick vmgen_buf_paddr in the info?
Because according to MS spec device should have ADDR object
with physical buffer address packed in Package(2). So that
Windows could read value from there.
[...]
> > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > pdev->devfn);
> > > > + g_free(last);
> > > > + return name;
> > > > +}
> > >
> > > Looks tricky, and duplicates logic for device naming.
> > > All this won't be necessary if you just add this as child
> > > of the correct device, without playing with scope.
> > > Why not do it?
> > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > fixed PATH to it and we need full path to it to send Notivy from
> > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
>
> I see. Still - can't this function return the full aml_name?
it's possible but I'd prefer to return back to 2 ACPI devices as it was
in v13 since Windows sees 2 devices anyway, even if they merged into one
PCI device description (which probably wrong but windows handles it because
PCI Standard RAM controller is driver less) and get rid of
acpi_get_pci_dev_scope_name() thing.
It will also help if vmgenid will be a part of multifunction device,
which current build_append_pci_bus_devices() ignores for now (i.e. it
describes only function 0 devices on slot).
[...]
> > > > +static void vmgenid_set_uuid(Object *obj, const char *value, Error
> > > > **errp) +{
> > > > + VmGenIdState *s = VMGENID(obj);
> > > > +
> > > > + if (qemu_uuid_parse(value, s->guid) < 0) {
> > > > + error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse
> > > > UUID string.",
> > >
> > > s/Fail/Failed/
> > sure
> >
> > > Also - print the string itself?
> > What do you mean?
>
> the uuid string which we failed to parse?
sure
>
> > [...]
> > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > > *opaque,
> > > > + const char *name, Error **errp)
> > > > +{
> > > > + VmGenIdState *s = VMGENID(obj);
> > >
> > > Why cast to VMGENID here?
> > Yep, there is no need to do it, I'll clean it up.
> >
> > >
> > > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > > +
> > > > + if (value == PCI_BAR_UNMAPPED) {
> > > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > > initialized",
> > > > + object_get_typename(OBJECT(s)));
> > >
> > > This is guest error. Pls don't print these to monitor by default.
> > Then how test case querying this property via QOM could get to know
> > that property is in wrong state yet?
>
> Maybe leave this around for tests (with a comment)
> but use plain pci_get_bar_addr internally?
Accessing it internally as property will also allow to
prevent guest starting if BIOS failed to initialize BAR
(not implemented but shouldn't be hard to do)
[...]
> > > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> > >
> > > Still looks scary.
> > Can do nothing about it,
> > it's closest class id to what this device is
> > (i.e. it exposes page of RAM) that works with Windows
> > without asking for drivers.
> > If that class id is not acceptable then let's drop PCI
> > approach altogether.
> >
> > More over it's limited to target-i386 only and possibly
> > could apply to ARM in the future when Windows comes there,
> > so in this case I'm not very concerned about pseries guests
>
> I don't think we should treat this as a windows only device,
> the function seems generally useful.
>
> > especially with buggy kernel as it was reported in
> > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
>
> I think it's firmware that's confused, not the guest kernel.
maybe both, should we care about faulty guest pieces when they
don't use this device. If the pseries would need to use it then
they should fix guest size instead of poking soldering iron
in HW.
>
> >
> > [...]
>
>
> Some options to think about/try
> 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
I've already tried this and a number of others,
Windows asks for driver.
> 2. Name(_HID, "PNP0A06") (or some other id)
experiment on Windows shows that _HID doesn't influence PCI devices described
in ACPI in any way. In this version _HID = QEMU0003 and its required
by VMGID spec to have unique vendor specific HID for VMGID device.
It looks like PCI driver mostly ignores PCI slots described in ACPI
and as result there are devices in device manager "PCI standard RAM Ctrl"
and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
tables.
>
>
>
> > > > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > > > index 1f678b4..a09cb3f 100644
> > > > --- a/include/hw/acpi/acpi.h
> > > > +++ b/include/hw/acpi/acpi.h
> > > > @@ -25,6 +25,7 @@
> > > > #include "qemu/option.h"
> > > > #include "exec/memory.h"
> > > > #include "hw/irq.h"
> > > > +#include "hw/acpi/acpi_dev_interface.h"
> > > >
> > > > /*
> > > > * current device naming scheme supports up to 256 memory devices
> > >
> > > I asked about this already I think - why is it here?
> > do you mean comment "current device naming scheme ..."
> >
> > [...]
>
> no - the extra include.
ok, will fix.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 13:12 ` Igor Mammedov
@ 2015-03-04 13:49 ` Michael S. Tsirkin
2015-03-04 14:03 ` Andreas Färber
2015-03-04 15:14 ` Igor Mammedov
0 siblings, 2 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-03-04 13:49 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, David Gibson, qemu-devel, afaerber
On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> On Wed, 4 Mar 2015 13:11:48 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > >
> > > > > Example of using vmgenid device:
> > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > > >
> > > > So this is not a great example since we are burning up
> > > > a pci slot. Management can work around this by making this
> > > > a multifunction device and making this part of chipset,
> > > > but how about we make this a default, by specifying
> > > > appropriate addr and multifunction properties
> > > > as part of machine type.
> > > Why make it default? It's some Windows specific thing that
> > > should be used when guest is Windows to be of any use
> > > and not present when it's not needed.
> >
> > yes, but when it's used, I'd like to avoid using up a whole slot.
> As I understand mutifunction devices are usually composit
> ones and created along with parent device.
> I'm not sure what to do here,
> an example or some hints how to implement it are welcome.
I think you can just set addr and multifunction properties.
> >
> >
> > > >
> > > > Also, how are we going to extend this device?
> > > > looks like we've burned it all just for vmid?
> > > I don't like the way MS uses yet another side-channel
> > > to communicate something (UUID) instead of using ACPI
> > > method for getting it.
> > > I'd rather avoid extending it beyond of what it's now
> > > and use channels that we already have.
> >
> > Famous last words :)
> >
> >
> > > > How about we have a slightly more generic container
> > > > where we'll be able to stick all kind of stuff
> > > > in the future, and make vmgenid a child of
> > > > this device?
> > > What other possible uses do you have in mind?
> >
> > I don't know for sure - some other value that applications want to map.
> Well then suggest something more concrete here I don't
> quietly have an idea what
> > > > How about we have a slightly more generic container
> > > > where we'll be able to stick all kind of stuff
> > > > in the future, and make vmgenid a child of
> > > > this device?
> means, maybe we need a canfcall with Gal to discuss idea?
No problem. It's unfortunate we missed the developer conf call this
tuesday. If you like, I'll try to setup something for Monday, I'd like
to attend too. Anyone else interested?
> >
> >
> > > > > To change uuid in runtime use:
> > > > > qom-set "/machine/peripheral/FOO.uuid"
> > > > > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > > >
> > > > Looking just at this, how does user discover this functionality?
> > > what do you mean?
> > >
> > > [...]
> >
> > Just this. You are a user. You want to change the vm gen id.
> > Adding devices is partially documented
> > in -help and -device help. commands are documented in hmp help.
> > But how do you find out that qom-set should be used to
> > update vm gen id, and how do you find out how to do this?
> I don't really know, it's approach used in original patches.
> Any suggestions?
> Do QOM properties have some 'help' connected to them? If yes we
> could stick explanation there so that -device vmgenid,help
> would show it at least.
Eric, Andreas, any comments on this part?
> >
> >
> > > > >
> > > > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > > > {
> > > > > + Object *obj;
> > > > > +
> > > > > + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> > > > > + info->vmgen_buf_paddr = 0;
> > > > > + if (obj) {
> > > > > + info->vmgen_buf_paddr =
> > > > > + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
> > > >
> > > > confused. So what happens if BAR is not mapped by guest?
> > > it will get 0 address on acpi_setup() stage but later
> > > when ACPI tables are read by BIOS (which happens after PCI is
> > > initialized) it will be updated and get mapped address.
> >
> > Yes but it's up to guest. What if guest does not map BAR
> > later, either?
> I'd prefer to abort machine since otherwise guest OS wouldn't
> get what its users expected to and silently would continue running.
> Considering that MS intends to use this value for cryptography
> purposes () it would be security risk.
An aborted guest is very secure but this is going way overboard I think.
It's easy for guest to detect that the ACPI device is not there,
whether crashing is the best solution in this case
is a policy question. In particular something like virtio
rng looks like an adequate replacement.
> >
> >
> > BTW, why do we need to stick vmgen_buf_paddr in the info?
> Because according to MS spec device should have ADDR object
> with physical buffer address packed in Package(2). So that
> Windows could read value from there.
>
> [...]
Yes but why not read the property when and where we
need it?
> > > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > > pdev->devfn);
> > > > > + g_free(last);
> > > > > + return name;
> > > > > +}
> > > >
> > > > Looks tricky, and duplicates logic for device naming.
> > > > All this won't be necessary if you just add this as child
> > > > of the correct device, without playing with scope.
> > > > Why not do it?
> > > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > > fixed PATH to it and we need full path to it to send Notivy from
> > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
> >
> > I see. Still - can't this function return the full aml_name?
> it's possible but I'd prefer to return back to 2 ACPI devices as it was
> in v13 since Windows sees 2 devices anyway, even if they merged into one
> PCI device description (which probably wrong but windows handles it because
> PCI Standard RAM controller is driver less) and get rid of
> acpi_get_pci_dev_scope_name() thing.
OK but I think it should be under PCI0 at least,
since that one claims the relevant resource in its CRS.
> It will also help if vmgenid will be a part of multifunction device,
> which current build_append_pci_bus_devices() ignores for now (i.e. it
> describes only function 0 devices on slot).
>
> [...]
OK, though we might need to add the description for the pci device anyway
e.g. in order to mark it hidden.
> > > > > +static void vmgenid_set_uuid(Object *obj, const char *value, Error
> > > > > **errp) +{
> > > > > + VmGenIdState *s = VMGENID(obj);
> > > > > +
> > > > > + if (qemu_uuid_parse(value, s->guid) < 0) {
> > > > > + error_setg(errp, "'%s." VMGENID_UUID "': Fail to parse
> > > > > UUID string.",
> > > >
> > > > s/Fail/Failed/
> > > sure
> > >
> > > > Also - print the string itself?
> > > What do you mean?
> >
> > the uuid string which we failed to parse?
> sure
>
> >
> > > [...]
> > > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > > > *opaque,
> > > > > + const char *name, Error **errp)
> > > > > +{
> > > > > + VmGenIdState *s = VMGENID(obj);
> > > >
> > > > Why cast to VMGENID here?
> > > Yep, there is no need to do it, I'll clean it up.
> > >
> > > >
> > > > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > > > +
> > > > > + if (value == PCI_BAR_UNMAPPED) {
> > > > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > > > initialized",
> > > > > + object_get_typename(OBJECT(s)));
> > > >
> > > > This is guest error. Pls don't print these to monitor by default.
> > > Then how test case querying this property via QOM could get to know
> > > that property is in wrong state yet?
> >
> > Maybe leave this around for tests (with a comment)
> > but use plain pci_get_bar_addr internally?
> Accessing it internally as property will also allow to
> prevent guest starting if BIOS failed to initialize BAR
> (not implemented but shouldn't be hard to do)
>
> [...]
I don't think it's a good idea. It's just a device,
it's not the most important thing for guests,
it's a policy question whether to initialize it.
> > > > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> > > >
> > > > Still looks scary.
> > > Can do nothing about it,
> > > it's closest class id to what this device is
> > > (i.e. it exposes page of RAM) that works with Windows
> > > without asking for drivers.
> > > If that class id is not acceptable then let's drop PCI
> > > approach altogether.
> > >
> > > More over it's limited to target-i386 only and possibly
> > > could apply to ARM in the future when Windows comes there,
> > > so in this case I'm not very concerned about pseries guests
> >
> > I don't think we should treat this as a windows only device,
> > the function seems generally useful.
> >
> > > especially with buggy kernel as it was reported in
> > > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
> >
> > I think it's firmware that's confused, not the guest kernel.
> maybe both, should we care about faulty guest pieces when they
> don't use this device. If the pseries would need to use it then
> they should fix guest size instead of poking soldering iron
> in HW.
It's better if we can avoid the issue altogether.
Assuming that we can't,
I'd like some confirmation from David Gibson on this.
> >
> > >
> > > [...]
> >
> >
> > Some options to think about/try
> > 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
> I've already tried this and a number of others,
> Windows asks for driver.
>
> > 2. Name(_HID, "PNP0A06") (or some other id)
> experiment on Windows shows that _HID doesn't influence PCI devices described
> in ACPI in any way. In this version _HID = QEMU0003 and its required
> by VMGID spec to have unique vendor specific HID for VMGID device.
> It looks like PCI driver mostly ignores PCI slots described in ACPI
> and as result there are devices in device manager "PCI standard RAM Ctrl"
> and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
> tables.
I see. Interesting.
And VM Gen ID isn't using the resources of the pci
device?
Any other ideas? Mark it hidden?
> >
> >
> >
> > > > > diff --git a/include/hw/acpi/acpi.h b/include/hw/acpi/acpi.h
> > > > > index 1f678b4..a09cb3f 100644
> > > > > --- a/include/hw/acpi/acpi.h
> > > > > +++ b/include/hw/acpi/acpi.h
> > > > > @@ -25,6 +25,7 @@
> > > > > #include "qemu/option.h"
> > > > > #include "exec/memory.h"
> > > > > #include "hw/irq.h"
> > > > > +#include "hw/acpi/acpi_dev_interface.h"
> > > > >
> > > > > /*
> > > > > * current device naming scheme supports up to 256 memory devices
> > > >
> > > > I asked about this already I think - why is it here?
> > > do you mean comment "current device naming scheme ..."
> > >
> > > [...]
> >
> > no - the extra include.
> ok, will fix.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 13:49 ` Michael S. Tsirkin
@ 2015-03-04 14:03 ` Andreas Färber
2015-03-04 15:14 ` Igor Mammedov
1 sibling, 0 replies; 24+ messages in thread
From: Andreas Färber @ 2015-03-04 14:03 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, Igor Mammedov, qemu-devel, David Gibson
Am 04.03.2015 um 14:49 schrieb Michael S. Tsirkin:
> On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
>> On Wed, 4 Mar 2015 13:11:48 +0100
>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
>>>> On Tue, 3 Mar 2015 18:35:39 +0100
>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>>>> On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov wrote:
>>>>>> To change uuid in runtime use:
>>>>>> qom-set "/machine/peripheral/FOO.uuid"
>>>>>> "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>>>>>
>>>>> Looking just at this, how does user discover this functionality?
>>>> what do you mean?
>>>>
>>>> [...]
>>>
>>> Just this. You are a user. You want to change the vm gen id.
>>> Adding devices is partially documented
>>> in -help and -device help. commands are documented in hmp help.
>>> But how do you find out that qom-set should be used to
>>> update vm gen id, and how do you find out how to do this?
>> I don't really know, it's approach used in original patches.
>> Any suggestions?
>> Do QOM properties have some 'help' connected to them? If yes we
>> could stick explanation there so that -device vmgenid,help
>> would show it at least.
>
> Eric, Andreas, any comments on this part?
Since recently there is some function to add a description to either
qdev or QOM properties, I don't quite remember where exactly...
Regards,
Andreas
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 13:49 ` Michael S. Tsirkin
2015-03-04 14:03 ` Andreas Färber
@ 2015-03-04 15:14 ` Igor Mammedov
2015-03-04 15:31 ` Michael S. Tsirkin
1 sibling, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-03-04 15:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, David Gibson, qemu-devel, afaerber
On Wed, 4 Mar 2015 14:49:00 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> > On Wed, 4 Mar 2015 13:11:48 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > > >
> > > > > > Example of using vmgenid device:
> > > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
[...]
> > > > > Also, how are we going to extend this device?
> > > > > looks like we've burned it all just for vmid?
> > > > I don't like the way MS uses yet another side-channel
> > > > to communicate something (UUID) instead of using ACPI
> > > > method for getting it.
> > > > I'd rather avoid extending it beyond of what it's now
> > > > and use channels that we already have.
> > >
> > > Famous last words :)
> > >
> > >
> > > > > How about we have a slightly more generic container
> > > > > where we'll be able to stick all kind of stuff
> > > > > in the future, and make vmgenid a child of
> > > > > this device?
> > > > What other possible uses do you have in mind?
> > >
> > > I don't know for sure - some other value that applications want to map.
> > Well then suggest something more concrete here I don't
> > quietly have an idea what
> > > > > How about we have a slightly more generic container
> > > > > where we'll be able to stick all kind of stuff
> > > > > in the future, and make vmgenid a child of
> > > > > this device?
> > means, maybe we need a canfcall with Gal to discuss idea?
>
> No problem. It's unfortunate we missed the developer conf call this
> tuesday. If you like, I'll try to setup something for Monday, I'd like
> to attend too. Anyone else interested?
Monday is fine for me.
>
> > >
> > >
> > > > > > To change uuid in runtime use:
> > > > > > qom-set "/machine/peripheral/FOO.uuid"
> > > > > > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > > > >
> > > > > Looking just at this, how does user discover this functionality?
> > > > what do you mean?
> > > >
> > > > [...]
> > >
> > > Just this. You are a user. You want to change the vm gen id.
> > > Adding devices is partially documented
> > > in -help and -device help. commands are documented in hmp help.
> > > But how do you find out that qom-set should be used to
> > > update vm gen id, and how do you find out how to do this?
> > I don't really know, it's approach used in original patches.
> > Any suggestions?
> > Do QOM properties have some 'help' connected to them? If yes we
> > could stick explanation there so that -device vmgenid,help
> > would show it at least.
>
> Eric, Andreas, any comments on this part?
>
> > >
> > >
> > > > > >
> > > > > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > > > > {
> > > > > > + Object *obj;
> > > > > > +
> > > > > > + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> > > > > > + info->vmgen_buf_paddr = 0;
> > > > > > + if (obj) {
> > > > > > + info->vmgen_buf_paddr =
> > > > > > + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
> > > > >
> > > > > confused. So what happens if BAR is not mapped by guest?
> > > > it will get 0 address on acpi_setup() stage but later
> > > > when ACPI tables are read by BIOS (which happens after PCI is
> > > > initialized) it will be updated and get mapped address.
> > >
> > > Yes but it's up to guest. What if guest does not map BAR
> > > later, either?
> > I'd prefer to abort machine since otherwise guest OS wouldn't
> > get what its users expected to and silently would continue running.
> > Considering that MS intends to use this value for cryptography
> > purposes () it would be security risk.
>
> An aborted guest is very secure but this is going way overboard I think.
> It's easy for guest to detect that the ACPI device is not there,
> whether crashing is the best solution in this case
> is a policy question. In particular something like virtio
> rng looks like an adequate replacement.
related comment is below.
>
>
> > >
> > >
> > > BTW, why do we need to stick vmgen_buf_paddr in the info?
> > Because according to MS spec device should have ADDR object
> > with physical buffer address packed in Package(2). So that
> > Windows could read value from there.
> >
> > [...]
>
> Yes but why not read the property when and where we
> need it?
It's basically to fit the style used in acpi-build.c
where we collect info by reading properties in
acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ...
and then just use pm, misc, pci in build_ssdt()
should we drop all above and just inline it in build_ssdt() ?
>
> > > > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > > > pdev->devfn);
> > > > > > + g_free(last);
> > > > > > + return name;
> > > > > > +}
> > > > >
> > > > > Looks tricky, and duplicates logic for device naming.
> > > > > All this won't be necessary if you just add this as child
> > > > > of the correct device, without playing with scope.
> > > > > Why not do it?
> > > > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > > > fixed PATH to it and we need full path to it to send Notivy from
> > > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
> > >
> > > I see. Still - can't this function return the full aml_name?
> > it's possible but I'd prefer to return back to 2 ACPI devices as it was
> > in v13 since Windows sees 2 devices anyway, even if they merged into one
> > PCI device description (which probably wrong but windows handles it because
> > PCI Standard RAM controller is driver less) and get rid of
> > acpi_get_pci_dev_scope_name() thing.
>
> OK but I think it should be under PCI0 at least,
> since that one claims the relevant resource in its CRS.
vmgenid device doesn't claim any resource if we use PCI for its
implementation since corresponding PCI device claims its BAR.
But I don't see any problem in putting VGID device into PCI0 scope.
>
> > It will also help if vmgenid will be a part of multifunction device,
> > which current build_append_pci_bus_devices() ignores for now (i.e. it
> > describes only function 0 devices on slot).
> >
> > [...]
>
> OK, though we might need to add the description for the pci device anyway
> e.g. in order to mark it hidden.
Experiments show that Windows ignores _STA for PCI devices,
it looks like it completely ignores SXX devices in ACPI for present at boot
devices except of _EJ().
BTW: I've already tried it, it doesn't hide anything.
[...]
> > > > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > > > > *opaque,
> > > > > > + const char *name, Error **errp)
> > > > > > +{
> > > > > > + VmGenIdState *s = VMGENID(obj);
> > > > >
> > > > > Why cast to VMGENID here?
> > > > Yep, there is no need to do it, I'll clean it up.
> > > >
> > > > >
> > > > > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > > > > +
> > > > > > + if (value == PCI_BAR_UNMAPPED) {
> > > > > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > > > > initialized",
> > > > > > + object_get_typename(OBJECT(s)));
> > > > >
> > > > > This is guest error. Pls don't print these to monitor by default.
> > > > Then how test case querying this property via QOM could get to know
> > > > that property is in wrong state yet?
> > >
> > > Maybe leave this around for tests (with a comment)
> > > but use plain pci_get_bar_addr internally?
> > Accessing it internally as property will also allow to
> > prevent guest starting if BIOS failed to initialize BAR
> > (not implemented but shouldn't be hard to do)
> >
> > [...]
>
> I don't think it's a good idea. It's just a device,
> it's not the most important thing for guests,
> it's a policy question whether to initialize it.
I don't think it's just policy, BIOS and ACPI in QEMU are tightly coupled
and if BIOS is unable initialize devices as it's supposed to
then I'd rather abort machine with error message pointing to source
of problem then silently continue boot and allow guest OS or even
some guest application to guess what went wrong if they will be able
to do so.
In this case Windows will continue to work just without using VGID
possibly leading to duplicate keys on to 2 different VMs
or something else (it's used not only for crypto).
Alternatively lets map page directly in QEMU before PCI hole
without any PCI BARs and pass reservation via E820,
- it would solve issue with selecting PCI CLASS ID, it would be just plain QEMU device
- no consuming of slot and/or addrX.functionY
- we would know immediately if device is correctly initialized
even before BIOS runs. i.e. no guest involved and with
clear end result.
> > > > > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > > > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> > > > >
> > > > > Still looks scary.
> > > > Can do nothing about it,
> > > > it's closest class id to what this device is
> > > > (i.e. it exposes page of RAM) that works with Windows
> > > > without asking for drivers.
> > > > If that class id is not acceptable then let's drop PCI
> > > > approach altogether.
> > > >
> > > > More over it's limited to target-i386 only and possibly
> > > > could apply to ARM in the future when Windows comes there,
> > > > so in this case I'm not very concerned about pseries guests
> > >
> > > I don't think we should treat this as a windows only device,
> > > the function seems generally useful.
> > >
> > > > especially with buggy kernel as it was reported in
> > > > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
> > >
> > > I think it's firmware that's confused, not the guest kernel.
> > maybe both, should we care about faulty guest pieces when they
> > don't use this device. If the pseries would need to use it then
> > they should fix guest size instead of poking soldering iron
> > in HW.
>
> It's better if we can avoid the issue altogether.
> Assuming that we can't,
> I'd like some confirmation from David Gibson on this.
>
>
> > >
> > > >
> > > > [...]
> > >
> > >
> > > Some options to think about/try
> > > 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
> > I've already tried this and a number of others,
> > Windows asks for driver.
> >
> > > 2. Name(_HID, "PNP0A06") (or some other id)
> > experiment on Windows shows that _HID doesn't influence PCI devices described
> > in ACPI in any way. In this version _HID = QEMU0003 and its required
> > by VMGID spec to have unique vendor specific HID for VMGID device.
> > It looks like PCI driver mostly ignores PCI slots described in ACPI
> > and as result there are devices in device manager "PCI standard RAM Ctrl"
> > and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
> > tables.
>
> I see. Interesting.
> And VM Gen ID isn't using the resources of the pci
> device?
Nope, resources are claimed by respective PCI device regardless of its
presence in ACPI tables. VGID device just exposes ADDR so that Windows could
poke into that buffer
> Any other ideas? Mark it hidden?
Gal's already checked, Windows doesn't hide VGID device.
I think we should just leave 2 devices as is (i.e. shown), no harm in it.
(I'd hide PCI device but it seems to be impossible, and it's anyway just cosmetic)
[...]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 15:14 ` Igor Mammedov
@ 2015-03-04 15:31 ` Michael S. Tsirkin
2015-03-04 16:33 ` Igor Mammedov
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-03-04 15:31 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, David Gibson, qemu-devel, afaerber
On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote:
> On Wed, 4 Mar 2015 14:49:00 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> > > On Wed, 4 Mar 2015 13:11:48 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >
> > > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > > > >
> > > > > > > Example of using vmgenid device:
> > > > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
>
> [...]
>
> > > > > > Also, how are we going to extend this device?
> > > > > > looks like we've burned it all just for vmid?
> > > > > I don't like the way MS uses yet another side-channel
> > > > > to communicate something (UUID) instead of using ACPI
> > > > > method for getting it.
> > > > > I'd rather avoid extending it beyond of what it's now
> > > > > and use channels that we already have.
> > > >
> > > > Famous last words :)
> > > >
> > > >
> > > > > > How about we have a slightly more generic container
> > > > > > where we'll be able to stick all kind of stuff
> > > > > > in the future, and make vmgenid a child of
> > > > > > this device?
> > > > > What other possible uses do you have in mind?
> > > >
> > > > I don't know for sure - some other value that applications want to map.
> > > Well then suggest something more concrete here I don't
> > > quietly have an idea what
> > > > > > How about we have a slightly more generic container
> > > > > > where we'll be able to stick all kind of stuff
> > > > > > in the future, and make vmgenid a child of
> > > > > > this device?
> > > means, maybe we need a canfcall with Gal to discuss idea?
> >
> > No problem. It's unfortunate we missed the developer conf call this
> > tuesday. If you like, I'll try to setup something for Monday, I'd like
> > to attend too. Anyone else interested?
> Monday is fine for me.
>
> >
> > > >
> > > >
> > > > > > > To change uuid in runtime use:
> > > > > > > qom-set "/machine/peripheral/FOO.uuid"
> > > > > > > "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > > > > >
> > > > > > Looking just at this, how does user discover this functionality?
> > > > > what do you mean?
> > > > >
> > > > > [...]
> > > >
> > > > Just this. You are a user. You want to change the vm gen id.
> > > > Adding devices is partially documented
> > > > in -help and -device help. commands are documented in hmp help.
> > > > But how do you find out that qom-set should be used to
> > > > update vm gen id, and how do you find out how to do this?
> > > I don't really know, it's approach used in original patches.
> > > Any suggestions?
> > > Do QOM properties have some 'help' connected to them? If yes we
> > > could stick explanation there so that -device vmgenid,help
> > > would show it at least.
> >
> > Eric, Andreas, any comments on this part?
> >
> > > >
> > > >
> > > > > > >
> > > > > > > static void acpi_get_misc_info(AcpiMiscInfo *info)
> > > > > > > {
> > > > > > > + Object *obj;
> > > > > > > +
> > > > > > > + obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> > > > > > > + info->vmgen_buf_paddr = 0;
> > > > > > > + if (obj) {
> > > > > > > + info->vmgen_buf_paddr =
> > > > > > > + object_property_get_int(obj, VMGENID_VMGID_ADDR, NULL);
> > > > > >
> > > > > > confused. So what happens if BAR is not mapped by guest?
> > > > > it will get 0 address on acpi_setup() stage but later
> > > > > when ACPI tables are read by BIOS (which happens after PCI is
> > > > > initialized) it will be updated and get mapped address.
> > > >
> > > > Yes but it's up to guest. What if guest does not map BAR
> > > > later, either?
> > > I'd prefer to abort machine since otherwise guest OS wouldn't
> > > get what its users expected to and silently would continue running.
> > > Considering that MS intends to use this value for cryptography
> > > purposes () it would be security risk.
> >
> > An aborted guest is very secure but this is going way overboard I think.
> > It's easy for guest to detect that the ACPI device is not there,
> > whether crashing is the best solution in this case
> > is a policy question. In particular something like virtio
> > rng looks like an adequate replacement.
> related comment is below.
>
> >
> >
> > > >
> > > >
> > > > BTW, why do we need to stick vmgen_buf_paddr in the info?
> > > Because according to MS spec device should have ADDR object
> > > with physical buffer address packed in Package(2). So that
> > > Windows could read value from there.
> > >
> > > [...]
> >
> > Yes but why not read the property when and where we
> > need it?
> It's basically to fit the style used in acpi-build.c
> where we collect info by reading properties in
> acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ...
> and then just use pm, misc, pci in build_ssdt()
> should we drop all above and just inline it in build_ssdt() ?
The issue is you have two items to track here:
- addr - you stick that in the info struct
- full object address - you don't
an inconsistency that I dislike.
> >
> > > > > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > > > > pdev->devfn);
> > > > > > > + g_free(last);
> > > > > > > + return name;
> > > > > > > +}
> > > > > >
> > > > > > Looks tricky, and duplicates logic for device naming.
> > > > > > All this won't be necessary if you just add this as child
> > > > > > of the correct device, without playing with scope.
> > > > > > Why not do it?
> > > > > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > > > > fixed PATH to it and we need full path to it to send Notivy from
> > > > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
> > > >
> > > > I see. Still - can't this function return the full aml_name?
> > > it's possible but I'd prefer to return back to 2 ACPI devices as it was
> > > in v13 since Windows sees 2 devices anyway, even if they merged into one
> > > PCI device description (which probably wrong but windows handles it because
> > > PCI Standard RAM controller is driver less) and get rid of
> > > acpi_get_pci_dev_scope_name() thing.
> >
> > OK but I think it should be under PCI0 at least,
> > since that one claims the relevant resource in its CRS.
> vmgenid device doesn't claim any resource if we use PCI for its
> implementation since corresponding PCI device claims its BAR.
> But I don't see any problem in putting VGID device into PCI0 scope.
>
> >
> > > It will also help if vmgenid will be a part of multifunction device,
> > > which current build_append_pci_bus_devices() ignores for now (i.e. it
> > > describes only function 0 devices on slot).
> > >
> > > [...]
> >
> > OK, though we might need to add the description for the pci device anyway
> > e.g. in order to mark it hidden.
> Experiments show that Windows ignores _STA for PCI devices,
> it looks like it completely ignores SXX devices in ACPI for present at boot
> devices except of _EJ().
> BTW: I've already tried it, it doesn't hide anything.
>
> [...]
So it boils down to the fact that windows thinks it's RAM,
so it binds a generic driver to it, but then we get
lucky and it does not try to use it as RAM.
Is that a fair summary? If yes, to me, it looks exactly like the reverse
side of the pseries issue.
> > > > > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > > > > > *opaque,
> > > > > > > + const char *name, Error **errp)
> > > > > > > +{
> > > > > > > + VmGenIdState *s = VMGENID(obj);
> > > > > >
> > > > > > Why cast to VMGENID here?
> > > > > Yep, there is no need to do it, I'll clean it up.
> > > > >
> > > > > >
> > > > > > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > > > > > +
> > > > > > > + if (value == PCI_BAR_UNMAPPED) {
> > > > > > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > > > > > initialized",
> > > > > > > + object_get_typename(OBJECT(s)));
> > > > > >
> > > > > > This is guest error. Pls don't print these to monitor by default.
> > > > > Then how test case querying this property via QOM could get to know
> > > > > that property is in wrong state yet?
> > > >
> > > > Maybe leave this around for tests (with a comment)
> > > > but use plain pci_get_bar_addr internally?
> > > Accessing it internally as property will also allow to
> > > prevent guest starting if BIOS failed to initialize BAR
> > > (not implemented but shouldn't be hard to do)
> > >
> > > [...]
> >
> > I don't think it's a good idea. It's just a device,
> > it's not the most important thing for guests,
> > it's a policy question whether to initialize it.
> I don't think it's just policy, BIOS and ACPI in QEMU are tightly coupled
> and if BIOS is unable initialize devices as it's supposed to
> then I'd rather abort machine
Right, less work for us :)
But I'm guessing *users* would rather have a debuggable guest.
> with error message pointing to source
> of problem then silently continue boot and allow guest OS or even
> some guest application to guess what went wrong if they will be able
> to do so.
That's still up to guest. BIOS can abort boot if it wants to.
> In this case Windows will continue to work just without using VGID
> possibly leading to duplicate keys on to 2 different VMs
> or something else (it's used not only for crypto).
windows will detect that it does not have VGID.
whether it's worth crashing in that case is up to windows, not us.
> Alternatively lets map page directly in QEMU before PCI hole
> without any PCI BARs and pass reservation via E820,
> - it would solve issue with selecting PCI CLASS ID, it would be just plain QEMU device
> - no consuming of slot and/or addrX.functionY
> - we would know immediately if device is correctly initialized
> even before BIOS runs. i.e. no guest involved and with
> clear end result.
Been there, done that.
Each time we try to steal memory, we get pain.
Guests should allocate memory.
Either via PCI, or linker like Gal's patches do.
> > > > > > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > > > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > > > > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> > > > > >
> > > > > > Still looks scary.
> > > > > Can do nothing about it,
> > > > > it's closest class id to what this device is
> > > > > (i.e. it exposes page of RAM) that works with Windows
> > > > > without asking for drivers.
> > > > > If that class id is not acceptable then let's drop PCI
> > > > > approach altogether.
> > > > >
> > > > > More over it's limited to target-i386 only and possibly
> > > > > could apply to ARM in the future when Windows comes there,
> > > > > so in this case I'm not very concerned about pseries guests
> > > >
> > > > I don't think we should treat this as a windows only device,
> > > > the function seems generally useful.
> > > >
> > > > > especially with buggy kernel as it was reported in
> > > > > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
> > > >
> > > > I think it's firmware that's confused, not the guest kernel.
> > > maybe both, should we care about faulty guest pieces when they
> > > don't use this device. If the pseries would need to use it then
> > > they should fix guest size instead of poking soldering iron
> > > in HW.
> >
> > It's better if we can avoid the issue altogether.
> > Assuming that we can't,
> > I'd like some confirmation from David Gibson on this.
> >
> >
> > > >
> > > > >
> > > > > [...]
> > > >
> > > >
> > > > Some options to think about/try
> > > > 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
> > > I've already tried this and a number of others,
> > > Windows asks for driver.
> > >
> > > > 2. Name(_HID, "PNP0A06") (or some other id)
> > > experiment on Windows shows that _HID doesn't influence PCI devices described
> > > in ACPI in any way. In this version _HID = QEMU0003 and its required
> > > by VMGID spec to have unique vendor specific HID for VMGID device.
> > > It looks like PCI driver mostly ignores PCI slots described in ACPI
> > > and as result there are devices in device manager "PCI standard RAM Ctrl"
> > > and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
> > > tables.
> >
> > I see. Interesting.
> > And VM Gen ID isn't using the resources of the pci
> > device?
> Nope, resources are claimed by respective PCI device regardless of its
> presence in ACPI tables. VGID device just exposes ADDR so that Windows could
> poke into that buffer
>
> > Any other ideas? Mark it hidden?
> Gal's already checked, Windows doesn't hide VGID device.
> I think we should just leave 2 devices as is (i.e. shown), no harm in it.
> (I'd hide PCI device but it seems to be impossible, and it's anyway just cosmetic)
>
> [...]
I agree, what worries me is the driver prompt if we set class to
something generic, and guest confusion if we set it to RAM.
--
MST
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 15:31 ` Michael S. Tsirkin
@ 2015-03-04 16:33 ` Igor Mammedov
2015-03-04 19:12 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-03-04 16:33 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, David Gibson, qemu-devel, afaerber
On Wed, 4 Mar 2015 16:31:39 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote:
> > On Wed, 4 Mar 2015 14:49:00 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> > > > On Wed, 4 Mar 2015 13:11:48 +0100
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > > > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >
> > > > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > > > > >
> > > > > > > > Example of using vmgenid device:
> > > > > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
[...]
> > >
> > > > >
> > > > >
> > > > > BTW, why do we need to stick vmgen_buf_paddr in the info?
> > > > Because according to MS spec device should have ADDR object
> > > > with physical buffer address packed in Package(2). So that
> > > > Windows could read value from there.
> > > >
> > > > [...]
> > >
> > > Yes but why not read the property when and where we
> > > need it?
> > It's basically to fit the style used in acpi-build.c
> > where we collect info by reading properties in
> > acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ...
> > and then just use pm, misc, pci in build_ssdt()
> > should we drop all above and just inline it in build_ssdt() ?
>
> The issue is you have two items to track here:
> - addr - you stick that in the info struct
> - full object address - you don't
> an inconsistency that I dislike.
What is "full object address"?
> > > > > > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > > > > > pdev->devfn);
> > > > > > > > + g_free(last);
> > > > > > > > + return name;
> > > > > > > > +}
> > > > > > >
> > > > > > > Looks tricky, and duplicates logic for device naming.
> > > > > > > All this won't be necessary if you just add this as child
> > > > > > > of the correct device, without playing with scope.
> > > > > > > Why not do it?
> > > > > > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > > > > > fixed PATH to it and we need full path to it to send Notivy from
> > > > > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
> > > > >
> > > > > I see. Still - can't this function return the full aml_name?
> > > > it's possible but I'd prefer to return back to 2 ACPI devices as it was
> > > > in v13 since Windows sees 2 devices anyway, even if they merged into one
> > > > PCI device description (which probably wrong but windows handles it because
> > > > PCI Standard RAM controller is driver less) and get rid of
> > > > acpi_get_pci_dev_scope_name() thing.
> > >
> > > OK but I think it should be under PCI0 at least,
> > > since that one claims the relevant resource in its CRS.
> > vmgenid device doesn't claim any resource if we use PCI for its
> > implementation since corresponding PCI device claims its BAR.
> > But I don't see any problem in putting VGID device into PCI0 scope.
> >
> > >
> > > > It will also help if vmgenid will be a part of multifunction device,
> > > > which current build_append_pci_bus_devices() ignores for now (i.e. it
> > > > describes only function 0 devices on slot).
> > > >
> > > > [...]
> > >
> > > OK, though we might need to add the description for the pci device anyway
> > > e.g. in order to mark it hidden.
> > Experiments show that Windows ignores _STA for PCI devices,
> > it looks like it completely ignores SXX devices in ACPI for present at boot
> > devices except of _EJ().
> > BTW: I've already tried it, it doesn't hide anything.
> >
> > [...]
>
> So it boils down to the fact that windows thinks it's RAM,
It thinks it's PCI Standard RAM Controller not RAM itself.
> so it binds a generic driver to it, but then we get
According to device manager no driver is bound to it and neither needed.
> lucky and it does not try to use it as RAM.
Video cards also use a bunch of "PCI Standard RAM Controller"
devices I guess to expose additional VRAM,
That doesn't mean that BARs are to be used by OS as conventional RAM
it's rather for usage by vendor's driver.
Same goes for ivshmem which is also expose RAM bar and has the same CLASS ID,
BAR's RAM is used only by means of ivshmem driver.
But yes we get lucky that Windows has stub device description.
> Is that a fair summary? If yes, to me, it looks exactly like the reverse
> side of the pseries issue.
>
>
> > > > > > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > > > > > > *opaque,
> > > > > > > > + const char *name, Error **errp)
> > > > > > > > +{
> > > > > > > > + VmGenIdState *s = VMGENID(obj);
> > > > > > >
> > > > > > > Why cast to VMGENID here?
> > > > > > Yep, there is no need to do it, I'll clean it up.
> > > > > >
> > > > > > >
> > > > > > > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > > > > > > +
> > > > > > > > + if (value == PCI_BAR_UNMAPPED) {
> > > > > > > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > > > > > > initialized",
> > > > > > > > + object_get_typename(OBJECT(s)));
> > > > > > >
> > > > > > > This is guest error. Pls don't print these to monitor by default.
> > > > > > Then how test case querying this property via QOM could get to know
> > > > > > that property is in wrong state yet?
> > > > >
> > > > > Maybe leave this around for tests (with a comment)
> > > > > but use plain pci_get_bar_addr internally?
> > > > Accessing it internally as property will also allow to
> > > > prevent guest starting if BIOS failed to initialize BAR
> > > > (not implemented but shouldn't be hard to do)
> > > >
> > > > [...]
> > >
> > > I don't think it's a good idea. It's just a device,
> > > it's not the most important thing for guests,
> > > it's a policy question whether to initialize it.
> > I don't think it's just policy, BIOS and ACPI in QEMU are tightly coupled
> > and if BIOS is unable initialize devices as it's supposed to
> > then I'd rather abort machine
>
> Right, less work for us :)
> But I'm guessing *users* would rather have a debuggable guest.
>
> > with error message pointing to source
> > of problem then silently continue boot and allow guest OS or even
> > some guest application to guess what went wrong if they will be able
> > to do so.
>
> That's still up to guest. BIOS can abort boot if it wants to.
>
>
> > In this case Windows will continue to work just without using VGID
> > possibly leading to duplicate keys on to 2 different VMs
> > or something else (it's used not only for crypto).
>
> windows will detect that it does not have VGID.
> whether it's worth crashing in that case is up to windows, not us.
>
> > Alternatively lets map page directly in QEMU before PCI hole
> > without any PCI BARs and pass reservation via E820,
> > - it would solve issue with selecting PCI CLASS ID, it would be just plain QEMU device
> > - no consuming of slot and/or addrX.functionY
> > - we would know immediately if device is correctly initialized
> > even before BIOS runs. i.e. no guest involved and with
> > clear end result.
>
> Been there, done that.
> Each time we try to steal memory, we get pain.
We are stealing it any way just by using more complex means.
Is there any technical reasons/concerns why direct stealing
from QEMU is bad and why is it better to used PCI/linker?
I'd really know answer instead of getting "just because it's bad".
Gal was also curios regarding this question.
> Guests should allocate memory.
> Either via PCI, or linker like Gal's patches do.
>
>
> > > > > > > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > > > > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > > > > > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> > > > > > >
> > > > > > > Still looks scary.
> > > > > > Can do nothing about it,
> > > > > > it's closest class id to what this device is
> > > > > > (i.e. it exposes page of RAM) that works with Windows
> > > > > > without asking for drivers.
> > > > > > If that class id is not acceptable then let's drop PCI
> > > > > > approach altogether.
> > > > > >
> > > > > > More over it's limited to target-i386 only and possibly
> > > > > > could apply to ARM in the future when Windows comes there,
> > > > > > so in this case I'm not very concerned about pseries guests
> > > > >
> > > > > I don't think we should treat this as a windows only device,
> > > > > the function seems generally useful.
> > > > >
> > > > > > especially with buggy kernel as it was reported in
> > > > > > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
> > > > >
> > > > > I think it's firmware that's confused, not the guest kernel.
> > > > maybe both, should we care about faulty guest pieces when they
> > > > don't use this device. If the pseries would need to use it then
> > > > they should fix guest size instead of poking soldering iron
> > > > in HW.
> > >
> > > It's better if we can avoid the issue altogether.
> > > Assuming that we can't,
> > > I'd like some confirmation from David Gibson on this.
> > >
> > >
> > > > >
> > > > > >
> > > > > > [...]
> > > > >
> > > > >
> > > > > Some options to think about/try
> > > > > 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
> > > > I've already tried this and a number of others,
> > > > Windows asks for driver.
> > > >
> > > > > 2. Name(_HID, "PNP0A06") (or some other id)
> > > > experiment on Windows shows that _HID doesn't influence PCI devices described
> > > > in ACPI in any way. In this version _HID = QEMU0003 and its required
> > > > by VMGID spec to have unique vendor specific HID for VMGID device.
> > > > It looks like PCI driver mostly ignores PCI slots described in ACPI
> > > > and as result there are devices in device manager "PCI standard RAM Ctrl"
> > > > and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
> > > > tables.
> > >
> > > I see. Interesting.
> > > And VM Gen ID isn't using the resources of the pci
> > > device?
> > Nope, resources are claimed by respective PCI device regardless of its
> > presence in ACPI tables. VGID device just exposes ADDR so that Windows could
> > poke into that buffer
> >
> > > Any other ideas? Mark it hidden?
> > Gal's already checked, Windows doesn't hide VGID device.
> > I think we should just leave 2 devices as is (i.e. shown), no harm in it.
> > (I'd hide PCI device but it seems to be impossible, and it's anyway just cosmetic)
> >
> > [...]
>
> I agree, what worries me is the driver prompt if we set class to
If we use PCI_CLASS_MEMORY_RAM it won't ask for driver, even XP.
Lets wait for Davids answer you've asked above.
> something generic, and guest confusion if we set it to RAM.
I don't see guest confusion because it's RAM controller not a RAM.
I've checked Linux sources as well, it also doesn't use this
class id in any way so BAR just sits there.
Perhaps PCI_CLASS_MEMORY_RAM is just bad naming, it really should be
PCI_CLASS_GENERIC_RAM_CONTROLLER if we are to be close to spec.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 16:33 ` Igor Mammedov
@ 2015-03-04 19:12 ` Michael S. Tsirkin
2015-03-05 14:22 ` Igor Mammedov
2015-03-11 5:35 ` David Gibson
0 siblings, 2 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-03-04 19:12 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, David Gibson, qemu-devel, afaerber
On Wed, Mar 04, 2015 at 05:33:42PM +0100, Igor Mammedov wrote:
> On Wed, 4 Mar 2015 16:31:39 +0100
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote:
> > > On Wed, 4 Mar 2015 14:49:00 +0100
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> > > > > On Wed, 4 Mar 2015 13:11:48 +0100
> > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > >
> > > > > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > > > > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > >
> > > > > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > > > > > >
> > > > > > > > > Example of using vmgenid device:
> > > > > > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> [...]
>
> > > >
> > > > > >
> > > > > >
> > > > > > BTW, why do we need to stick vmgen_buf_paddr in the info?
> > > > > Because according to MS spec device should have ADDR object
> > > > > with physical buffer address packed in Package(2). So that
> > > > > Windows could read value from there.
> > > > >
> > > > > [...]
> > > >
> > > > Yes but why not read the property when and where we
> > > > need it?
> > > It's basically to fit the style used in acpi-build.c
> > > where we collect info by reading properties in
> > > acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ...
> > > and then just use pm, misc, pci in build_ssdt()
> > > should we drop all above and just inline it in build_ssdt() ?
> >
> > The issue is you have two items to track here:
> > - addr - you stick that in the info struct
> > - full object address - you don't
> > an inconsistency that I dislike.
> What is "full object address"?
where you look up the vmgen id pci device.
>
> > > > > > > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > > > > > > pdev->devfn);
> > > > > > > > > + g_free(last);
> > > > > > > > > + return name;
> > > > > > > > > +}
> > > > > > > >
> > > > > > > > Looks tricky, and duplicates logic for device naming.
> > > > > > > > All this won't be necessary if you just add this as child
> > > > > > > > of the correct device, without playing with scope.
> > > > > > > > Why not do it?
> > > > > > > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > > > > > > fixed PATH to it and we need full path to it to send Notivy from
> > > > > > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
> > > > > >
> > > > > > I see. Still - can't this function return the full aml_name?
> > > > > it's possible but I'd prefer to return back to 2 ACPI devices as it was
> > > > > in v13 since Windows sees 2 devices anyway, even if they merged into one
> > > > > PCI device description (which probably wrong but windows handles it because
> > > > > PCI Standard RAM controller is driver less) and get rid of
> > > > > acpi_get_pci_dev_scope_name() thing.
> > > >
> > > > OK but I think it should be under PCI0 at least,
> > > > since that one claims the relevant resource in its CRS.
> > > vmgenid device doesn't claim any resource if we use PCI for its
> > > implementation since corresponding PCI device claims its BAR.
> > > But I don't see any problem in putting VGID device into PCI0 scope.
> > >
> > > >
> > > > > It will also help if vmgenid will be a part of multifunction device,
> > > > > which current build_append_pci_bus_devices() ignores for now (i.e. it
> > > > > describes only function 0 devices on slot).
> > > > >
> > > > > [...]
> > > >
> > > > OK, though we might need to add the description for the pci device anyway
> > > > e.g. in order to mark it hidden.
> > > Experiments show that Windows ignores _STA for PCI devices,
> > > it looks like it completely ignores SXX devices in ACPI for present at boot
> > > devices except of _EJ().
> > > BTW: I've already tried it, it doesn't hide anything.
> > >
> > > [...]
> >
> > So it boils down to the fact that windows thinks it's RAM,
> It thinks it's PCI Standard RAM Controller not RAM itself.
>
> > so it binds a generic driver to it, but then we get
> According to device manager no driver is bound to it and neither needed.
>
> > lucky and it does not try to use it as RAM.
> Video cards also use a bunch of "PCI Standard RAM Controller"
> devices I guess to expose additional VRAM,
> That doesn't mean that BARs are to be used by OS as conventional RAM
> it's rather for usage by vendor's driver.
> Same goes for ivshmem which is also expose RAM bar and has the same CLASS ID,
> BAR's RAM is used only by means of ivshmem driver.
>
> But yes we get lucky that Windows has stub device description.
OK. So if you are going to rely on this,
I think it's a good idea to get ack from David to confirm
this is solvable for pseries.
> > Is that a fair summary? If yes, to me, it looks exactly like the reverse
> > side of the pseries issue.
> >
> >
> > > > > > > > > +static void vmgenid_get_vmgid_addr(Object *obj, Visitor *v, void
> > > > > > > > > *opaque,
> > > > > > > > > + const char *name, Error **errp)
> > > > > > > > > +{
> > > > > > > > > + VmGenIdState *s = VMGENID(obj);
> > > > > > > >
> > > > > > > > Why cast to VMGENID here?
> > > > > > > Yep, there is no need to do it, I'll clean it up.
> > > > > > >
> > > > > > > >
> > > > > > > > > + int64_t value = pci_get_bar_addr(PCI_DEVICE(s), 0);
> > > > > > > > > +
> > > > > > > > > + if (value == PCI_BAR_UNMAPPED) {
> > > > > > > > > + error_setg(errp, "'%s." VMGENID_VMGID_ADDR "': not
> > > > > > > > > initialized",
> > > > > > > > > + object_get_typename(OBJECT(s)));
> > > > > > > >
> > > > > > > > This is guest error. Pls don't print these to monitor by default.
> > > > > > > Then how test case querying this property via QOM could get to know
> > > > > > > that property is in wrong state yet?
> > > > > >
> > > > > > Maybe leave this around for tests (with a comment)
> > > > > > but use plain pci_get_bar_addr internally?
> > > > > Accessing it internally as property will also allow to
> > > > > prevent guest starting if BIOS failed to initialize BAR
> > > > > (not implemented but shouldn't be hard to do)
> > > > >
> > > > > [...]
> > > >
> > > > I don't think it's a good idea. It's just a device,
> > > > it's not the most important thing for guests,
> > > > it's a policy question whether to initialize it.
> > > I don't think it's just policy, BIOS and ACPI in QEMU are tightly coupled
> > > and if BIOS is unable initialize devices as it's supposed to
> > > then I'd rather abort machine
> >
> > Right, less work for us :)
> > But I'm guessing *users* would rather have a debuggable guest.
> >
> > > with error message pointing to source
> > > of problem then silently continue boot and allow guest OS or even
> > > some guest application to guess what went wrong if they will be able
> > > to do so.
> >
> > That's still up to guest. BIOS can abort boot if it wants to.
> >
> >
> > > In this case Windows will continue to work just without using VGID
> > > possibly leading to duplicate keys on to 2 different VMs
> > > or something else (it's used not only for crypto).
> >
> > windows will detect that it does not have VGID.
> > whether it's worth crashing in that case is up to windows, not us.
> >
> > > Alternatively lets map page directly in QEMU before PCI hole
> > > without any PCI BARs and pass reservation via E820,
> > > - it would solve issue with selecting PCI CLASS ID, it would be just plain QEMU device
> > > - no consuming of slot and/or addrX.functionY
> > > - we would know immediately if device is correctly initialized
> > > even before BIOS runs. i.e. no guest involved and with
> > > clear end result.
> >
> > Been there, done that.
> > Each time we try to steal memory, we get pain.
> We are stealing it any way just by using more complex means.
Not really. guest reserves memory and gives us the address.
With linker this is standard DMA that happens with a ton of devices.
With pci this is standard BAR mapping.
> Is there any technical reasons/concerns why direct stealing
> from QEMU is bad and why is it better to used PCI/linker?
>
> I'd really know answer instead of getting "just because it's bad".
> Gal was also curios regarding this question.
Simply put reserving RAM by hardware is not something
that happens on real hardware.
Rather it's bios that reserves RAM.
>
> > Guests should allocate memory.
> > Either via PCI, or linker like Gal's patches do.
> >
> >
> > > > > > > > > + k->vendor_id = PCI_VENDOR_ID_REDHAT;
> > > > > > > > > + k->device_id = PCI_DEVICE_ID_REDHAT_VMGENID;
> > > > > > > > > + k->class_id = PCI_CLASS_MEMORY_RAM;
> > > > > > > >
> > > > > > > > Still looks scary.
> > > > > > > Can do nothing about it,
> > > > > > > it's closest class id to what this device is
> > > > > > > (i.e. it exposes page of RAM) that works with Windows
> > > > > > > without asking for drivers.
> > > > > > > If that class id is not acceptable then let's drop PCI
> > > > > > > approach altogether.
> > > > > > >
> > > > > > > More over it's limited to target-i386 only and possibly
> > > > > > > could apply to ARM in the future when Windows comes there,
> > > > > > > so in this case I'm not very concerned about pseries guests
> > > > > >
> > > > > > I don't think we should treat this as a windows only device,
> > > > > > the function seems generally useful.
> > > > > >
> > > > > > > especially with buggy kernel as it was reported in
> > > > > > > http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
> > > > > >
> > > > > > I think it's firmware that's confused, not the guest kernel.
> > > > > maybe both, should we care about faulty guest pieces when they
> > > > > don't use this device. If the pseries would need to use it then
> > > > > they should fix guest size instead of poking soldering iron
> > > > > in HW.
> > > >
> > > > It's better if we can avoid the issue altogether.
> > > > Assuming that we can't,
> > > > I'd like some confirmation from David Gibson on this.
> > > >
> > > >
> > > > > >
> > > > > > >
> > > > > > > [...]
> > > > > >
> > > > > >
> > > > > > Some options to think about/try
> > > > > > 1. PCI_CLASS_MEMORY_OTHER (or some other class?)
> > > > > I've already tried this and a number of others,
> > > > > Windows asks for driver.
> > > > >
> > > > > > 2. Name(_HID, "PNP0A06") (or some other id)
> > > > > experiment on Windows shows that _HID doesn't influence PCI devices described
> > > > > in ACPI in any way. In this version _HID = QEMU0003 and its required
> > > > > by VMGID spec to have unique vendor specific HID for VMGID device.
> > > > > It looks like PCI driver mostly ignores PCI slots described in ACPI
> > > > > and as result there are devices in device manager "PCI standard RAM Ctrl"
> > > > > and "VM Gen ID" despite the fact that it's one Device(SXXX) {} in ACPI
> > > > > tables.
> > > >
> > > > I see. Interesting.
> > > > And VM Gen ID isn't using the resources of the pci
> > > > device?
> > > Nope, resources are claimed by respective PCI device regardless of its
> > > presence in ACPI tables. VGID device just exposes ADDR so that Windows could
> > > poke into that buffer
> > >
> > > > Any other ideas? Mark it hidden?
> > > Gal's already checked, Windows doesn't hide VGID device.
> > > I think we should just leave 2 devices as is (i.e. shown), no harm in it.
> > > (I'd hide PCI device but it seems to be impossible, and it's anyway just cosmetic)
> > >
> > > [...]
> >
> > I agree, what worries me is the driver prompt if we set class to
> If we use PCI_CLASS_MEMORY_RAM it won't ask for driver, even XP.
> Lets wait for Davids answer you've asked above.
>
> > something generic, and guest confusion if we set it to RAM.
> I don't see guest confusion because it's RAM controller not a RAM.
> I've checked Linux sources as well, it also doesn't use this
> class id in any way so BAR just sits there.
>
> Perhaps PCI_CLASS_MEMORY_RAM is just bad naming, it really should be
> PCI_CLASS_GENERIC_RAM_CONTROLLER if we are to be close to spec.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description Igor Mammedov
@ 2015-03-04 19:57 ` Eric Blake
0 siblings, 0 replies; 24+ messages in thread
From: Eric Blake @ 2015-03-04 19:57 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: ghammer, mst
[-- Attachment #1: Type: text/plain, Size: 2928 bytes --]
On 03/03/2015 09:18 AM, Igor Mammedov wrote:
> From: Gal Hammer <ghammer@redhat.com>
>
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
> - ammend doc since no more MMIO occupied by vmgenid
s/ammend/amend/ (but this isn't part of the commit message)
> device and device became PCIDevice
> ---
> docs/specs/vmgenid.txt | 36 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 36 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..718850a
> --- /dev/null
> +++ b/docs/specs/vmgenid.txt
> @@ -0,0 +1,36 @@
> +VIRTUAL MACHINE GENERATION ID
> +=============================
> +
> +Copyright (C) 2014 Red Hat, Inc.
Might also want to claim 2015, as you have redesigned this since 2014.
> +
> +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).
It sounds like management applications (and not qemu) is responsible for
populating the 128 bits with cryptographically random data. That is, if
libvirt fails to put truly random data in the memory via qom-set, the
bug would be in libvirt and not qemu. Correct? If so, should this doc
make it explicit that qemu is leaving it up to management to write
correct data into the memory?
> +
> +This is specified on the web at: http://go.microsoft.com/fwlink/?LinkId=260709
How sure are we that this link won't go stale in the future? Should we
archive a copy of it somewhere on a site we control?
> +
> +---
> +
> +The vmgenid device is a PCI device with the following ACPI ID: "QEMU0003".
> +
> +The device has a "vmgenid.uuid" property, which can be set using
> +the command line argument or the QMP interface.
> +For example:
> +QEMU -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> +
> +Or to change uuid in runtime use:
> + qom-set "/machine/peripheral/FOO.uuid" "124e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> +
> +According to the specification, any change to the GUID executes an
> +ACPI notification. The vmgenid device triggers the \_GPE._E00 handler
> +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.
>
--
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 --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 19:12 ` Michael S. Tsirkin
@ 2015-03-05 14:22 ` Igor Mammedov
2015-03-11 5:35 ` David Gibson
1 sibling, 0 replies; 24+ messages in thread
From: Igor Mammedov @ 2015-03-05 14:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, David Gibson, qemu-devel, afaerber
On Wed, 4 Mar 2015 20:12:31 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Mar 04, 2015 at 05:33:42PM +0100, Igor Mammedov wrote:
> > On Wed, 4 Mar 2015 16:31:39 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote:
> > > > On Wed, 4 Mar 2015 14:49:00 +0100
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> > > > > > On Wed, 4 Mar 2015 13:11:48 +0100
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >
> > > > > > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > > > > > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > >
> > > > > > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > > > > > > >
> > > > > > > > > > Example of using vmgenid device:
> > > > > > > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
[...]
> > > > - no consuming of slot and/or addrX.functionY
> > > > - we would know immediately if device is correctly initialized
> > > > even before BIOS runs. i.e. no guest involved and with
> > > > clear end result.
> > >
> > > Been there, done that.
> > > Each time we try to steal memory, we get pain.
> > We are stealing it any way just by using more complex means.
>
> Not really. guest reserves memory and gives us the address.
> With linker this is standard DMA that happens with a ton of devices.
Let's see how "not really complex" is in case of linker
1. QEMU creates VGID bugger entry in linker so BIOS could allocate
that buffer in conventional RAM -> reducing available RAM on 1 page
2. QEMU creates ACPI description for VGID device with fake address
value that should be patched to VGID buffer address by linker in
BIOS when it loads SSDT table.
3. BIOS linker loads linker table and allocates VGID buffer
(and adds space occupied by buffer in into E820 table as E820_RESERVED),
then it loads SSDT table and patches VGID.ADDR object with
address of allocated VGID buffer.
4. QEMU the needs to know VGID address as well so it could write
UUID in that buffer, for that we introduce MMIO interface
(consuming resources in limited IO address space) so that
ACPI code could write back VGID buffer address.
5. We wait until guest OS loads and executes VGID.INIT() method
which writes in MMIO ports VGID.ADDR address which linker patched
at #3 step so that QEMU would know where VGID buffer is to write UUID.
6. When QEMU gets VGID buffer address from OS it writes there
UUID and sends GPE0._E00 notification which notifies guest that
there is a new/updated UUID in VGID buffer
7. Guest OS can query VGID.ADDR for getting access to UUID at
that address
benefits of approach:
there is no additional state that should be migrated since buffer
in initial RAM provided by QEMU and migrated along with it.
drawbacks:
- resource wise:
- guest's RAM size is reduced on 1 page
- IO address space reduced on 4 bytes
- Device is not in correct state until guest OS is booted
and executes VGID.INIT ACPI method.
- too complex for what needs to be done:
- harder to maintain since maintainer would have to
know/recall how it works when considering changes to this code
- difficult to debug loop QEMU -> BIOS (3)-> OSPM(5) -> QEMU
if something went wrong there
> With pci this is standard BAR mapping.
with PCI device it's quite a bit simpler:
1. QEMU creates PCI device with not mapped into guest address-space RAM BAR
which is VGID buffer and writes UUID into it
2. BIOS initializes PCI devices and maps VGID BAR somewhere in PCI hole
(i.e. VGID RAM buffer is allocated by QEMU and doesn't reduce initial RAM)
3. BIOS loads ACPI tables from QEMU, at that time QEMU generates them
taking mapped address of VGID BAR directly from VGID PCI device to create
VGID.ADDR ACPI object
4. Guest OS boots and can query VGID.ADDR for getting access to UUID at
that address
benefits of approach:
- resource wise:
- doesn't consume IO address space ports
- quite a bit less complex than BIOS linker approach
- incorrect device state lasts only till BIOS initializes PCI devices
i.e. guest OS is not involved.
drawbacks:
- resource wise:
- consumes a PCI function at least
- still has device in incorrect state until BIOS maps
VGID's provided RAM somewhere
- debugging is a bit easier since it contained to QEMU+BIOS only:
QEMU -> BIOS(maps bar) -> QEMU(update ACPI tables) -> BIOS(load SSDT)
- unit test should wait till BIOS initializes PCI devices to start tests
- migration stream will have extra state to carry device's VGID buffer
>
> > Is there any technical reasons/concerns why direct stealing
> > from QEMU is bad and why is it better to used PCI/linker?
> >
> > I'd really know answer instead of getting "just because it's bad".
> > Gal was also curios regarding this question.
>
> Simply put reserving RAM by hardware is not something
> that happens on real hardware.
That's not really technical reason against direct mapping in QEMU,
if that were ACPI table wouldn't be ever moved to QEMU since
it's job of BIOS. But for reducing complexity and to avoid
split brain issues between QEMU and BIOS, ACPI generation was moved
into QEMU.
> Rather it's bios that reserves RAM.
The same applies in this case, it's not RAM reservation since
it's Device backed RAM block and it doesn't interfere with
conventional RAM memory in any way.
It's exactly the same like QEMU allocates and maps low and high RAM
regions at certain addresses.
Compared to PCI approach the difference would be that it's not BIOS
selecting where to map VGID's RAM but rather a QEMU itself deciding
where to map VGID's bar.
So steps with direct mapping would look like:
1. QEMU creates a device, maps device's VGID buffer into guest address-space
and writes UUID into it
2. BIOS loads ACPI tables from QEMU with correctly set VGID.ADDR ACPI object
since address is known in advance even before BIOS runs.
3. Guest OS boots and can query VGID.ADDR for getting access to UUID at
that address
benefits of this approach over PCI one:
- device is in correct state once it's realized, i.e. even before guest runs
- there is no need to worry which PCI CLASS ID to choose so that Windows wouldn't
ask for non existing driver or confuse stub PCI device with something else
- simple straightforward design which doesn't depend on guest side, easy to debug
and understand
- unit test doesn't need to execute guest code at all, we could actually switch
accel=tcg to qtest in it.
- we don't even need a separate device for it, it could be a feature/property
of chipset/board
drawbacks:
- migration stream will have extra state to carry device's VGID buffer
- you name it
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-04 19:12 ` Michael S. Tsirkin
2015-03-05 14:22 ` Igor Mammedov
@ 2015-03-11 5:35 ` David Gibson
2015-03-19 9:50 ` Michael S. Tsirkin
1 sibling, 1 reply; 24+ messages in thread
From: David Gibson @ 2015-03-11 5:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, Igor Mammedov, qemu-devel, afaerber
[-- Attachment #1: Type: text/plain, Size: 6489 bytes --]
On Wed, 4 Mar 2015 20:12:31 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Mar 04, 2015 at 05:33:42PM +0100, Igor Mammedov wrote:
> > On Wed, 4 Mar 2015 16:31:39 +0100
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Wed, Mar 04, 2015 at 04:14:44PM +0100, Igor Mammedov wrote:
> > > > On Wed, 4 Mar 2015 14:49:00 +0100
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > On Wed, Mar 04, 2015 at 02:12:32PM +0100, Igor Mammedov wrote:
> > > > > > On Wed, 4 Mar 2015 13:11:48 +0100
> > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > >
> > > > > > > On Tue, Mar 03, 2015 at 09:33:51PM +0100, Igor Mammedov wrote:
> > > > > > > > On Tue, 3 Mar 2015 18:35:39 +0100
> > > > > > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > >
> > > > > > > > > On Tue, Mar 03, 2015 at 05:18:14PM +0100, Igor Mammedov 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 "vmgenid.uuid" property.
> > > > > > > > > >
> > > > > > > > > > Example of using vmgenid device:
> > > > > > > > > > -device vmgenid,id=FOO,uuid="324e6eaf-d1d1-4bf6-bf41-b9bb6c91fb87"
> > [...]
> >
> > > > >
> > > > > > >
> > > > > > >
> > > > > > > BTW, why do we need to stick vmgen_buf_paddr in the info?
> > > > > > Because according to MS spec device should have ADDR object
> > > > > > with physical buffer address packed in Package(2). So that
> > > > > > Windows could read value from there.
> > > > > >
> > > > > > [...]
> > > > >
> > > > > Yes but why not read the property when and where we
> > > > > need it?
> > > > It's basically to fit the style used in acpi-build.c
> > > > where we collect info by reading properties in
> > > > acpi_get_pm_info(), acpi_get_misc_info(), acpi_get_pci_info() ...
> > > > and then just use pm, misc, pci in build_ssdt()
> > > > should we drop all above and just inline it in build_ssdt() ?
> > >
> > > The issue is you have two items to track here:
> > > - addr - you stick that in the info struct
> > > - full object address - you don't
> > > an inconsistency that I dislike.
> > What is "full object address"?
>
> where you look up the vmgen id pci device.
>
> >
> > > > > > > > > > + name = g_strdup_printf("PCI0%s.S%.02X_", name ? name : "",
> > > > > > > > > > pdev->devfn);
> > > > > > > > > > + g_free(last);
> > > > > > > > > > + return name;
> > > > > > > > > > +}
> > > > > > > > >
> > > > > > > > > Looks tricky, and duplicates logic for device naming.
> > > > > > > > > All this won't be necessary if you just add this as child
> > > > > > > > > of the correct device, without playing with scope.
> > > > > > > > > Why not do it?
> > > > > > > > since vmgenid PCI device is located somewhere on PCI bus we don't have
> > > > > > > > fixed PATH to it and we need full path to it to send Notivy from
> > > > > > > > "\\_GPE" scope see "aml_notify(aml_name("\\_SB.%s", vgid_path)" below.
> > > > > > >
> > > > > > > I see. Still - can't this function return the full aml_name?
> > > > > > it's possible but I'd prefer to return back to 2 ACPI devices as it was
> > > > > > in v13 since Windows sees 2 devices anyway, even if they merged into one
> > > > > > PCI device description (which probably wrong but windows handles it because
> > > > > > PCI Standard RAM controller is driver less) and get rid of
> > > > > > acpi_get_pci_dev_scope_name() thing.
> > > > >
> > > > > OK but I think it should be under PCI0 at least,
> > > > > since that one claims the relevant resource in its CRS.
> > > > vmgenid device doesn't claim any resource if we use PCI for its
> > > > implementation since corresponding PCI device claims its BAR.
> > > > But I don't see any problem in putting VGID device into PCI0 scope.
> > > >
> > > > >
> > > > > > It will also help if vmgenid will be a part of multifunction device,
> > > > > > which current build_append_pci_bus_devices() ignores for now (i.e. it
> > > > > > describes only function 0 devices on slot).
> > > > > >
> > > > > > [...]
> > > > >
> > > > > OK, though we might need to add the description for the pci device anyway
> > > > > e.g. in order to mark it hidden.
> > > > Experiments show that Windows ignores _STA for PCI devices,
> > > > it looks like it completely ignores SXX devices in ACPI for present at boot
> > > > devices except of _EJ().
> > > > BTW: I've already tried it, it doesn't hide anything.
> > > >
> > > > [...]
> > >
> > > So it boils down to the fact that windows thinks it's RAM,
> > It thinks it's PCI Standard RAM Controller not RAM itself.
> >
> > > so it binds a generic driver to it, but then we get
> > According to device manager no driver is bound to it and neither needed.
> >
> > > lucky and it does not try to use it as RAM.
> > Video cards also use a bunch of "PCI Standard RAM Controller"
> > devices I guess to expose additional VRAM,
> > That doesn't mean that BARs are to be used by OS as conventional RAM
> > it's rather for usage by vendor's driver.
> > Same goes for ivshmem which is also expose RAM bar and has the same CLASS ID,
> > BAR's RAM is used only by means of ivshmem driver.
> >
> > But yes we get lucky that Windows has stub device description.
>
> OK. So if you are going to rely on this,
> I think it's a good idea to get ack from David to confirm
> this is solvable for pseries.
I've looked into this a bit more. We've confirmed it's definitely a
bug in SLOF - but fixing it is a bit more subtle than I thought.
Basically, SLOF is setting the device_type property for all PCI devices
based on the PCI class code - it's device_type = "memory" that causes
the kernel to erroneously pick up the PCI device as regular RAM.
In fact, device_type is supposed to indicate the capabilities of the OF
driver attached to the device, so it should only be set by an actual OF
driver binding to the device, not generically in the PCI code.
The catch is whether we'll break any existing SLOF supported devices is
we remove setting of the device_type. This will need some testing.
--
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-11 5:35 ` David Gibson
@ 2015-03-19 9:50 ` Michael S. Tsirkin
2015-03-19 11:31 ` Gal Hammer
2015-03-20 4:58 ` David Gibson
0 siblings, 2 replies; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-03-19 9:50 UTC (permalink / raw)
To: David Gibson; +Cc: ghammer, Igor Mammedov, qemu-devel, afaerber
On Wed, Mar 11, 2015 at 04:35:41PM +1100, David Gibson wrote:
> > > > So it boils down to the fact that windows thinks it's RAM,
> > > It thinks it's PCI Standard RAM Controller not RAM itself.
> > >
> > > > so it binds a generic driver to it, but then we get
> > > According to device manager no driver is bound to it and neither needed.
> > >
> > > > lucky and it does not try to use it as RAM.
> > > Video cards also use a bunch of "PCI Standard RAM Controller"
> > > devices I guess to expose additional VRAM,
> > > That doesn't mean that BARs are to be used by OS as conventional RAM
> > > it's rather for usage by vendor's driver.
> > > Same goes for ivshmem which is also expose RAM bar and has the same CLASS ID,
> > > BAR's RAM is used only by means of ivshmem driver.
> > >
> > > But yes we get lucky that Windows has stub device description.
> >
> > OK. So if you are going to rely on this,
> > I think it's a good idea to get ack from David to confirm
> > this is solvable for pseries.
>
> I've looked into this a bit more. We've confirmed it's definitely a
> bug in SLOF - but fixing it is a bit more subtle than I thought.
>
> Basically, SLOF is setting the device_type property for all PCI devices
> based on the PCI class code - it's device_type = "memory" that causes
> the kernel to erroneously pick up the PCI device as regular RAM.
>
> In fact, device_type is supposed to indicate the capabilities of the OF
> driver attached to the device, so it should only be set by an actual OF
> driver binding to the device, not generically in the PCI code.
>
> The catch is whether we'll break any existing SLOF supported devices is
> we remove setting of the device_type. This will need some testing.
I guess we can look for some other IDs to use, as well.
Host pci bridge class binds to NO_DRV too:
class 0x0604. So that's one other option.
There are also many devices for which windows won't require a driver.
For example, Intel, taken at random:
2620 E8500/E8501 eXternal Memory Bridge
277c 82975X Memory Controller Hub
3600 7300 Chipset Memory Controller Hub
Are we more, or less likely to see problems
with one of these?
It seems hard to decide, either way.
> --
> David Gibson <dgibson@redhat.com>
> Senior Software Engineer, Virtualization, Red Hat
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-19 9:50 ` Michael S. Tsirkin
@ 2015-03-19 11:31 ` Gal Hammer
2015-03-20 4:58 ` David Gibson
1 sibling, 0 replies; 24+ messages in thread
From: Gal Hammer @ 2015-03-19 11:31 UTC (permalink / raw)
To: Michael S. Tsirkin, David Gibson; +Cc: Igor Mammedov, qemu-devel, afaerber
On 19/03/2015 11:50, Michael S. Tsirkin wrote:
> On Wed, Mar 11, 2015 at 04:35:41PM +1100, David Gibson wrote:
>>>>> So it boils down to the fact that windows thinks it's RAM,
>>>> It thinks it's PCI Standard RAM Controller not RAM itself.
>>>>
>>>>> so it binds a generic driver to it, but then we get
>>>> According to device manager no driver is bound to it and neither needed.
>>>>
>>>>> lucky and it does not try to use it as RAM.
>>>> Video cards also use a bunch of "PCI Standard RAM Controller"
>>>> devices I guess to expose additional VRAM,
>>>> That doesn't mean that BARs are to be used by OS as conventional RAM
>>>> it's rather for usage by vendor's driver.
>>>> Same goes for ivshmem which is also expose RAM bar and has the same CLASS ID,
>>>> BAR's RAM is used only by means of ivshmem driver.
>>>>
>>>> But yes we get lucky that Windows has stub device description.
>>>
>>> OK. So if you are going to rely on this,
>>> I think it's a good idea to get ack from David to confirm
>>> this is solvable for pseries.
>>
>> I've looked into this a bit more. We've confirmed it's definitely a
>> bug in SLOF - but fixing it is a bit more subtle than I thought.
>>
>> Basically, SLOF is setting the device_type property for all PCI devices
>> based on the PCI class code - it's device_type = "memory" that causes
>> the kernel to erroneously pick up the PCI device as regular RAM.
>>
>> In fact, device_type is supposed to indicate the capabilities of the OF
>> driver attached to the device, so it should only be set by an actual OF
>> driver binding to the device, not generically in the PCI code.
>>
>> The catch is whether we'll break any existing SLOF supported devices is
>> we remove setting of the device_type. This will need some testing.
>
> I guess we can look for some other IDs to use, as well.
> Host pci bridge class binds to NO_DRV too:
> class 0x0604. So that's one other option.
>
> There are also many devices for which windows won't require a driver.
> For example, Intel, taken at random:
> 2620 E8500/E8501 eXternal Memory Bridge
> 277c 82975X Memory Controller Hub
> 3600 7300 Chipset Memory Controller Hub
> Are we more, or less likely to see problems
> with one of these?
>
> It seems hard to decide, either way.
I think it is wrong to have a fake PCI device just to avoid the fact
that Windows will show the device in the devices tree. If the device
doesn't implement the required interface, then it should impersonate to
be one.
The PCI device implementation might be a simpler/cleaner solution but it
can cause more problems if an application or a driver will try to access
the device and get might get an access to the GUID memory region.
Gal.
>> --
>> David Gibson <dgibson@redhat.com>
>> Senior Software Engineer, Virtualization, Red Hat
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device
2015-03-19 9:50 ` Michael S. Tsirkin
2015-03-19 11:31 ` Gal Hammer
@ 2015-03-20 4:58 ` David Gibson
1 sibling, 0 replies; 24+ messages in thread
From: David Gibson @ 2015-03-20 4:58 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, Igor Mammedov, qemu-devel, afaerber
[-- Attachment #1: Type: text/plain, Size: 3158 bytes --]
On Thu, 19 Mar 2015 10:50:49 +0100
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Mar 11, 2015 at 04:35:41PM +1100, David Gibson wrote:
> > > > > So it boils down to the fact that windows thinks it's RAM,
> > > > It thinks it's PCI Standard RAM Controller not RAM itself.
> > > >
> > > > > so it binds a generic driver to it, but then we get
> > > > According to device manager no driver is bound to it and neither needed.
> > > >
> > > > > lucky and it does not try to use it as RAM.
> > > > Video cards also use a bunch of "PCI Standard RAM Controller"
> > > > devices I guess to expose additional VRAM,
> > > > That doesn't mean that BARs are to be used by OS as conventional RAM
> > > > it's rather for usage by vendor's driver.
> > > > Same goes for ivshmem which is also expose RAM bar and has the same CLASS ID,
> > > > BAR's RAM is used only by means of ivshmem driver.
> > > >
> > > > But yes we get lucky that Windows has stub device description.
> > >
> > > OK. So if you are going to rely on this,
> > > I think it's a good idea to get ack from David to confirm
> > > this is solvable for pseries.
> >
> > I've looked into this a bit more. We've confirmed it's definitely a
> > bug in SLOF - but fixing it is a bit more subtle than I thought.
> >
> > Basically, SLOF is setting the device_type property for all PCI devices
> > based on the PCI class code - it's device_type = "memory" that causes
> > the kernel to erroneously pick up the PCI device as regular RAM.
> >
> > In fact, device_type is supposed to indicate the capabilities of the OF
> > driver attached to the device, so it should only be set by an actual OF
> > driver binding to the device, not generically in the PCI code.
> >
> > The catch is whether we'll break any existing SLOF supported devices is
> > we remove setting of the device_type. This will need some testing.
>
> I guess we can look for some other IDs to use, as well.
> Host pci bridge class binds to NO_DRV too:
> class 0x0604. So that's one other option.
Fwiw, some further investigation suggests that removing the bogus
device_type setting should be safer than we initially feared. I am
planning to merge the SLOF change downstream just as soon as I get a
chance.
So, pseries shouldn't be a barrier to this.
> There are also many devices for which windows won't require a driver.
> For example, Intel, taken at random:
> 2620 E8500/E8501 eXternal Memory Bridge
> 277c 82975X Memory Controller Hub
> 3600 7300 Chipset Memory Controller Hub
> Are we more, or less likely to see problems
> with one of these?
>
> It seems hard to decide, either way.
The current class 0x0500 device does have the advantage of sorts that
there isn't really a specification of what precisely it's supposed to
do (i.e. what the programming interface is). If any of those other
examples *do* require a specific programming interface, then
advertising as one without implementing that interface would be worse
than the current class 0x0500 approach.
--
David Gibson <dgibson@redhat.com>
Senior Software Engineer, Virtualization, Red Hat
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID
2015-03-03 16:18 [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Igor Mammedov
` (2 preceding siblings ...)
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 3/3] tests: add a unit test for the vmgenid device Igor Mammedov
@ 2015-04-15 10:38 ` Michael S. Tsirkin
2015-04-15 13:59 ` Igor Mammedov
3 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-15 10:38 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, Yan Vugenfirer, qemu-devel
On Tue, Mar 03, 2015 at 05:18:12PM +0100, Igor Mammedov wrote:
> Changes since v13:
> * fix comment style to /*... */ in testcase
> * make BAR TARGET_PAGE_SIZE as required by spec
> * make BAR prefetchable, spec also says that it shouldn't be
> marked as non cached
> * ACPI part
> * merge separate VGID device with PCI device description
> * mark device as not shown in UI,
> it hides "VM Generation ID" device from device manager
> and leaves only "PCI Standard RAM Controller" device there
In an offline chat, Yan (Cc) mentioned that with windows guests,
PCI rebalancing can move BAR values around.
Yan, could you comment on-list please?
Is there a way to force this, for testing?
ACPI spec mentions this:
If a platform uses a PCI BAR Target operation region, an ACPI OS will
not load a native device driver for the associated PCI function. For
example, if any of the BARs in a PCI function are associated with a PCI
BAR Target operation region, then the OS will assume that the PCI
function is to be entirely under the control of the ACPI BIOS. No driver
will be loaded. Thus, a PCI function can be used as a platform
controller for some task (hot-plug PCI, and so on) that the ACPI BIOS
performs.
This might also avoid driver prompt from windows?
--
MST
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID
2015-04-15 10:38 ` [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Michael S. Tsirkin
@ 2015-04-15 13:59 ` Igor Mammedov
2015-04-19 7:52 ` Michael S. Tsirkin
0 siblings, 1 reply; 24+ messages in thread
From: Igor Mammedov @ 2015-04-15 13:59 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, Yan Vugenfirer, qemu-devel
On Wed, 15 Apr 2015 12:38:57 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Tue, Mar 03, 2015 at 05:18:12PM +0100, Igor Mammedov wrote:
> > Changes since v13:
> > * fix comment style to /*... */ in testcase
> > * make BAR TARGET_PAGE_SIZE as required by spec
> > * make BAR prefetchable, spec also says that it shouldn't be
> > marked as non cached
> > * ACPI part
> > * merge separate VGID device with PCI device description
> > * mark device as not shown in UI,
> > it hides "VM Generation ID" device from device manager
> > and leaves only "PCI Standard RAM Controller" device there
>
> In an offline chat, Yan (Cc) mentioned that with windows guests,
> PCI rebalancing can move BAR values around.
> Yan, could you comment on-list please?
> Is there a way to force this, for testing?
>
> ACPI spec mentions this:
> If a platform uses a PCI BAR Target operation region, an ACPI OS will
> not load a native device driver for the associated PCI function. For
> example, if any of the BARs in a PCI function are associated with a PCI
> BAR Target operation region, then the OS will assume that the PCI
> function is to be entirely under the control of the ACPI BIOS. No driver
> will be loaded. Thus, a PCI function can be used as a platform
> controller for some task (hot-plug PCI, and so on) that the ACPI BIOS
> performs.
It seems that WS2012R2 doesn't honor this part of spec,
it still tries to find matching driver and load it.
>
> This might also avoid driver prompt from windows?
it isn't.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID
2015-04-15 13:59 ` Igor Mammedov
@ 2015-04-19 7:52 ` Michael S. Tsirkin
2015-04-19 13:38 ` Yan Vugenfirer
0 siblings, 1 reply; 24+ messages in thread
From: Michael S. Tsirkin @ 2015-04-19 7:52 UTC (permalink / raw)
To: Igor Mammedov; +Cc: ghammer, Yan Vugenfirer, qemu-devel
On Wed, Apr 15, 2015 at 03:59:09PM +0200, Igor Mammedov wrote:
> On Wed, 15 Apr 2015 12:38:57 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Tue, Mar 03, 2015 at 05:18:12PM +0100, Igor Mammedov wrote:
> > > Changes since v13:
> > > * fix comment style to /*... */ in testcase
> > > * make BAR TARGET_PAGE_SIZE as required by spec
> > > * make BAR prefetchable, spec also says that it shouldn't be
> > > marked as non cached
> > > * ACPI part
> > > * merge separate VGID device with PCI device description
> > > * mark device as not shown in UI,
> > > it hides "VM Generation ID" device from device manager
> > > and leaves only "PCI Standard RAM Controller" device there
> >
> > In an offline chat, Yan (Cc) mentioned that with windows guests,
> > PCI rebalancing can move BAR values around.
> > Yan, could you comment on-list please?
> > Is there a way to force this, for testing?
> >
> > ACPI spec mentions this:
> > If a platform uses a PCI BAR Target operation region, an ACPI OS will
> > not load a native device driver for the associated PCI function. For
> > example, if any of the BARs in a PCI function are associated with a PCI
> > BAR Target operation region, then the OS will assume that the PCI
> > function is to be entirely under the control of the ACPI BIOS. No driver
> > will be loaded. Thus, a PCI function can be used as a platform
> > controller for some task (hot-plug PCI, and so on) that the ACPI BIOS
> > performs.
> It seems that WS2012R2 doesn't honor this part of spec,
> it still tries to find matching driver and load it.
Interesting. Looking at msdn:
https://msdn.microsoft.com/en-us/library/windows/hardware/ff563877(v=vs.85).aspx
Looks like what windows does to rebalance is query device for
rebalancing support, stop driver, rebalance, and restart driver.
If device has no driver, it seems likely windows will assume
it's safe to rebalance the resources.
There's also a very old document:
http://www.microsoft.com/whdc/connect/pci/PCI-rsc.mspx
which says it's possible to implement _DSM method to deny rebalance
requests.
Alternatively, we could put this device behind a pci extender, e.g. as
implemented by Marcel. The resources would then be limited by the _CRS
or the root.
> >
> > This might also avoid driver prompt from windows?
> it isn't.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID
2015-04-19 7:52 ` Michael S. Tsirkin
@ 2015-04-19 13:38 ` Yan Vugenfirer
0 siblings, 0 replies; 24+ messages in thread
From: Yan Vugenfirer @ 2015-04-19 13:38 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: ghammer, Igor Mammedov, qemu-devel
----- Original Message -----
> From: "Michael S. Tsirkin" <mst@redhat.com>
> To: "Igor Mammedov" <imammedo@redhat.com>
> Cc: ghammer@redhat.com, "Yan Vugenfirer" <yvugenfi@redhat.com>, qemu-devel@nongnu.org
> Sent: Sunday, April 19, 2015 10:52:37 AM
> Subject: Re: [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID
>
> On Wed, Apr 15, 2015 at 03:59:09PM +0200, Igor Mammedov wrote:
> > On Wed, 15 Apr 2015 12:38:57 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Tue, Mar 03, 2015 at 05:18:12PM +0100, Igor Mammedov wrote:
> > > > Changes since v13:
> > > > * fix comment style to /*... */ in testcase
> > > > * make BAR TARGET_PAGE_SIZE as required by spec
> > > > * make BAR prefetchable, spec also says that it shouldn't be
> > > > marked as non cached
> > > > * ACPI part
> > > > * merge separate VGID device with PCI device description
> > > > * mark device as not shown in UI,
> > > > it hides "VM Generation ID" device from device manager
> > > > and leaves only "PCI Standard RAM Controller" device there
> > >
> > > In an offline chat, Yan (Cc) mentioned that with windows guests,
> > > PCI rebalancing can move BAR values around.
> > > Yan, could you comment on-list please?
> > > Is there a way to force this, for testing?
> > >
Yes. It is part of HCK test kit:
https://msdn.microsoft.com/en-us/library/windows/hardware/jj673015(v=vs.85).aspx#About_rebalance_tests
> > > ACPI spec mentions this:
> > > If a platform uses a PCI BAR Target operation region, an ACPI OS will
> > > not load a native device driver for the associated PCI function. For
> > > example, if any of the BARs in a PCI function are associated with a PCI
> > > BAR Target operation region, then the OS will assume that the PCI
> > > function is to be entirely under the control of the ACPI BIOS. No driver
> > > will be loaded. Thus, a PCI function can be used as a platform
> > > controller for some task (hot-plug PCI, and so on) that the ACPI BIOS
> > > performs.
> > It seems that WS2012R2 doesn't honor this part of spec,
> > it still tries to find matching driver and load it.
>
> Interesting. Looking at msdn:
> https://msdn.microsoft.com/en-us/library/windows/hardware/ff563877(v=vs.85).aspx
> Looks like what windows does to rebalance is query device for
> rebalancing support, stop driver, rebalance, and restart driver.
>
> If device has no driver, it seems likely windows will assume
> it's safe to rebalance the resources.
>
> There's also a very old document:
> http://www.microsoft.com/whdc/connect/pci/PCI-rsc.mspx
> which says it's possible to implement _DSM method to deny rebalance
> requests.
> Alternatively, we could put this device behind a pci extender, e.g. as
> implemented by Marcel. The resources would then be limited by the _CRS
> or the root.
>
> > >
> > > This might also avoid driver prompt from windows?
> > it isn't.
>
>
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2015-04-19 13:38 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-03 16:18 [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Igor Mammedov
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 1/3] docs: vm generation id device's description Igor Mammedov
2015-03-04 19:57 ` Eric Blake
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 2/3] pc: add a Virtual Machine Generation ID device Igor Mammedov
2015-03-03 17:35 ` Michael S. Tsirkin
2015-03-03 20:33 ` Igor Mammedov
2015-03-04 12:11 ` Michael S. Tsirkin
2015-03-04 13:12 ` Igor Mammedov
2015-03-04 13:49 ` Michael S. Tsirkin
2015-03-04 14:03 ` Andreas Färber
2015-03-04 15:14 ` Igor Mammedov
2015-03-04 15:31 ` Michael S. Tsirkin
2015-03-04 16:33 ` Igor Mammedov
2015-03-04 19:12 ` Michael S. Tsirkin
2015-03-05 14:22 ` Igor Mammedov
2015-03-11 5:35 ` David Gibson
2015-03-19 9:50 ` Michael S. Tsirkin
2015-03-19 11:31 ` Gal Hammer
2015-03-20 4:58 ` David Gibson
2015-03-03 16:18 ` [Qemu-devel] [PATCH V14 3/3] tests: add a unit test for the vmgenid device Igor Mammedov
2015-04-15 10:38 ` [Qemu-devel] [PATCH V14 0/3] Virtual Machine Generation ID Michael S. Tsirkin
2015-04-15 13:59 ` Igor Mammedov
2015-04-19 7:52 ` Michael S. Tsirkin
2015-04-19 13:38 ` Yan Vugenfirer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.