Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
Shannon Zhao zhaoshengl...@huawei.com writes:
From: Shannon Zhao shannon.z...@linaro.org
RSDT points to other tables FADT, MADT, GTDT.
Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a7aba75..85e84b1 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -176,6 +176,30 @@ static void acpi_dsdt_add_virtio(Aml *scope, const
hwaddr *mmio_addrs,
}
}
+/* RSDT */
+static void
+build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
+{
+AcpiRsdtDescriptorRev1 *rsdt;
+size_t rsdt_len;
+int i;
+
+rsdt_len = sizeof(*rsdt) + sizeof(uint32_t) * table_offsets-len;
You should use explicit brackets to be unambiguous:
rsdt_len = sizeof(*rsdt) + (sizeof(uint32_t) * table_offsets-len);
+rsdt = acpi_data_push(table_data, rsdt_len);
+memcpy(rsdt-table_offset_entry, table_offsets-data,
+ sizeof(uint32_t) * table_offsets-len);
Or perhaps split the sizes:
const int table_data_len = (sizeof(uint32_t) * table_offsets-len);
rsdt_len = sizeof(*rsdt) + table_data_len;
rsdt = acpi_data_push(table_data, rsdt_len);
memcpy(rsdt-table_offset_entry, table_offsets-data, table_data_len)
Maybe?
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
+}
+build_header(linker, table_data,
+ (void *)rsdt, RSDT, rsdt_len, 1);
+}
+
/* GTDT */
static void
build_gtdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
@@ -348,6 +372,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_gtdt(tables_blob, tables-linker, guest_info);
+/* RSDT is pointed to by RSDP */
+build_rsdt(tables_blob, tables-linker, table_offsets);
+
/* Cleanup memory that's no longer used. */
g_array_free(table_offsets, true);
}
--
Alex Bennée
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
From: Shannon Zhao shannon.z...@linaro.org
RSDT points to other tables FADT, MADT, GTDT.
Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a7aba75..85e84b1 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -176,6 +176,30 @@ static void acpi_dsdt_add_virtio(Aml *scope, const
hwaddr *mmio_addrs,
}
}
+/* RSDT */
+static void
+build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
this function looks like exact copy of x86 impl.,
could you reuse that?
+{
+AcpiRsdtDescriptorRev1 *rsdt;
+size_t rsdt_len;
+int i;
+
+rsdt_len = sizeof(*rsdt) + sizeof(uint32_t) * table_offsets-len;
You should use explicit brackets to be unambiguous:
rsdt_len = sizeof(*rsdt) + (sizeof(uint32_t) * table_offsets-len);
+rsdt = acpi_data_push(table_data, rsdt_len);
+memcpy(rsdt-table_offset_entry, table_offsets-data,
+ sizeof(uint32_t) * table_offsets-len);
Or perhaps split the sizes:
const int table_data_len = (sizeof(uint32_t) * table_offsets-len);
rsdt_len = sizeof(*rsdt) + table_data_len;
rsdt = acpi_data_push(table_data, rsdt_len);
memcpy(rsdt-table_offset_entry, table_offsets-data, table_data_len)
Maybe?
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address space?
+}
+build_header(linker, table_data,
+ (void *)rsdt, RSDT, rsdt_len, 1);
+}
+
/* GTDT */
static void
build_gtdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
@@ -348,6 +372,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_gtdt(tables_blob, tables-linker, guest_info);
+/* RSDT is pointed to by RSDP */
+build_rsdt(tables_blob, tables-linker, table_offsets);
+
/* Cleanup memory that's no longer used. */
g_array_free(table_offsets, true);
}
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On 04/09/15 18:03, Peter Maydell wrote:
On 9 April 2015 at 17:00, Laszlo Ersek ler...@redhat.com wrote:
On 04/09/15 15:59, Peter Maydell wrote:
On 9 April 2015 at 14:51, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 9 Apr 2015 14:27:58 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address
space?
I confirmed that before, in the v2 discussion:
http://thread.gmane.org/gmane.comp.emulators.qemu/316670/focus=317560
But in fact the RSDT / XSDT that QEMU exports for UEFI doesn't matter.
If this table is never used, presumably we should just
not generate it at all, then?
Unfortunately, this is not the case. In order to identify ACPI tables *at all*
in UEFI, I need relocate pointer commands for pointers that point to those
tables. And those pointers must *reside* somewhere, in some blob.
Here's how the relocate pointer command is defined in edk2
(OvmfPkg/AcpiPlatformDxe/QemuLoader.h):
//
// QemuLoaderCmdAddPointer: the bytes at
// [PointerOffset..PointerOffset+PointerSize) in the file PointerFile contain a
// relative pointer (an offset) into PointeeFile. Increment the relative
// pointer's value by the base address of where PointeeFile's contents have
// been placed (when QemuLoaderCmdAllocate has been executed for PointeeFile).
//
typedef struct {
UINT8 PointerFile[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
UINT8 PointeeFile[QEMU_LOADER_FNAME_SIZE]; // NUL-terminated
UINT32 PointerOffset;
UINT8 PointerSize; // one of 1, 2, 4, 8
} QEMU_LOADER_ADD_POINTER;
In the qemu tree, see COMMAND_ADD_POINTER in hw/acpi/bios-linker-loader.c,
for the same. (I rewrote the types and the comments in edk2 from scratch, both
for coding style reasons and for clearer documentation.)
... To be clear: the top-level pointers must exist somewhere (in some blob),
because that helps edk2 find the tables (in some other blobs). However, the
top-level pointers themselves don't need to reside in any ACPI table (RSDT,
XSDT); they can just live in an otherwise unreferenced portion of one of the
blobs.
But, IMO, implementing that wouldn't be much easier (and it would certainly be
uglier) than composing a correct RSDT or XSDT. The latter would also keep the
similarity with the x86 SeaBIOS case (where the RSDT is a hard requirement).
Thanks
Laszlo
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On 04/09/15 15:59, Peter Maydell wrote:
On 9 April 2015 at 14:51, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 9 Apr 2015 14:27:58 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address
space?
I confirmed that before, in the v2 discussion:
http://thread.gmane.org/gmane.comp.emulators.qemu/316670/focus=317560
But in fact the RSDT / XSDT that QEMU exports for UEFI doesn't matter.
See below.
In the general case you can't guarantee that there will
be any RAM at all below the 4G point. (The virt board
isn't like that, obviously, but I believe there's real
hardware out there that's designed that way.) I don't
think we should have any 32 bit assumptions in the
code at all -- pointer values should always be 64 bits
everywhere.
then that forces us to use xsdt instead of 32-bit rsdt
Does that matter much?
I can mention two points here.
(1) See this kernel patch:
http://thread.gmane.org/gmane.linux.acpi.devel/74369/focus=1915858
+The ACPI core will ignore any provided RSDT (Root System Description Table).
+RSDTs have been deprecated and are ignored on arm64 since they only allow
+for 32-bit addresses.
So you could argue that providing an XSDT instead of an RSDT is
justified by this alone.
(2) the ACPI linker/loader client in edk2 (used for both OVMF and AAVMF)
*does* restrict initial allocations to under 4GB. This is a super hairy
subject, but I'll try to summarize it quickly.
The ACPI linker/loader interface was originally designed for SeaBIOS.
The central structure of the interface is a command table, which
contains three kinds of commands:
(a) allocate blob (which includes download blob as well),
(b) relocate pointer (also known as update pointer), and
(c) checksum blob segment.
Importantly, the allocate command comes with allocation hints; it can
instruct the firmware to allocate the blob in some specific zone.
So, the general process (as designed) is something like this:
- The firmware allocates and downloads the blobs -- the allocate
commands always come first in the table. Each blob can contain several
ACPI tables, in any kind of arrangement.
- Then the relocate pointer commands are processed (simply because
they come later in the command table, that's guaranteed), which update
pointers in some tables (residing in some blobs) to some other tables
(residing in some other, or the same, blob(s)). In more detail, the
pointers in the tables in the blobs are pre-initialized with relative
offsets (into other blobs), and the relocation means that these
relative offsets are made absolute -- they are incremented with the
actual allocation base addresses that are the results of the
allocation command processing (see previous step).
- Finally (in fact, intermixed with relocate pointer commands),
checksums are updated.
The idea is that after the initial allocations, everything is processed
*in place*. (This is what SeaBIOS does.) Because pointer fields, updated
by the relocate pointer commands (which basically mean increments by
actual blob base addresses) can come in various sizes (1, 2, 4, 8
bytes), the allocation commands must take care to instruct the firmware
to allocate the target blobs low enough so that the referring
pointers can accommodate these actual base addresses.
All fine; again, SeaBIOS does exactly this; the important thing to note
is that everything is processed, and then left for the runtime OS, *in
place*.
And then UEFI / edk2 came along. :)
The problem with UEFI is that you are not supposed to just throw a bunch
of binary stuff into RAM. Instead, the RSD PTR table needs to be linked
into the UEFI system config table, plus each table needs to be installed
*individually*, by passing it to EFI_ACPI_TABLE_PROTOCOL.InstallTable().
The first requirement is actually a relaxation -- the RSD PTR can be
anywhere in memory, it doesn't need to be low. However, the second
requirement is a huge pain, because it doesn't match the design of the
ACPI linker/loader interface. EFI_ACPI_TABLE_PROTOCOL is smart about
the specification, and knows what to allocate where -- it copies tables,
links the copies together, checksums them, handles the RSD PTR
internally, and so on; but it does need to *receive*
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On Thu, 9 Apr 2015 14:59:09 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:51, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 9 Apr 2015 14:27:58 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address
space?
In the general case you can't guarantee that there will
be any RAM at all below the 4G point. (The virt board
isn't like that, obviously, but I believe there's real
hardware out there that's designed that way.) I don't
think we should have any 32 bit assumptions in the
code at all -- pointer values should always be 64 bits
everywhere.
then that forces us to use xsdt instead of 32-bit rsdt
Does that matter much?
not much, using rsdt would allow to share this code with x86.
also having tables below 4Gb in rsdt would make life of
32 bit guests easier, not that there are such guests now and may be
there wouldn't be any.
-- PMM
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On 9 April 2015 at 17:00, Laszlo Ersek ler...@redhat.com wrote:
On 04/09/15 15:59, Peter Maydell wrote:
On 9 April 2015 at 14:51, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 9 Apr 2015 14:27:58 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address
space?
I confirmed that before, in the v2 discussion:
http://thread.gmane.org/gmane.comp.emulators.qemu/316670/focus=317560
But in fact the RSDT / XSDT that QEMU exports for UEFI doesn't matter.
If this table is never used, presumably we should just
not generate it at all, then?
-- PMM
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On 9 April 2015 at 14:51, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 9 Apr 2015 14:27:58 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address space?
In the general case you can't guarantee that there will
be any RAM at all below the 4G point. (The virt board
isn't like that, obviously, but I believe there's real
hardware out there that's designed that way.) I don't
think we should have any 32 bit assumptions in the
code at all -- pointer values should always be 64 bits
everywhere.
then that forces us to use xsdt instead of 32-bit rsdt
Does that matter much?
-- PMM
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address space?
In the general case you can't guarantee that there will
be any RAM at all below the 4G point. (The virt board
isn't like that, obviously, but I believe there's real
hardware out there that's designed that way.) I don't
think we should have any 32 bit assumptions in the
code at all -- pointer values should always be 64 bits
everywhere.
-- PMM
Re: [Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
On Thu, 9 Apr 2015 14:27:58 +0100
Peter Maydell peter.mayd...@linaro.org wrote:
On 9 April 2015 at 14:17, Igor Mammedov imamm...@redhat.com wrote:
On Thu, 09 Apr 2015 13:50:52 +0100
Alex Bennée alex.ben...@linaro.org wrote:
Shannon Zhao zhaoshengl...@huawei.com writes:
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
Why are these pointers always 32 bit? Can they ever be 64 bit?
Laszlo, can you confirm that UEFI puts APCI tables below 4G address space?
In the general case you can't guarantee that there will
be any RAM at all below the 4G point. (The virt board
isn't like that, obviously, but I believe there's real
hardware out there that's designed that way.) I don't
think we should have any 32 bit assumptions in the
code at all -- pointer values should always be 64 bits
everywhere.
then that forces us to use xsdt instead of 32-bit rsdt
-- PMM
[Qemu-devel] [PATCH v4 10/20] hw/arm/virt-acpi-build: Generate RSDT table
From: Shannon Zhao shannon.z...@linaro.org
RSDT points to other tables FADT, MADT, GTDT.
Signed-off-by: Shannon Zhao zhaoshengl...@huawei.com
Signed-off-by: Shannon Zhao shannon.z...@linaro.org
---
hw/arm/virt-acpi-build.c | 27 +++
1 file changed, 27 insertions(+)
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index a7aba75..85e84b1 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -176,6 +176,30 @@ static void acpi_dsdt_add_virtio(Aml *scope, const hwaddr
*mmio_addrs,
}
}
+/* RSDT */
+static void
+build_rsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
+{
+AcpiRsdtDescriptorRev1 *rsdt;
+size_t rsdt_len;
+int i;
+
+rsdt_len = sizeof(*rsdt) + sizeof(uint32_t) * table_offsets-len;
+rsdt = acpi_data_push(table_data, rsdt_len);
+memcpy(rsdt-table_offset_entry, table_offsets-data,
+ sizeof(uint32_t) * table_offsets-len);
+for (i = 0; i table_offsets-len; ++i) {
+/* rsdt-table_offset_entry to be filled by Guest linker */
+bios_linker_loader_add_pointer(linker,
+ ACPI_BUILD_TABLE_FILE,
+ ACPI_BUILD_TABLE_FILE,
+ table_data,
rsdt-table_offset_entry[i],
+ sizeof(uint32_t));
+}
+build_header(linker, table_data,
+ (void *)rsdt, RSDT, rsdt_len, 1);
+}
+
/* GTDT */
static void
build_gtdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
@@ -348,6 +372,9 @@ void virt_acpi_build(VirtGuestInfo *guest_info,
AcpiBuildTables *tables)
acpi_add_table(table_offsets, tables_blob);
build_gtdt(tables_blob, tables-linker, guest_info);
+/* RSDT is pointed to by RSDP */
+build_rsdt(tables_blob, tables-linker, table_offsets);
+
/* Cleanup memory that's no longer used. */
g_array_free(table_offsets, true);
}
--
2.0.4
