Re: [Qemu-devel] [PATCH V13 3/4] pc: add a Virtual Machine Generation ID device

[Michael S. Tsirkin]

On Wed, Feb 25, 2015 at 05:08:52PM +0000, 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"

If you do this, doesn't windows then prompt for a driver?


> 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 <address@hidden>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> 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               |  33 ++++++++++
>  hw/i386/acpi-dsdt.dsl              |   2 -
>  hw/i386/q35-acpi-dsdt.dsl          |   2 -
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/vmgenid.c                  | 130 
> +++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/acpi.h             |   1 +
>  include/hw/misc/vmgenid.h          |  21 ++++++
>  include/hw/pci/pci.h               |   1 +
>  11 files changed, 190 insertions(+), 4 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 4d5d7e3..9290ae3 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;
>      DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> @@ -245,6 +247,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();
> @@ -981,6 +991,29 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>      sb_scope = aml_scope("_SB");
>      {
> +        if (misc->vmgen_buf_paddr) {
> +            dev = aml_device("VMGI");
> +
> +            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")));
> +
> +            pkg = aml_package(2);
> +            /* low 32 bits of UUID buffer addr*/
> +            aml_append(pkg, aml_int(misc->vmgen_buf_paddr));
> +            aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr 
> */

Really should be full 64 bit, and use a 64 bit BAR.


> +            aml_append(dev, aml_name_decl("ADDR", pkg));
> +
> +            aml_append(sb_scope, dev);
> +
> +            scope = aml_scope("\\_GPE");
> +            method = aml_method("_E00", 0);
> +            aml_append(method,
> +                aml_notify(aml_name("\\_SB.VMGI"), aml_int(0x80)));
> +            aml_append(scope, method);
> +            aml_append(ssdt, scope);
> +        }
> +

This confuses me.
Shouldn't VMGI device be on PCI bus, where it was
added?



>          /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO 
> */
>          dev = aml_device("PCI0." stringify(CPU_HOTPLUG_RESOURCE_DEVICE));
>          aml_append(dev, aml_name_decl("_HID", aml_eisaid("PNP0A06")));
> diff --git a/hw/i386/acpi-dsdt.dsl b/hw/i386/acpi-dsdt.dsl
> index a611e07..884038b 100644
> --- a/hw/i386/acpi-dsdt.dsl
> +++ b/hw/i386/acpi-dsdt.dsl
> @@ -306,8 +306,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 e1cee5d..9eb794c 100644
> --- a/hw/i386/q35-acpi-dsdt.dsl
> +++ b/hw/i386/q35-acpi-dsdt.dsl
> @@ -414,8 +414,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..631c9a3
> --- /dev/null
> +++ b/hw/misc/vmgenid.c
> @@ -0,0 +1,130 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2014 Red Hat Inc.
> + *
> + *  Authors: Gal Hammer <address@hidden>
> + *           Igor Mammedov <address@hidden>
> + *
> + * 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;
> +    uint8_t guid[16];
> +    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)));

Looks wrong - this is guest configuration.
I don't think we want to print errors and exit in this way just
because guest did not map the device.
Better to mask it in acpi.


> +        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),
> +                           &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_SPACE_MEMORY, 
> &s->iomem);

This means this BAR is 16 bytes in size?

Not good, should be full 4K.

You also make the BAR non prefetcheable, which makes
some OS-es (e.g. old linux) map it non-cacheable.






> +    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 <address@hidden>
> + *           Igor Mammedov <address@hidden>
> + *
> + * 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
> -- 
> 1.8.3.1