qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [RFC PATCH 02/11] hw/arm/virt-acpi-build: Basic framewo


From: Igor Mammedov
Subject: Re: [Qemu-devel] [RFC PATCH 02/11] hw/arm/virt-acpi-build: Basic framework for building ACPI tables
Date: Tue, 27 Jan 2015 11:30:58 +0100

On Tue, 27 Jan 2015 14:47:44 +0800
Shannon Zhao <address@hidden> wrote:

> On 2015/1/26 18:19, Igor Mammedov wrote:
> > On Sat, 24 Jan 2015 17:21:11 +0800
> > Shannon Zhao <address@hidden> wrote:
> > 
> >> Introduce a preliminary framework in virt-acpi-build.c with the main
> >> ACPI build functions. It exposes the generated ACPI contents to
> >> guest over fw_cfg. Some codes borrowed from hw/i386/acpi-build.c.
> >>
> >> The minimum required ACPI v5.1 tables for ARM are:
> >> - RSDP: Initial table that points to XSDT
> >> - XSDT: Points to all other tables (except FACS & DSDT)
> >> - FADT: Generic information about the machine
> >> - DSDT: Holds all information about system devices/peripherals
> >> - FACS: Needs to be pointed from FADT
> > 
> > most of table manipulation code (tables adding to blob/linking)
> > could/should be shared with x86,
> > could you generalize it instead of just copying, please.
> > 
> 
> Thanks for your suggestion and your great work to make ACPI table generation 
> simpler :-)
> 
> Apparently this code is similar with the x86 as their approach are same.
> But the detail of implementation is different at some degree.
> E.g, X86 use some static DSL files while this dynamically generate all thing.
I'm talking not about build_fooTable() functions but rather helpers, like:
acpi_data_push, acpi_add_table, acpi_build_*, huge chunk of acpi_build()
you also probably don't need ACPI_BUILD_TABLE_SIZE and other related macroes.

