On Wed, 4 Feb 2015 10:09:38 -0500 (EST) Gal Hammer <gham...@redhat.com> wrote:
> Hi Igor, > > ----- Original Message ----- > > From: "Igor Mammedov" <imamm...@redhat.com> > > To: "Gal Hammer" <gham...@redhat.com> > > Cc: qemu-devel@nongnu.org, m...@redhat.com > > Sent: Monday, February 2, 2015 3:55:02 PM > > Subject: Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine > > Generation ID device > > > > On Mon, 02 Feb 2015 15:13:39 +0200 > > Gal Hammer <gham...@redhat.com> wrote: > > > > > On 02/02/2015 14:46, Igor Mammedov wrote: > > > > On Sun, 01 Feb 2015 14:56:26 +0200 > > > > Gal Hammer <gham...@redhat.com> wrote: > > > > > > > >> On 22/01/2015 15:52, Igor Mammedov wrote: > > > >>> On Tue, 16 Dec 2014 17:50:43 +0200 > > > >>> Gal Hammer <gham...@redhat.com> 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 <gham...@redhat.com> > > > >>>> > > > >>> > > > >>>> --- 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. > > It will, but is it performance critical? VM supposed to read it > > at start-up and on getting notification. So I think VMEXIT in this case > > is not sufficient to drop simple and strait-forward design. > > I agree with you on that and one of the previous patches did used a > fixed-address to store the GUID while read/write access were handled by qemu > driver code. But as I wrote before, it was Michael who didn't approved it so > I proposed this method although it is a bit more complicated. > > I don't know how to break out of this dead-lock... :( Could you post a link to driver based version of series. Perhaps we could address Michael's comments and still stay with a simple implementation. > > > > > BTW: > > For start-up fw_cfg file is not any way better, it's also causes VMEXIT > > for every byte it reads from it. > > I don't understand your claim. Accessing the fw_cfg "file" doesn't cause > VMEXIT as it located somewhere in the guest's memory range. As far as I'm aware MMIO or ioport is used for reading fw_cfg contents on guest side, one byte at a time and every such access causes VMEXIT into QEMU callback. > > > > > > > > > > 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. > > > >> > > > > > > > > > > > > >