Re: [Qemu-devel] [PATCH v6 03/22] hw/arm/virt-acpi-build: Basic framework for building ACPI tables on ARM

[Shannon Zhao]

On 2015/5/7 18:50, Alex Bennée wrote:
> Shannon Zhao <address@hidden> writes:
> 
>> > From: Shannon Zhao <address@hidden>
>> >
>> > 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.
>> >
>> > The required ACPI v5.1 tables for ARM are:
>> > - RSDP: Initial table that points to XSDT
>> > - RSDT: Points to FADT GTDT MADT tables
>> > - FADT: Generic information about the machine
>> > - GTDT: Generic timer description table
>> > - MADT: Multiple APIC description table
>> > - DSDT: Holds all information about system devices/peripherals, pointed by 
>> > FADT
>> >
>> > Signed-off-by: Shannon Zhao <address@hidden>
>> > Signed-off-by: Shannon Zhao <address@hidden>
>> > ---
>> >  hw/arm/Makefile.objs             |   1 +
>> >  hw/arm/virt-acpi-build.c         | 184 
>> > +++++++++++++++++++++++++++++++++++++++
>> >  include/hw/arm/virt-acpi-build.h |  70 +++++++++++++++
>> >  qemu-options.hx                  |   2 +-
>> >  trace-events                     |   3 +
>> >  5 files changed, 259 insertions(+), 1 deletion(-)
>> >  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 2577f68..a1bfb19 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 += netduino2.o
>> >  
>> >  obj-y += armv7m.o exynos4210.o pxa2xx.o pxa2xx_gpio.o pxa2xx_pic.o
>> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> > new file mode 100644
>> > index 0000000..960206d
>> > --- /dev/null
>> > +++ b/hw/arm/virt-acpi-build.c
>> > @@ -0,0 +1,184 @@
>> > +/* Support for generating ACPI tables and passing them to Guests
>> > + *
>> > + * ARM virt ACPI generation
>> > + *
>> > + * Copyright (C) 2008-2010  Kevin O'Connor <address@hidden>
>> > + * Copyright (C) 2006 Fabrice Bellard
>> > + * Copyright (C) 2013 Red Hat Inc
>> > + *
>> > + * Author: Michael S. Tsirkin <address@hidden>
>> > + *
>> > + * Copyright (c) 2015 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 of the GNU General Public License as published by
>> > + * the Free Software Foundation; either version 2 of the License, or
>> > + * (at your option) any later version.
>> > +
>> > + * This program is distributed in the hope that 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 "trace.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/hw.h"
>> > +#include "hw/acpi/aml-build.h"
>> > +
>> > +typedef
>> > +struct AcpiBuildState {
>> > +    /* Copy of table in RAM (for patching). */
>> > +    MemoryRegion *table_mr;
>> > +    MemoryRegion *rsdp_mr;
>> > +    MemoryRegion *linker_mr;
>> > +    /* Is table patched? */
>> > +    uint8_t patched;
> I missed this last time but a uint8_t seems excessive if a bool will do
> (using true/false and VMSTATE_BOOL later on).
> 
> Otherwise:
> 
> Reviewed-by: Alex Bennée <address@hidden>
> 

Ok, will replace this. Thanks.

-- 
Shannon