Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device

[Igor Mammedov <imammedo@redhat.com>]

On Tue, 16 Dec 2014 17:50:43 +0200
Gal Hammer <ghammer@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 <ghammer@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,
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])

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.

>
[...]
>  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(...)/