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

[Igor Mammedov]

On Sun, 01 Feb 2015 14:56:26 +0200
Gal Hammer <address@hidden> wrote:

> On 22/01/2015 15:52, 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,
> 
> 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.

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.
>