From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53211) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIGo1-0001xI-Pn for qemu-devel@nongnu.org; Mon, 02 Feb 2015 08:12:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIGnt-0000kS-D3 for qemu-devel@nongnu.org; Mon, 02 Feb 2015 08:12:42 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49512) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIGnt-0000kE-6V for qemu-devel@nongnu.org; Mon, 02 Feb 2015 08:12:37 -0500 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 (8.14.4/8.14.4) with ESMTP id t12DCZWf010443 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL) for ; Mon, 2 Feb 2015 08:12:36 -0500 Message-ID: <54CF7803.7020906@redhat.com> Date: Mon, 02 Feb 2015 15:13:39 +0200 From: Gal Hammer MIME-Version: 1.0 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> <54CE227A.9020000@redhat.com> <20150202134635.434233f7@nial.brq.redhat.com> In-Reply-To: <20150202134635.434233f7@nial.brq.redhat.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit 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: qemu-devel@nongnu.org, mst@redhat.com On 02/02/2015 14:46, Igor Mammedov wrote: > On Sun, 01 Feb 2015 14:56:26 +0200 > Gal Hammer wrote: > >> On 22/01/2015 15:52, 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, >> >> I can try, but don't you think that a magic is gone once explained? ;-) >> >>> 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 >> >> The vm_gid_offset should point where in the ACPI_BUILD_VMGENID_FILE the >> VM's GUID is stored. At the moment, it should always be zero because the >> GUID is stored at the begging of the ACPI_BUILD_VMGENID_FILE. >> >>> >>> 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. >> >> Yes. This offset is later patched by the linker to the full physical >> address. >> >>> 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]) >> >> We know exactly where it is, no need to call for god's help :-). >> >>> This approach in general of patching arbitrary place in AML blob >>> 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. >> >> I'll be glad to hear a new idea that I didn't already try in one of >> other previous patches. The problem is that the specification requires >> working with a physical address, so it must be allocated from inside the >> guest. Since the OS is not exist in this stage and I also don't want to >> write a special driver just to allocate this buffer I had to choose this >> approach. > how about creating device which will map 4K MMIO region in PCI hole > address space and passing it as a reservation via e820 table we have in QEMU. > Then address could be directly built in ACPI tables as constant value > at the time of ACPI tables creation. Isn't this will cause a VMEXIT when the guest is reading the GUID? If it is then this idea was already presented and Michael didn't approve it. > That way it would be possible to get address of buffer without > firmware + guest OS doing anything and going through quite complex > chain for getting buffer address (qemu->bios->OSPM->qemu). > If you go current route, it would be needed to teach linker a new command > to make reservation in E820 so that allocated buffer won't be part of > of AddressRangeMemory as required by spec or anything else. > Which would make already hard to understand/use correctly linker API > even more complex. > > >> >>>> >>> [...] >>>> 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(...)/ >>> >> >> I'm not too familiar with the migration process, but I assume that this >> memory will be copied as part of the guest memory. >> >> Gal. >> >