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

[Michael S. Tsirkin]

On Mon, Apr 27, 2015 at 02:19:50PM +0300, Gal Hammer 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 a global "vmgenid.uuid" parameter.
> 
> 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               |  41 +++++++++++++
>  hw/i386/pc.c                       |   8 +++
>  hw/misc/Makefile.objs              |   1 +
>  hw/misc/vmgenid.c                  | 116 
> +++++++++++++++++++++++++++++++++++++
>  include/hw/i386/pc.h               |   3 +
>  include/hw/misc/vmgenid.h          |  21 +++++++
>  8 files changed, 192 insertions(+)
>  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 6a74e00..9fc0a3c 100644
> --- a/default-configs/i386-softmmu.mak
> +++ b/default-configs/i386-softmmu.mak
> @@ -45,3 +45,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 46b87dd..f66fd3c 100644
> --- a/default-configs/x86_64-softmmu.mak
> +++ b/default-configs/x86_64-softmmu.mak
> @@ -45,3 +45,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 e761005..8d1a761 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -42,6 +42,7 @@
>  #include "hw/acpi/memory_hotplug.h"
>  #include "sysemu/tpm.h"
>  #include "hw/acpi/tpm.h"
> +#include "hw/misc/vmgenid.h"
>  
>  /* Supported chipsets: */
>  #include "hw/acpi/piix4.h"
> @@ -116,6 +117,7 @@ typedef struct AcpiMiscInfo {
>      unsigned dsdt_size;
>      uint16_t pvpanic_port;
>      uint16_t applesmc_io_base;
> +    bool vm_generation_id_set;
>  } AcpiMiscInfo;
>  
>  typedef struct AcpiBuildPciBusHotplugState {
> @@ -242,6 +244,7 @@ static void acpi_get_misc_info(AcpiMiscInfo *info)
>      info->has_tpm = tpm_find();
>      info->pvpanic_port = pvpanic_port();
>      info->applesmc_io_base = applesmc_port();
> +    info->vm_generation_id_set = vm_generation_id_set();
>  }
>  
>  static void acpi_get_pci_info(PcPciInfo *info)
> @@ -815,6 +818,44 @@ build_ssdt(GArray *table_data, GArray *linker,
>          aml_append(ssdt, scope);
>      }
>  
> +    if (misc->vm_generation_id_set) {
> +        scope = aml_scope("\\_SB");
> +
> +        dev = aml_device("VMGI");
> +        aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> +        aml_append(dev,
> +            aml_name_decl("_CID", aml_string("VM_Gen_Counter")));
> +        aml_append(dev,
> +            aml_name_decl("_DDN", aml_string("VM_Gen_Counter")));
> +
> +        crs = aml_resource_template();
> +        aml_append(crs, aml_memory32_fixed(aml_ReadOnly,
> +            VMGENID_BASE_ADDRESS, VMGENID_BASE_ADDR_LEN));
> +        aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +        aml_append(dev, aml_name_decl("_STA", aml_int(0xF)));
> +
> +        method = aml_method("ADDR", 0);
> +        pkg = aml_package(2);
> +        aml_append(pkg, aml_int(VMGENID_BASE_ADDRESS));
> +        aml_append(pkg, aml_int(0)); /* high 32 bits of UUID buffer addr */

So VMGENID_BASE_ADDRESS >> 32 then, won't need a comment.

> +        aml_append(method, aml_store(pkg, aml_local(0)));
> +        aml_append(method, aml_return(aml_local(0)));
> +        aml_append(dev, method);
> +
> +        aml_append(scope, dev);
> +        aml_append(ssdt, scope);
> +
> +        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);
> +    }
> +
>      sb_scope = aml_scope("_SB");
>      {
>          /* create PCI0.PRES device and its _CRS to reserve CPU hotplug MMIO 
> */
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index a8e6be1..266c5f3 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -63,6 +63,7 @@
>  #include "hw/pci/pci_host.h"
>  #include "acpi-build.h"
>  #include "hw/mem/pc-dimm.h"
> +#include "hw/misc/vmgenid.h"
>  #include "trace.h"
>  #include "qapi/visitor.h"
>  #include "qapi-visit.h"
> @@ -1402,6 +1403,7 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
> *gsi,
>      int i;
>      DriveInfo *fd[MAX_FD];
>      DeviceState *hpet = NULL;
> +    DeviceState *vmgenid;
>      int pit_isa_irq = 0;
>      qemu_irq pit_alt_irq = NULL;
>      qemu_irq rtc_irq = NULL;
> @@ -1491,6 +1493,12 @@ void pc_basic_device_init(ISABus *isa_bus, qemu_irq 
> *gsi,
>          fd[i] = drive_get(IF_FLOPPY, 0, i);
>      }
>      *floppy = fdctrl_init_isa(isa_bus, fd);
> +
> +    vmgenid = qdev_try_create(NULL, VMGENID_DEVICE);
> +    if (vmgenid) {
> +        qdev_init_nofail(vmgenid);
> +        sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS);
> +    }
>  }
>  
>  void pc_nic_init(ISABus *isa_bus, PCIBus *pci_bus)

