Re: [PATCH V3 4/8] hw/riscv/virt: Enable basic ACPI infrastructure

[Sunil V L]

On Thu, Feb 16, 2023 at 06:36:19PM +0100, Andrew Jones wrote:
> On Thu, Feb 16, 2023 at 10:11:21PM +0530, Sunil V L wrote:
> > Add basic ACPI infrastructure for RISC-V with below tables.
> >         1) DSDT with below basic objects
> >                 - CPUs
> >                 - fw_cfg
> >         2) FADT revision 6 with HW_REDUCED flag
> >         3) XSDT
> >         4) RSDP
> > 
> > Add this functionality in a new file virt-acpi-build.c and enable
> > building this infrastructure.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/riscv/Kconfig           |   1 +
> >  hw/riscv/meson.build       |   1 +
> >  hw/riscv/virt-acpi-build.c | 267 +++++++++++++++++++++++++++++++++++++
> >  include/hw/riscv/virt.h    |   1 +
> >  4 files changed, 270 insertions(+)
> >  create mode 100644 hw/riscv/virt-acpi-build.c
> > 
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > index 4550b3b938..6528ebfa3a 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -44,6 +44,7 @@ config RISCV_VIRT
> >      select VIRTIO_MMIO
> >      select FW_CFG_DMA
> >      select PLATFORM_BUS
> > +    select ACPI
> >  
> >  config SHAKTI_C
> >      bool
> > diff --git a/hw/riscv/meson.build b/hw/riscv/meson.build
> > index ab6cae57ea..2f7ee81be3 100644
> > --- a/hw/riscv/meson.build
> > +++ b/hw/riscv/meson.build
> > @@ -9,5 +9,6 @@ riscv_ss.add(when: 'CONFIG_SIFIVE_E', if_true: 
> > files('sifive_e.c'))
> >  riscv_ss.add(when: 'CONFIG_SIFIVE_U', if_true: files('sifive_u.c'))
> >  riscv_ss.add(when: 'CONFIG_SPIKE', if_true: files('spike.c'))
> >  riscv_ss.add(when: 'CONFIG_MICROCHIP_PFSOC', if_true: 
> > files('microchip_pfsoc.c'))
> > +riscv_ss.add(when: 'CONFIG_ACPI', if_true: files('virt-acpi-build.c'))
> >  
> >  hw_arch += {'riscv': riscv_ss}
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> > new file mode 100644
> > index 0000000000..30af974db3
> > --- /dev/null
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -0,0 +1,267 @@
> > +/*
> > + * Support for generating ACPI tables and passing them to Guests
> > + *
> > + * RISC-V virt ACPI generation
> > + *
> > + * Copyright (C) 2008-2010  Kevin O'Connor <kevin@koconnor.net>
> > + * Copyright (C) 2006 Fabrice Bellard
> > + * Copyright (C) 2013 Red Hat Inc
> > + * Copyright (c) 2015 HUAWEI TECHNOLOGIES CO.,LTD.
> > + * Copyright (C) 2021-2023 Ventana Micro Systems Inc
> > + *
> > + * 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 "qemu/osdep.h"
> > +#include "hw/acpi/acpi-defs.h"
> > +#include "hw/acpi/acpi.h"
> > +#include "hw/acpi/aml-build.h"
> > +#include "hw/acpi/utils.h"
> > +#include "qapi/error.h"
> > +#include "sysemu/reset.h"
> > +#include "migration/vmstate.h"
> > +#include "hw/riscv/virt.h"
> > +
> > +#define ACPI_BUILD_TABLE_SIZE             0x20000
> > +
> > +typedef struct AcpiBuildState {
> > +    /* Copy of table in RAM (for patching) */
> > +    MemoryRegion *table_mr;
> > +    MemoryRegion *rsdp_mr;
> > +    MemoryRegion *linker_mr;
> > +    /* Is table patched? */
> > +    bool patched;
> > +} AcpiBuildState;
> > +
> > +static void acpi_dsdt_add_cpus(Aml *scope, RISCVVirtState *s)
> > +{
> > +    MachineState *ms = MACHINE(s);
> > +    uint16_t i;
> > +
> > +    for (i = 0; i < ms->smp.cpus; i++) {
> > +        Aml *dev = aml_device("C%.03X", i);
> > +        aml_append(dev, aml_name_decl("_HID", aml_string("ACPI0007")));
> > +        aml_append(dev, aml_name_decl("_UID", aml_int(i)));
> > +        aml_append(scope, dev);
> > +    }
> > +}
> > +
> > +static void acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry 
> > *fw_cfg_memmap)
> > +{
> > +    Aml *dev = aml_device("FWCF");
> > +    aml_append(dev, aml_name_decl("_HID", aml_string("QEMU0002")));
> > +
> > +    /* device present, functioning, decoding, not shown in UI */
> > +    aml_append(dev, aml_name_decl("_STA", aml_int(0xB)));
> > +    aml_append(dev, aml_name_decl("_CCA", aml_int(1)));
> > +
> > +    Aml *crs = aml_resource_template();
> > +    aml_append(crs, aml_memory32_fixed(fw_cfg_memmap->base,
> > +                                       fw_cfg_memmap->size, 
> > AML_READ_WRITE));
> > +    aml_append(dev, aml_name_decl("_CRS", crs));
> > +    aml_append(scope, dev);
> > +}
> > +
> > +/* FADT */
> > +static void build_fadt_rev6(GArray *table_data,
> > +                            BIOSLinker *linker,
> > +                            RISCVVirtState *s,
> > +                            unsigned dsdt_tbl_offset)
> > +{
> > +    AcpiFadtData fadt = {
> > +        .rev = 6,
> > +        .minor_ver = 0,
> > +        .flags = 1 << ACPI_FADT_F_HW_REDUCED_ACPI,
> > +        .xdsdt_tbl_offset = &dsdt_tbl_offset,
> > +    };
> > +
> > +    build_fadt(table_data, linker, &fadt, s->oem_id, s->oem_table_id);
> > +}
> > +
> > +/* DSDT */
> > +static void build_dsdt(GArray *table_data,
> > +                       BIOSLinker *linker,
> > +                       RISCVVirtState *s)
> > +{
> > +    Aml *scope, *dsdt;
> > +    const MemMapEntry *memmap = s->memmap;
> > +    AcpiTable table = { .sig = "DSDT", .rev = 2, .oem_id = s->oem_id,
> > +                        .oem_table_id = s->oem_table_id };
> > +
> > +
> > +    acpi_table_begin(&table, table_data);
> > +    dsdt = init_aml_allocator();
> > +
> > +    /*
> > +     * When booting the VM with UEFI, UEFI takes ownership of the RTC 
> > hardware.
> > +     * While UEFI can use libfdt to disable the RTC device node in the DTB 
> > that
> > +     * it passes to the OS, it cannot modify AML. Therefore, we won't 
> > generate
> > +     * the RTC ACPI device at all when using UEFI.
> > +     */
> > +    scope = aml_scope("\\_SB");
> > +    acpi_dsdt_add_cpus(scope, s);
> > +
> > +    acpi_dsdt_add_fw_cfg(scope, &memmap[VIRT_FW_CFG]);
> > +
> > +    aml_append(dsdt, scope);
> > +
> > +    /* copy AML table into ACPI tables blob and patch header there */
> > +    g_array_append_vals(table_data, dsdt->buf->data, dsdt->buf->len);
> > +
> > +    acpi_table_end(linker, &table);
> > +    free_aml_allocator();
> > +}
> > +
> > +static void virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
> > +{
> > +    GArray *table_offsets;
> > +    unsigned dsdt, xsdt;
> > +    GArray *tables_blob = tables->table_data;
> > +
> > +    table_offsets = g_array_new(false, true,
> > +                                sizeof(uint32_t));
> > +
> > +    bios_linker_loader_alloc(tables->linker,
> > +                             ACPI_BUILD_TABLE_FILE, tables_blob,
> > +                             64, false);
> > +
> > +    /* DSDT is pointed to by FADT */
> > +    dsdt = tables_blob->len;
> > +    build_dsdt(tables_blob, tables->linker, s);
> > +
> > +    /* FADT and others pointed to by XSDT */
> > +    acpi_add_table(table_offsets, tables_blob);
> > +    build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
> > +
> > +    /* XSDT is pointed to by RSDP */
> > +    xsdt = tables_blob->len;
> > +    build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
> > +                s->oem_table_id);
> > +
> > +    /* RSDP is in FSEG memory, so allocate it separately */
> > +    {
> > +        AcpiRsdpData rsdp_data = {
> > +            .revision = 2,
> > +            .oem_id = s->oem_id,
> > +            .xsdt_tbl_offset = &xsdt,
> > +            .rsdt_tbl_offset = NULL,
> > +        };
> > +        build_rsdp(tables->rsdp, tables->linker, &rsdp_data);
> > +    }
> > +
> > +    /*
> > +     * The align size is 128, warn if 64k is not enough therefore
> > +     * the align size could be resized.
> > +     */
> > +    if (tables_blob->len > ACPI_BUILD_TABLE_SIZE / 2) {
> > +        warn_report("ACPI table size %u exceeds %d bytes,"
> > +                    " migration may not work",
> > +                    tables_blob->len, ACPI_BUILD_TABLE_SIZE / 2);
> > +        error_printf("Try removing some objects.");
> > +    }
> > +    g_array_set_size(tables_blob, ROUND_UP(acpi_data_len(tables_blob),
> > +                     ACPI_BUILD_TABLE_SIZE));
> 
> I would have kept acpi_align_size(), as it'll be easier to find the
> duplication when refactoring later.
> 
Okay.. Let me add this back. Will wait till Friday to check if there are
any other comments and send updated series.

Thanks!
Sunil