Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
On Wed, Feb 04, 2015 at 04:53:54PM +0100, Igor Mammedov wrote: 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. The point is to keep all allocations in guest. I don't want to steal a page from guest. 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. It's highly unlikely to be measureable. Prove me wrong if you like. -- MST
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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.
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.
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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.
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
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
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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.
[...]
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.
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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,
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(...)/
Re: [Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
On Thu, Jan 22, 2015 at 02:52:46PM +0100, 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,
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
[Qemu-devel] [PATCH V11 2/3] i386: Add a Virtual Machine Generation ID device
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
---
default-configs/i386-softmmu.mak | 1 +
default-configs/x86_64-softmmu.mak | 1 +
hw/acpi/core.c | 8 +++
hw/acpi/ich9.c | 8 +++
hw/acpi/piix4.c | 8 +++
hw/i386/acpi-build.c | 26 +++
hw/i386/acpi-dsdt.dsl| 4 +-
hw/i386/pc.c | 8 +++
hw/i386/q35-acpi-dsdt.dsl| 5 +-
hw/i386/ssdt-misc.dsl| 43
hw/isa/lpc_ich9.c| 1 +
hw/misc/Makefile.objs| 1 +
hw/misc/vmgenid.c| 131 +++
include/hw/acpi/acpi.h | 2 +
include/hw/acpi/acpi_dev_interface.h | 4 ++
include/hw/acpi/ich9.h | 2 +
include/hw/i386/pc.h | 3 +
include/hw/misc/vmgenid.h| 21 ++
18 files changed, 275 insertions(+), 2 deletions(-)
create mode 100644 hw/misc/vmgenid.c
create mode 100644 include/hw/misc/vmgenid.h
diff --git a/default-configs/i386-softmmu.mak b/default-configs/i386-softmmu.mak
index 8e08841..bd33c75 100644
--- a/default-configs/i386-softmmu.mak
+++ b/default-configs/i386-softmmu.mak
@@ -45,3 +45,4 @@ CONFIG_IOAPIC=y
CONFIG_ICC_BUS=y
CONFIG_PVPANIC=y
CONFIG_MEM_HOTPLUG=y
+CONFIG_VMGENID=y
diff --git a/default-configs/x86_64-softmmu.mak
b/default-configs/x86_64-softmmu.mak
index 66557ac..006fc7c 100644
--- a/default-configs/x86_64-softmmu.mak
+++ b/default-configs/x86_64-softmmu.mak
@@ -45,3 +45,4 @@ CONFIG_IOAPIC=y
CONFIG_ICC_BUS=y
CONFIG_PVPANIC=y
CONFIG_MEM_HOTPLUG=y
+CONFIG_VMGENID=y
diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 51913d6..d4597c6 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -28,6 +28,8 @@
#include qapi-visit.h
#include qapi-event.h
+#define ACPI_VM_GENERATION_ID_CHANGED_STATUS 1
+
struct acpi_table_header {
uint16_t _length; /* our length, not actual part of the hdr */
/* allows easier parsing for fw_cfg clients */
@@ -683,3 +685,9 @@ void acpi_update_sci(ACPIREGS *regs, qemu_irq irq)
(regs-pm1.evt.en ACPI_BITMASK_TIMER_ENABLE)
!(pm1a_sts ACPI_BITMASK_TIMER_STATUS));
}
+
+void acpi_vm_generation_id_changed(ACPIREGS *acpi_regs, qemu_irq irq)
+{
+acpi_regs-gpe.sts[0] |= ACPI_VM_GENERATION_ID_CHANGED_STATUS;
+acpi_update_sci(acpi_regs, irq);
+}
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index ea991a3..12a9387 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -307,3 +307,11 @@ void ich9_pm_ospm_status(AcpiDeviceIf *adev,
ACPIOSTInfoList ***list)
acpi_memory_ospm_status(s-pm.acpi_memory_hotplug, list);
}
+
+void ich9_vm_generation_id_changed(AcpiDeviceIf *adev)
+{
+ICH9LPCState *s = ICH9_LPC_DEVICE(adev);
+ICH9LPCPMRegs *pm = s-pm;
+
+acpi_vm_generation_id_changed(pm-acpi_regs, pm-irq);
+}
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 481a16c..41b6eb6 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -574,6 +574,13 @@ static void piix4_ospm_status(AcpiDeviceIf *adev,
ACPIOSTInfoList ***list)
acpi_memory_ospm_status(s-acpi_memory_hotplug, list);
}
+static void piix4_vm_generation_id_changed(AcpiDeviceIf *adev)
+{
+PIIX4PMState *s = PIIX4_PM(adev);
+
+acpi_vm_generation_id_changed(s-ar, s-irq);
+}
+
static Property piix4_pm_properties[] = {
DEFINE_PROP_UINT32(smb_io_base, PIIX4PMState, smb_io_base, 0),
DEFINE_PROP_UINT8(ACPI_PM_PROP_S3_DISABLED, PIIX4PMState, disable_s3, 0),
@@ -611,6 +618,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void
*data)
hc-plug = piix4_device_plug_cb;
hc-unplug_request = piix4_device_unplug_request_cb;
adevc-ospm_status = piix4_ospm_status;
+adevc-vm_generation_id_changed = piix4_vm_generation_id_changed;
}
static const TypeInfo piix4_pm_info = {
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a4d0c0c..f20a6a5 100644
--- 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;
