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

[Gal Hammer <ghammer@redhat.com>]

On 27/04/2015 16:38, Michael S. Tsirkin wrote:
> 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 <ghammer@redhat.com>
>> ---
>>   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.

Comments are not evil, you know. But it should be fixed because the 
addresses can be 64-bit in this solution.

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

I think I did it because it is not possible to create a sysbus device 
when using the -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 <ghammer@redhat.com>
>> + *
>> + * 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.

Your comment on the doc patch said that I should remove the reference to 
LE because UUID is just a buffer, so I don't understand this comment.

The code works in my tests so I assume there is no problem with the 
initialization of the value.

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

And your alternative is...?

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

Do you mean that I should use memory_region_init_ram?

>> +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 <ghammer@redhat.com>
>> + *
>> + * 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.

OK.

>
>> +#define VMGENID_BASE_ADDR_LEN   16
>
>
> Spec says this must not be in same 4K with uncacheable memory.
> How do you guarantee this?

I assume that if the device is using part of a page (and the current 
device's address is page-aligned) then the OS won't use that page. And 
an unused page is uncacheable.

>> +
>> +#endif
>> --
>> 2.1.0

     Gal.