[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation
From: |
Michael S. Tsirkin |
Subject: |
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device |
Date: |
Thu, 22 Jan 2015 23:21:51 +0200 |
On Thu, Jan 22, 2015 at 02:52:46PM +0100, Igor Mammedov wrote:
> On Tue, 16 Dec 2014 17:50:43 +0200
> Gal Hammer <address@hidden> 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 <address@hidden>
> >
>
> > --- 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