From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36640) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0BaL-0002jQ-72 for qemu-devel@nongnu.org; Wed, 03 Jun 2015 12:32:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0BaF-0002zn-6C for qemu-devel@nongnu.org; Wed, 03 Jun 2015 12:32:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:51029) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0BaE-0002zR-VK for qemu-devel@nongnu.org; Wed, 03 Jun 2015 12:32:03 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 9DBCD33D8C9 for ; Wed, 3 Jun 2015 16:32:02 +0000 (UTC) Date: Wed, 3 Jun 2015 18:32:00 +0200 From: "Michael S. Tsirkin" Message-ID: <20150603183107-mutt-send-email-mst@redhat.com> References: <1430133591-6197-1-git-send-email-ghammer@redhat.com> <1430133591-6197-5-git-send-email-ghammer@redhat.com> <20150529134644.67bbfd27@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150529134644.67bbfd27@nial.brq.redhat.com> 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: Igor Mammedov Cc: Gal Hammer , qemu-devel@nongnu.org On Fri, May 29, 2015 at 01:46:44PM +0200, Igor Mammedov wrote: > On Mon, 27 Apr 2015 14:19:50 +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) { > would be better to make it conditional depending on if vmgenid device is present > > > + scope = aml_scope("\\_SB"); > > + > > + dev = aml_device("VMGI"); > > [...] > > > + > > + vmgenid = qdev_try_create(NULL, VMGENID_DEVICE); > > + if (vmgenid) { > > + qdev_init_nofail(vmgenid); > > + sysbus_mmio_map(SYS_BUS_DEVICE(vmgenid), 0, VMGENID_BASE_ADDRESS); > > + } > it should be possible not to create device if user haven't asked for it, > my guess is that you are using sysbus device as a convenience for mapping. > > I'd suggest to use plain Device type instead, with it > vmgenid would be created when management asks for it using CLI: > -device vmgenid,uuid=XXXX > and from mgmt side it would treat device as any other device using the same APIs. > > Mapping device to board is also simple, for example see: > > pc_machine_device_plug_cb()->pc_dimm_plug()->memory_region_add_subregion() > > for PC machine pc_machine_device_plug_cb() callback is called for > every bus-less device during device_realize() time > hw/core/qdev.c:hotplug_handler_plug(hotplug_ctrl, dev, &local_err); > > you just need to add something like: > pc_vmgenid_plug() > addr = object_property_get_int(dev, VMGENID_BASE_ADDR_PROP, &local_err); > VMGenIdDeviceClass vmgidc = CAST(dev) > memory_region_add_subregion(&system_memory, addr, vmgidc->get_memory_region()); > > [...] > Yes, I think it is best not to add devices by default, -device is safer. > > + > > +#define VMGENID_BASE_ADDRESS 0xfedf0000 > I'd make it a property, so that every target would be able to set > its own value when times come and ACPI part would automatically pick it up, > it also helps with test cases. I think there's no need for this change as we plan to modify this code down the road anyway. > > +#define VMGENID_BASE_ADDR_LEN 16 > > + > > +#endif