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

[Michael S. Tsirkin]

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 <address@hidden>
> Signed-off-by: Igor Mammedov <address@hidden>
> ---
> 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 <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;
> +    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 <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"

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