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

[Igor Mammedov]

On Sun, 1 Mar 2015 16:09:33 +0100
"Michael S. Tsirkin" <address@hidden> wrote:

> 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?
it doesn't since PCI_CLASS_MEMORY_RAM is displayed as driver less 
"PCI standard RAM Controller" binding in device manager.

There was an issue with
 virtio balloon device + pseries firmware + kernel bug
 http://lists.gnu.org/archive/html/qemu-devel/2012-03/msg04704.html
but it shouldn't be an issue for x86 targets with which device is
supposed to be used.
CCing David and Laszlo in case UEFI might do some crazy stuff like
pseries firmware.

> 
> 
> > 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.
for compatibility with 32 windows guest, it probably should stay below
4Gb address space.

> 
> 
> > +            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?
I'll try to merge it with PCI slot description that associated with this device,
Though I'm not sure that windows would handle it correctly.

> 
> 
> 
> >          /* 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.
that would mean that user asked to run guest with VMGID but BIOS failed
to do so for some reason and error was just ignored.
I think it's better to abort guest at early start-up stage
i.e. when it loads ACPI tables, than proceed with invalid state
silently.

> 
> 
> > +        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.
yep, it should be page

> 
> You also make the BAR non prefetcheable, which makes
> some OS-es (e.g. old linux) map it non-cacheable.
we don't care about old linux here because it doesn't have
vmgenid driver neither a modern linux but at least it could
be added to it.
But there shouldn't be a issue with tagging it as prefetchable.


> 
> 
> 
> 
> 
> 
> > +    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
>