From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35306) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEPCT-0003Fj-Hv for qemu-devel@nongnu.org; Thu, 22 Jan 2015 16:22:02 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YEPCO-0004bm-Gr for qemu-devel@nongnu.org; Thu, 22 Jan 2015 16:22:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:34222) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEPCO-0004bi-86 for qemu-devel@nongnu.org; Thu, 22 Jan 2015 16:21:56 -0500 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 (8.14.4/8.14.4) with ESMTP id t0MLLsAA031008 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Thu, 22 Jan 2015 16:21:54 -0500 Date: Thu, 22 Jan 2015 23:21:51 +0200 From: "Michael S. Tsirkin" Message-ID: <20150122212151.GE30185@redhat.com> References: <1418745044-3986-1-git-send-email-ghammer@redhat.com> <1418745044-3986-3-git-send-email-ghammer@redhat.com> <20150122145246.5d020fab@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150122145246.5d020fab@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH V11 2/3] 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 Thu, Jan 22, 2015 at 02:52:46PM +0100, Igor Mammedov wrote: > On Tue, 16 Dec 2014 17:50:43 +0200 > 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 > > > > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -257,6 +257,7 @@ static void acpi_get_pci_info(PcPciInfo *info) > > #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > > #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > > +#define ACPI_BUILD_VMGENID_FILE "etc/vm-generation-id" > > > > static void > > build_header(GArray *linker, GArray *table_data, > > @@ -1068,6 +1069,8 @@ build_ssdt(GArray *table_data, GArray *linker, > > { > > MachineState *machine = MACHINE(qdev_get_machine()); > > uint32_t nr_mem = machine->ram_slots; > > + uint32_t vm_gid_physical_address; > > + uint32_t vm_gid_offset = 0; > > unsigned acpi_cpus = guest_info->apic_id_limit; > > int ssdt_start = table_data->len; > > uint8_t *ssdt_ptr; > > @@ -1096,6 +1099,21 @@ build_ssdt(GArray *table_data, GArray *linker, > > ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > ssdt_isa_pest[0], 16, misc->pvpanic_port); > > > > + if (vm_generation_id_set()) { > > + vm_gid_physical_address = ssdt_start + ssdt_acpi_vm_gid_addr[0]; > > + bios_linker_loader_alloc(linker, ACPI_BUILD_VMGENID_FILE, 8, true); > > + bios_linker_loader_add_pointer(linker, ACPI_BUILD_VMGENID_FILE, > > + ACPI_BUILD_TABLE_FILE, > > + table_data, > > + &vm_gid_offset, > > + sizeof(vm_gid_offset)); > could some explain how this pointer magic works, > From my weak understanding it seems broken. > Lets see: > > [1] &vm_gid_offset - must be pointer inside of dest_file blob (ACPI_BUILD_VMGENID_FILE) > [2] vm_gid_offset - should hold offset of the place inside of src_file > (ACPI_BUILD_TABLE_FILE) where to pointer inside of dest_file should point to > > now: > vm_gid_physical_address - holds [2] i.e. offset of VGIA constant in inside SSDT in ACPI_BUILD_TABLE_FILE. > > > + ACPI_BUILD_SET_LE(ssdt_ptr, sizeof(ssdp_misc_aml), > > + ssdt_acpi_vm_gid_addr[0], 32, vm_gid_physical_address); > Then we write this offset into VGIA in ACPI_BUILD_TABLE_FILE. > > After BIOS loads tables it's going to patch at > [3] ACPI_BUILD_VMGENID_FILE + (&vm_gid_offset - table_data->data) /* only god knows where it will be/ > > and on top of it write in it value: > *(ACPI_BUILD_TABLE_FILE + *[3]) Too late today - I'll need to re-review this, will do next week. > This approach in general of patching arbitrary place in AML blob But I think this isn't very different from other pointers we have though. > to get PHY addr of buffer with UUID, is quite a hack, especially > in light of that we are trying to hide all direct access to AML > blobs with related pointer arithmetic and manual patching. > > Why not reserve some potion of RAM and pass to BIOS/guest > a reservation so it won't be part of AddressRangeMemory or > AddressRangeACPI as MS spec requires? Then you won't need > jump all above hoops to just get buffer's PHY addr. Yea, look at the pain we are in each time we try. Allocating by guest seems cleaner. > > > [...] > > typedef > > @@ -1790,6 +1811,11 @@ void acpi_setup(PcGuestInfo *guest_info) > > fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE, > > tables.tcpalog->data, acpi_data_len(tables.tcpalog)); > > > > + /* Add a 128-bit fw cfg file which stores the VM generation id. */ > > + g_array_set_size(tables.vmgenid, 16); > > + fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_VMGENID_FILE, > > + tables.vmgenid->data, tables.vmgenid->len); > shouldn't it be migratable? /i.e. acpi_add_rom_blob(...)/ Not necessarily because there's nothing there: it's only purpose in life is to make guest allocate and zero out memory. -- MST