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

[Laszlo Ersek]

On 03/02/15 18:05, Igor Mammedov wrote:
> 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.

UEFI doing crazy stuff? Never. :)

But, I need more context. Is this a new PCI device with class code
PCI_CLASS_MEMORY_RAM? I grepped edk2 for it, and for
PCI_CLASS_MEMORY_CONTROLLER too. Only the macro definitions are there, I
don't see them used anywhere.

If I wanted to add different kinds of RAM to the system, I'd have to
produce new memory resource descriptor HOBs (hand-off blocks) during
PEI, or call gDS->AddMemorySpace() during DXE. Since I won't do that, I
expect the addition of just another PCI device to QEMU will simply go
unnoticed in OVMF (with there being no particular driver for it in OVMF).

So, unless I misunderstood something, this should be safe to do in qemu.

Thanks
Laszlo

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