| Message ID | 1428055432-12120-11-git-send-email-zhaoshenglong@huawei.com |
|---|---|
| State | New |
| Headers | show |
Shannon Zhao <zhaoshenglong@huawei.com> writes: > From: Shannon Zhao <shannon.zhao@linaro.org> > > RSDT points to other tables FADT, MADT, GTDT. > > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com> > Signed-off-by: Shannon Zhao <shannon.zhao@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); > }
On 9 April 2015 at 14:17, Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 09 Apr 2015 13:50:52 +0100 > Alex Bennée <alex.bennee@linaro.org> wrote: > >> >> Shannon Zhao <zhaoshenglong@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
On 9 April 2015 at 14:51, Igor Mammedov <imammedo@redhat.com> wrote: > On Thu, 9 Apr 2015 14:27:58 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: > >> On 9 April 2015 at 14:17, Igor Mammedov <imammedo@redhat.com> wrote: >> > On Thu, 09 Apr 2015 13:50:52 +0100 >> > Alex Bennée <alex.bennee@linaro.org> wrote: >> > >> >> >> >> Shannon Zhao <zhaoshenglong@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
On 9 April 2015 at 17:00, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/09/15 15:59, Peter Maydell wrote: >> On 9 April 2015 at 14:51, Igor Mammedov <imammedo@redhat.com> wrote: >>> On Thu, 9 Apr 2015 14:27:58 +0100 >>> Peter Maydell <peter.maydell@linaro.org> wrote: >>> >>>> On 9 April 2015 at 14:17, Igor Mammedov <imammedo@redhat.com> wrote: >>>>> On Thu, 09 Apr 2015 13:50:52 +0100 >>>>> Alex Bennée <alex.bennee@linaro.org> wrote: >>>>> >>>>>> >>>>>> Shannon Zhao <zhaoshenglong@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
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); }