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

[Gal Hammer]

Hi Igor,

----- Original Message -----
> From: "Igor Mammedov" <address@hidden>
> To: "Gal Hammer" <address@hidden>
> Cc: address@hidden, address@hidden
> 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 <address@hidden> wrote:
> 
> > On 02/02/2015 14:46, Igor Mammedov wrote:
> > > 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.
> > 
> > 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... :(

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

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