From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49650) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRMQ-0001nN-Ta for qemu-devel@nongnu.org; Wed, 29 Apr 2015 08:45:12 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YnRMK-0000PQ-B3 for qemu-devel@nongnu.org; Wed, 29 Apr 2015 08:45:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YnRMJ-0000PL-Q3 for qemu-devel@nongnu.org; Wed, 29 Apr 2015 08:45:00 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 5ABD98E3D9 for ; Wed, 29 Apr 2015 12:44:59 +0000 (UTC) Message-ID: <5540D296.8050104@redhat.com> Date: Wed, 29 Apr 2015 15:46:14 +0300 From: Gal Hammer MIME-Version: 1.0 References: <1430133591-6197-1-git-send-email-ghammer@redhat.com> <1430133591-6197-5-git-send-email-ghammer@redhat.com> <20150427152606-mutt-send-email-mst@redhat.com> In-Reply-To: <20150427152606-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH V15 4/5] i386: add a Virtual Machine Generation ID device List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: imammedo@redhat.com, qemu-devel@nongnu.org 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 >> --- >> 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 >> + * >> + * 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 >> + * >> + * 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.