> 
> >>
> >> Signed-off-by: Shannon Zhao <address@hidden>
> >> ---
> >>  hw/arm/Makefile.objs             |    1 +
> >>  hw/arm/virt-acpi-build.c         |  263 
> >> ++++++++++++++++++++++++++++++++++++++
> >>  include/hw/arm/virt-acpi-build.h |   71 ++++++++++
> >>  3 files changed, 335 insertions(+), 0 deletions(-)
> >>  create mode 100644 hw/arm/virt-acpi-build.c
> >>  create mode 100644 include/hw/arm/virt-acpi-build.h
> >>
> >> diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
> >> index 6088e53..8daf825 100644
> >> --- a/hw/arm/Makefile.objs
> >> +++ b/hw/arm/Makefile.objs
> >> @@ -3,6 +3,7 @@ obj-$(CONFIG_DIGIC) += digic_boards.o
> >>  obj-y += integratorcp.o kzm.o mainstone.o musicpal.o nseries.o
> >>  obj-y += omap_sx1.o palm.o realview.o spitz.o stellaris.o
> >>  obj-y += tosa.o versatilepb.o vexpress.o virt.o xilinx_zynq.o z2.o
> >> +obj-$(CONFIG_ACPI) += virt-acpi-build.o
> >>  
> >>  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
> >>  obj-$(CONFIG_DIGIC) += digic.o
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> new file mode 100644
> >> index 0000000..4eed0a3
> >> --- /dev/null
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -0,0 +1,263 @@
> >> +/* Support for generating ACPI tables and passing them to Guests
> >> + *
> >> + * ARM virt ACPI generation
> >> + *
> >> + * Copyright (c) 2014 HUAWEI TECHNOLOGIES CO.,LTD.
> >> + *
> >> + * Author: Shannon Zhao <address@hidden>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2 or later, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> >> for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License 
> >> along with
> >> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#include "hw/arm/virt-acpi-build.h"
> >> +#include <stddef.h>
> >> +#include <glib.h>
> >> +#include "qemu-common.h"
> >> +#include "qemu/bitmap.h"
> >> +#include "qemu/osdep.h"
> >> +#include "qemu/range.h"
> >> +#include "qemu/error-report.h"
> >> +#include "qom/cpu.h"
> >> +#include "target-arm/cpu.h"
> >> +#include "hw/acpi/acpi-defs.h"
> >> +#include "hw/acpi/acpi.h"
> >> +#include "hw/nvram/fw_cfg.h"
> >> +#include "hw/acpi/bios-linker-loader.h"
> >> +#include "hw/loader.h"
> >> +
> >> +#include "hw/acpi/acpi-build-utils.h"
> >> +
> >> +#include "qapi/qmp/qint.h"
> >> +#include "qom/qom-qobject.h"
> >> +#include "exec/ram_addr.h"
> >> +
> >> +
> >> +#define ACPI_BUILD_TABLE_SIZE             0x20000
> >> +
> >> +/* Reserve RAM space for tables: add another order of magnitude. */
> >> +#define ACPI_BUILD_TABLE_MAX_SIZE         0x200000
> >> +
> >> +/* #define DEBUG_ACPI_BUILD */
> >> +#ifdef DEBUG_ACPI_BUILD
> >> +#define ACPI_BUILD_DPRINTF(fmt, ...)        \
> >> +    do {printf("ACPI_BUILD: " fmt, ## __VA_ARGS__); } while (0)
> >> +#else
> >> +#define ACPI_BUILD_DPRINTF(fmt, ...)
> >> +#endif
> >> +
> >> +#define ACPI_BUILD_APPNAME  "Bochs"
> >> +#define ACPI_BUILD_APPNAME6 "BOCHS "
> >> +
> >> +#define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp"
> >> +#define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log"
> >> +
> >> +static inline void *acpi_data_push(GArray *table_data, uint64_t size)
> >> +{
> >> +    unsigned off = table_data->len;
> >> +    g_array_set_size(table_data, off + size);
> >> +    return table_data->data + off;
> >> +}
> >> +
> >> +static unsigned acpi_data_len(GArray *table)
> >> +{
> >> +#if GLIB_CHECK_VERSION(2, 22, 0)
> >> +    assert(g_array_get_element_size(table) == 1);
> >> +#endif
> >> +    return table->len;
> >> +}
> >> +
> >> +static inline void acpi_add_table(GArray *table_offsets, GArray 
> >> *table_data)
> >> +{
> >> +    uint32_t offset = cpu_to_le32(table_data->len);
> >> +    g_array_append_val(table_offsets, offset);
> >> +}
> >> +
> >> +/* RSDP */
> >> +static GArray *
> >> +build_rsdp(GArray *rsdp_table, GArray *linker, uint64_t xsdt)
> >> +{
> >> +    return rsdp_table;
> >> +}
> >> +
> >> +/* XSDT */
> >> +static void
> >> +build_xsdt(GArray *table_data, GArray *linker, GArray *table_offsets)
> >> +{
> >> +}
> >> +
> >> +/* MADT */
> >> +static void
> >> +build_madt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >> +{
> >> +}
> >> +
> >> +/* GTDT */
> >> +static void
> >> +build_gtdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >> +{
> >> +}
> >> +
> >> +/* FADT */
> >> +static void
> >> +build_fadt(GArray *table_data, GArray *linker, uint64_t facs, uint64_t 
> >> dsdt)
> >> +{
> >> +}
> >> +
> >> +/* FACS */
> >> +static void
> >> +build_facs(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
> >> +{
> >> +}
> >> +
> >> +/* DSDT */
> >> +static void
> >> +build_dsdt(AcpiAml *table_aml, GArray *linker, VirtGuestInfo *guest_info)
> >> +{
> >> +}
Instead of adding empty place holders please add complete functions
in respective patches.

> >> +
> >> +typedef
> >> +struct AcpiBuildTables {
> >> +    AcpiAml table_data;
> >> +    GArray *rsdp;
> >> +    GArray *tcpalog;
> >> +    GArray *linker;
> >> +} AcpiBuildTables;
> >> +
> >> +static inline void acpi_build_tables_init(AcpiBuildTables *tables)
> >> +{
> >> +    tables->rsdp = g_array_new(false, true /* clear */, 1);
> >> +    tables->table_data.buf = g_array_new(false, true /* clear */, 1);
> >> +    tables->tcpalog = g_array_new(false, true /* clear */, 1);
> >> +    tables->linker = bios_linker_loader_init();
> >> +    tables->table_data.linker = tables->linker;
> >> +}
> >> +
> >> +static inline void acpi_build_tables_cleanup(AcpiBuildTables *tables, 
> >> bool mfre)
> >> +{
> >> +    void *linker_data = bios_linker_loader_cleanup(tables->linker);
> >> +    g_free(linker_data);
> >> +    g_array_free(tables->rsdp, mfre);
> >> +    g_array_free(tables->table_data.buf, true);
> >> +    g_array_free(tables->tcpalog, mfre);
> >> +}
> >> +
> >> +typedef
> >> +struct AcpiBuildState {
> >> +    /* Copy of table in RAM (for patching). */
> >> +    ram_addr_t table_ram;
> >> +    uint32_t table_size;
> >> +    /* Is table patched? */
> >> +    uint8_t patched;
> >> +    VirtGuestInfo *guest_info;
> >> +} AcpiBuildState;
> >> +
> >> +static
> >> +void acpi_build(VirtGuestInfo *guest_info, AcpiBuildTables *tables)
> >> +{
> >> +    GArray *table_offsets;
> >> +    unsigned facs, dsdt, xsdt;
> >> +
> >> +    table_offsets = g_array_new(false, true /* clear */,
> >> +                                        sizeof(uint32_t));
> >> +    ACPI_BUILD_DPRINTF("init ACPI tables\n");
> > use tracing infrastructure instead of DPRINTFs
> > 
> >> +
> >> +    bios_linker_loader_alloc(tables->linker, ACPI_BUILD_TABLE_FILE,
> >> +                             64 /* Ensure FACS is aligned */,
> >> +                             false /* high memory */);
> >> +
> >> +    /*
> >> +     * FACS is pointed to by FADT.
> >> +     * We place it first since it's the only table that has alignment
> >> +     * requirements.
> >> +     */
> >> +    facs = tables->table_data.buf->len;
> >> +    build_facs(tables->table_data.buf, tables->linker, guest_info);
> >> +
> >> +    /* DSDT is pointed to by FADT */
> >> +    dsdt = tables->table_data.buf->len;
> >> +    build_dsdt(&tables->table_data, tables->linker, guest_info);
> >> +
> >> +    /* ACPI tables pointed to by XSDT */
> >> +    acpi_add_table(table_offsets, tables->table_data.buf);
> >> +    build_fadt(tables->table_data.buf, tables->linker, facs, dsdt);
> >> +
> >> +    acpi_add_table(table_offsets, tables->table_data.buf);
> >> +    build_madt(tables->table_data.buf, tables->linker, guest_info);
> >> +
> >> +    acpi_add_table(table_offsets, tables->table_data.buf);
> >> +    build_gtdt(tables->table_data.buf, tables->linker, guest_info);
> >> +
> >> +    /* XSDT is pointed to by RSDP */
> >> +    xsdt = tables->table_data.buf->len;
> >> +    build_xsdt(tables->table_data.buf, tables->linker, table_offsets);
> >> +
> >> +    /* RSDP is in FSEG memory, so allocate it separately */
> >> +    build_rsdp(tables->rsdp, tables->linker, xsdt);
> >> +
> >> +    /* Cleanup memory that's no longer used. */
> >> +    g_array_free(table_offsets, true);
> >> +}
> >> +
> >> +static ram_addr_t acpi_add_rom_blob(AcpiBuildState *build_state, GArray 
> >> *blob,
> >> +                               const char *name, uint64_t max_size)
> >> +{
> >> +    return rom_add_blob(name, blob->data, acpi_data_len(blob), max_size, 
> >> -1,
> >> +                        name, NULL, build_state);
> >> +}
> >> +
> >> +void virt_acpi_setup(VirtGuestInfo *guest_info)
> >> +{
> >> +    AcpiBuildTables tables;
> >> +    AcpiBuildState *build_state;
> >> +
> >> +    if (!guest_info->fw_cfg) {
> >> +        ACPI_BUILD_DPRINTF("No fw cfg. Bailing out.\n");
> >> +        return;
> >> +    }
> >> +
> >> +    if (!acpi_enabled) {
> >> +        ACPI_BUILD_DPRINTF("ACPI disabled. Bailing out.\n");
> >> +        return;
> >> +    }
> >> +
> >> +    build_state = g_malloc0(sizeof *build_state);
> >> +    build_state->guest_info = guest_info;
> >> +
> >> +    acpi_build_tables_init(&tables);
> >> +    acpi_build(build_state->guest_info, &tables);
> >> +
> >> +    /* Now expose it all to Guest */
> >> +    build_state->table_ram = acpi_add_rom_blob(build_state,
> >> +                                               tables.table_data.buf,
> >> +                                               ACPI_BUILD_TABLE_FILE,
> >> +                                               ACPI_BUILD_TABLE_MAX_SIZE);
> >> +    assert(build_state->table_ram != RAM_ADDR_MAX);
> >> +    build_state->table_size = acpi_data_len(tables.table_data.buf);
> >> +
> >> +    acpi_add_rom_blob(NULL, tables.linker, "etc/table-loader", 0);
> >> +
> >> +    fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_TPMLOG_FILE,
> >> +                    tables.tcpalog->data, acpi_data_len(tables.tcpalog));
> >> +
> >> +    /*
> >> +     * RSDP is small so it's easy to keep it immutable, no need to
> >> +     * bother with ROM blobs.
> >> +     */
> >> +    fw_cfg_add_file(guest_info->fw_cfg, ACPI_BUILD_RSDP_FILE,
> >> +                    tables.rsdp->data, acpi_data_len(tables.rsdp));
> > that's broken since position of RSDT could change on reboot and initial RSDP
> > would point to a wrong offset afterwards. Ith should be rom_blob as well
> > which is re-creatable on reboot as ACPI_BUILD_TABLE_FILE.
> > 
> > 
> 
> Sorry, I'm not clear about your comment. I did inside reboot and outside 
> reset, it looks well.
In gist the issue is if any table between xsdt changes its size across
reboots (i.e. XSDT moves) static RSDP will point to wrong offset and
firmware will corrupt tables by patching blob at wrong offset.

For x86 fix we agreed that RSDP will be made dynamic
(which is sufficient to fix issue).

see for example 
http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg01699.html

> 
> >> +
> >> +    /* Cleanup tables but don't free the memory: we track it
> >> +     * in build_state.
> >> +     */
> >> +    acpi_build_tables_cleanup(&tables, false);
> >> +}
> >> diff --git a/include/hw/arm/virt-acpi-build.h 
> >> b/include/hw/arm/virt-acpi-build.h
> >> new file mode 100644
> >> index 0000000..7a09c34
> >> --- /dev/null
> >> +++ b/include/hw/arm/virt-acpi-build.h
> >> @@ -0,0 +1,71 @@
> >> +/*
> >> + *
> >> + * Copyright (c) 2014 HUAWEI TECHNOLOGIES CO.,LTD.
> >> + *
> >> + * Author: Shannon Zhao <address@hidden>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify it
> >> + * under the terms and conditions of the GNU General Public License,
> >> + * version 2 or later, as published by the Free Software Foundation.
> >> + *
> >> + * This program is distributed in the hope it will be useful, but WITHOUT
> >> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> >> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
> >> for
> >> + * more details.
> >> + *
> >> + * You should have received a copy of the GNU General Public License 
> >> along with
> >> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> >> + */
> >> +
> >> +#ifndef QEMU_VIRT_ACPI_BUILD_H
> >> +#define QEMU_VIRT_ACPI_BUILD_H
> >> +
> >> +#include "qemu-common.h"
> >> +#include "hw/acpi/acpi-defs.h"
> >> +
> >> +
> >> +#define ACPI_VIRT_QEMU_STR_4 "QEMU"
> >> +#define ACPI_VIRT_QEMU_STR_6 "QEMU  "
> >> +#define ACPI_VIRT_MACH_STR_8 "MACHVIRT"
> >> +
> >> +struct acpi_gtdt_info {
> >> +    uint32_t timer_virt;
> >> +    uint32_t timer_s_el1;
> >> +    uint32_t timer_ns_el1;
> >> +    uint32_t timer_ns_el2;
> >> +};
> >> +
> >> +struct acpi_madt_info {
> >> +    const hwaddr *gic_cpu_base_addr;
> >> +    const hwaddr *gic_dist_base_addr;
> >> +};
> >> +
> >> +struct acpi_dsdt_info {
> >> +    const hwaddr *uart_addr;
> >> +    const int *uart_irq;
> >> +    const hwaddr *virtio_mmio_addr;
> >> +    const int *virtio_mmio_irq;
> >> +    int virtio_mmio_num;
> >> +    const hwaddr *rtc_addr;
> >> +    const int *rtc_irq;
> >> +    const hwaddr *flash_addr;
> >> +};
> >> +
> >> +typedef struct VirtGuestInfo {
> >> +    int nb_cpus;
> >> +    int max_cpus;
> >> +    FWCfgState *fw_cfg;
> >> +    const struct acpi_madt_info *madt_info;
> >> +    const struct acpi_dsdt_info *dsdt_info;
> >> +    const struct acpi_gtdt_info *gtdt_info;
> >> +} VirtGuestInfo;
> >> +
> >> +
> >> +typedef struct VirtGuestInfoState {
> >> +    VirtGuestInfo info;
> >> +    Notifier machine_done;
> >> +} VirtGuestInfoState;
> >> +
> >> +void virt_acpi_setup(VirtGuestInfo *guest_info);
> >> +
> >> +#endif
> > 
> > 
> > .
> > 
> 
> 
> 




reply via email to

[Prev in Thread] Current Thread [Next in Thread]