Please leave pc_basic_device_init alone.  This is legacy stuff, new
devices should be added with -device parameter.


> 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..01aacd4
> --- /dev/null
> +++ b/hw/misc/vmgenid.c
> @@ -0,0 +1,116 @@
> +/*
> + *  Virtual Machine Generation ID Device
> + *
> + *  Copyright (C) 2014 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/sysbus.h"
> +#include "hw/misc/vmgenid.h"
> +#include "hw/acpi/acpi_dev_interface.h"
> +
> +#define PROPERTY_UUID "uuid"
> +
> +#define VMGENID(obj) OBJECT_CHECK(VmGenIdState, (obj), VMGENID_DEVICE)
> +
> +typedef struct VmGenIdState {
> +    SysBusDevice parent_obj;
> +    MemoryRegion iomem;
> +    uint8_t guid[16];
> +    bool guid_set;
> +} VmGenIdState;
> +
> +bool vm_generation_id_set(void)
> +{
> +    Object *obj = object_resolve_path_type("", VMGENID_DEVICE, NULL);
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    if (!obj) {
> +        return false;
> +    }
> +    return s->guid_set;
> +}
> +
> +static uint64_t vmgenid_ram_read(void *opaque, hwaddr addr,
> +                                 unsigned size)
> +{
> +    VmGenIdState *s = VMGENID(opaque);
> +    uint64_t value;
> +
> +    memcpy(&value, s->guid + addr, size);
> +    return value;
> +}

I tried to think through what this does on BE platforms
and got a headache. It seems clear not all of value
is initialized here, though.

> +
> +static const MemoryRegionOps vmgenid_ram_ops = {
> +    .read = vmgenid_ram_read,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4,
> +    },
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +};
> +

DEVICE_NATIVE_ENDIAN is generally evil, and best avoided.
I think you should make it a RAM region, not an IO region. Otherwise the
spec text that wants to enable caching for it seems useless as caching
has no effect on io regions.  Will also take care of migrating this,
which you currently don't.

> +static void vmgenid_set_uuid(Object *obj, const char *value, Error **errp)
> +{
> +    VmGenIdState *s = VMGENID(obj);
> +    Object *acpi_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. */

/* Always
 * Like this
 */

/*
 * or
 * Like this
 */

/* never
 * Like this */


> +    if (!first_set) {
> +        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);
> +        }
> +    }
> +}
> +
> +static void vmgenid_init(Object *obj)
> +{
> +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> +    VmGenIdState *s = VMGENID(obj);
> +
> +    memory_region_init_io(&s->iomem, obj, &vmgenid_ram_ops, s, "vgid", 16);
> +    sysbus_init_mmio(sbd, &s->iomem);
> +
> +    object_property_add_str(obj, PROPERTY_UUID, NULL, vmgenid_set_uuid, 
> NULL);
> +}
> +
> +static void vmgenid_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> +}
> +
> +static const TypeInfo vmgenid_device_info = {
> +    .name          = VMGENID_DEVICE,
> +    .parent        = TYPE_SYS_BUS_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 1b35168..9df3a9f 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -284,6 +284,9 @@ void pc_system_firmware_init(MemoryRegion *rom_memory,
>  /* pvpanic.c */
>  uint16_t pvpanic_port(void);
>  
> +/* vmgenid.c */
> +bool vm_generation_id_set(void);
> +
>  /* e820 types */
>  #define E820_RAM        1
>  #define E820_RESERVED   2
> diff --git a/include/hw/misc/vmgenid.h b/include/hw/misc/vmgenid.h
> new file mode 100644
> index 0000000..3d44421
> --- /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>
> + *
> + * 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_BASE_ADDRESS    0xfedf0000

Please do not leave the reader to guess where
did this magic number come from.
This needs to be documented.

> +#define VMGENID_BASE_ADDR_LEN   16


Spec says this must not be in same 4K with uncacheable memory.
How do you guarantee this?

> +
> +#endif
> -- 
> 2.1.0