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

[Igor Mammedov]

On Tue, 16 Jun 2015 17:11:02 +0300
Gal Hammer <address@hidden> wrote:

> Based on Microsoft's specifications (paper can be downloaded 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 a global "vmgenid.uuid" parameter.
-device vmgenid,uuid=FOO
or
some QMP FOO

> 
> Signed-off-by: Gal Hammer <address@hidden>
> ---
>  default-configs/i386-softmmu.mak   |   1 +
>  default-configs/x86_64-softmmu.mak |   1 +
>  hw/i386/acpi-build.c               |  32 +++++++++
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/vmgenid.c                  | 140
> +++++++++++++++++++++++++++++++++++++
> include/hw/i386/pc.h               |   3 + 6 files changed, 178
> insertions(+) create mode 100644 hw/misc/vmgenid.c
> 
> diff --git a/default-configs/i386-softmmu.mak
> b/default-configs/i386-softmmu.mak index 91d602c..3c2fda8 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -48,3 +48,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/default-configs/x86_64-softmmu.mak
> b/default-configs/x86_64-softmmu.mak index 62575eb..36e654d 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -49,3 +49,4 @@ CONFIG_MEM_HOTPLUG=y
>  CONFIG_XIO3130=y
>  CONFIG_IOH3420=y
>  CONFIG_I82801B11=y
> +CONFIG_VMGENID=y
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index b71e942..784e23d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -112,6 +112,7 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
>      uint16_t applesmc_io_base;
> +    uint64_t vm_generation_addr;
>  } AcpiMiscInfo;
>  
>  typedef struct AcpiBuildPciBusHotplugState {
> @@ -238,6 +239,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      info->tpm_version = tpm_get_version();
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
> +    info->vm_generation_addr = vm_generation_addr();
>  }
>  
>  /*
> @@ -1128,6 +1130,36 @@ build_ssdt(GArray *table_data, GArray *linker,
>      }
>  
>      sb_scope = aml_scope("\\_SB");
move ^^ inside of if(misc->vm_generation_addr) block

> +    if (misc->vm_generation_addr) {
like this:
+          sb_scope = aml_scope("\\_SB"); // <= scope comes
> +        dev = aml_device("VMGI");
...
> +
> +        aml_append(sb_scope, dev);
> +        aml_append(ssdt, sb_scope);
                                ^^^ scope goes away

here you've added sb_scope to ssdt and later it continues to be
extended with PCI0.PRES and other devices. it most likely will cause a
mess in the table.
Try to verify by running "make check" to see what changes you've added.


> +
> +        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);
> +    }
leave original line as it was:
  sb_scope = aml_scope("\\_SB");
>      {
>          /* create PCI0.PRES device and its _CRS to reserve CPU
> hotplug MMIO */ dev = aml_device("PCI0."
> stringify(CPU_HOTPLUG_RESOURCE_DEVICE)); diff --git
> a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs index
> 4aa76ff..3db11de 100644 --- a/hw/misc/Makefile.objs
> +++ b/hw/misc/Makefile.objs
> @@ -40,3 +40,4 @@ obj-$(CONFIG_STM32F2XX_SYSCFG) += stm32f2xx_syscfg.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..4484952
> --- /dev/null
> +++ b/hw/misc/vmgenid.c
> @@ -0,0 +1,140 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2015 Red Hat Inc.
> + *
> + *  Authors: Gal Hammer <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/acpi/acpi_dev_interface.h"
> +
> +#define VMGENID_DEVICE "vmgenid"
> +
> +/* Use the memory address which follows HPET for no special reason.
> */ +#define VMGENID_DEFAULT_ADDRESS 0xfedf0000
> +
> +#define PROPERTY_ADDR "addr"
> +#define PROPERTY_UUID "uuid"
> +
> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj),
> VMGENID_DEVICE) +
> +typedef struct VmGenIdState {
> +    DeviceState parent_obj;
> +    MemoryRegion iomem;
> +    uint64_t addr;
> +    uint8_t guid[16];
> +    bool guid_set;
> +} VmGenIdState;
> +
> +uint64_t vm_generation_addr(void)
> +{
> +    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    if (!obj) {
> +        return 0;
> +    }
> +    return s->addr;
> +}
> +
> +static void update_guest(VmGenIdState *s, bool notify)
> +{
> +    void *ptr = memory_region_get_ram_ptr(&s->iomem);
> +    Object *acpi_obj;
> +
> +    memcpy(ptr, &s->guid, sizeof(s->guid));
> +    memory_region_set_dirty(&s->iomem, 0, sizeof(s->guid));
> +
> +    if (notify) {
> +        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);
> +
> +           adevc->vm_generation_id_changed(adev);
> +        }
else {
   error_setg(errp, "Not supported");
}

so mgmt would know that it's doing something wrong

> +     }
> +}
> +
> +static void vmgenid_set_uuid(Object *obj, const char *value, Error
> **errp) +{
> +    VmGenIdState *s = VMGENID(obj);
> +    bool first_set = !s->guid_set;
> +
> +    if (qemu_uuid_parse(value, s->guid) < 0) {
> +        error_setg(errp, "Fail to parse UUID string.");
> +        return;
> +    }
> +    s->guid_set = true;
> +
> +    /* Skip the acpi notification when setting the vm generation id
> for the
> +     * first time. This is done because in a q35 machine the gpe
> register is
> +     * allocated after the device is initialized.
which device is initialized, my thought was that all onboard devices
are initialised/realised first (including ICH9 with it's GPE register)
and only then -device CLI options are parsed.

> +     */
> +    update_guest(s, !first_set);
> +}
> +
> +static void vmgenid_realize(DeviceState *qdev, Error **errp)
> +{
> +    VmGenIdState *s = VMGENID(qdev);
> +
> +    if (!s->guid_set) {
> +        error_setg(errp, "Missing virtual machine generation ID.");
> +        return;
> +    }
> +
> +    if (s->addr % 8 != 0) {
> +        error_setg(errp, "Address must be 8-byte aligned.");
> +        return;
> +    }
> +
> +    vmstate_register_ram(&s->iomem, DEVICE(s));
> +    memory_region_add_subregion(get_system_memory(), s->addr,
> &s->iomem); +}
> +
> +static void vmgenid_init(Object *obj)
> +{
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    object_property_add_str(obj, PROPERTY_UUID, NULL,
> vmgenid_set_uuid, NULL); +
> +    memory_region_init_ram(&s->iomem, OBJECT(s), "vm-generation-id",
There were concerns about using RAM since it's going to be in
migration stream as separate entry. Suggestion was to use
MMIO instead so replace memory_region_init_ram() with
memory_region_init_io()
and that also alleviates need in dirtying updated memory.
 

> 0x1000,
> +        &error_abort);
> +}
> +
> +static Property vmgenid_properties[] = {
> +    DEFINE_PROP_UINT64(PROPERTY_ADDR, VmGenIdState, addr,
> +                       VMGENID_DEFAULT_ADDRESS),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void vmgenid_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    dc->realize = vmgenid_realize;
> +    dc->props = vmgenid_properties;
> +    dc->hotpluggable = false;
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo vmgenid_device_info = {
> +    .name          = VMGENID_DEVICE,
> +    .parent        = TYPE_DEVICE,
> +    .instance_size = sizeof(VmGenIdState),
> +    .instance_init = vmgenid_init,
> +    .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/i386/pc.h b/include/hw/i386/pc.h
> index 86c5651..9492e32 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -281,6 +281,9 @@ void pc_system_firmware_init(MemoryRegion
> *rom_memory, /* pvpanic.c */
>  uint16_t pvpanic_port(void);
>  
> +/* vmgenid.c */
> +uint64_t vm_generation_addr(void);
> +
>  /* e820 types */
>  #define E820_RAM        1
>  #define E820_RESERVED   2