[V2,04/10] hw/riscv/virt: virt-acpi-build.c: Add basic ACPI tables

Add few basic ACPI tables and DSDT with few devices in a
new file virt-acpi-build.c.

These are mostly leveraged from arm64.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/virt-acpi-build.c | 285 +++++++++++++++++++++++++++++++++++++
 include/hw/riscv/virt.h    |   1 +
 2 files changed, 286 insertions(+)
 create mode 100644 hw/riscv/virt-acpi-build.c

Sunil,

This patch is a bit confusing to me. You're using functions that doesn't exist
in the code base yet (build_madt and build_rhct) because they are introduced
in later patches. This also means that this patch is not being compiled tested,
because otherwise it would throw a compile error. And the build of the file only
happens after patch 8.

Now, there is no hard rule in QEMU that dictates that every patch must be compile
tested, but nevertheless this is a good rule to follow that makes our lives easier
when bisecting and cherry-pick individual patches.

My suggestion for this patch is:

- squash both patches 7 and 8 into this patch to allow the file to be built;

- remove the code that is referring to stuff that you haven't add yet:

$ git diff
diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 3c4da6c385..eb17029b64 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -156,12 +156,6 @@ virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
      acpi_add_table(table_offsets, tables_blob);
      build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
  
-    acpi_add_table(table_offsets, tables_blob);
-    build_madt(tables_blob, tables->linker, s);
-
-    acpi_add_table(table_offsets, tables_blob);
-    build_rhct(tables_blob, tables->linker, s);
-
      /* XSDT is pointed to by RSDP */
      xsdt = tables_blob->len;
      build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,


- in patch 5, add back the lines in virt_acpi_build() that uses build_madt();

- in patch 6, add back the lines in virt_acpi_build() that uses build_rhct();


This will make this patch to be compiled and built right away without interfering with
the end result of the series.


One more suggestion:


On 2/13/23 11:40, Sunil V L wrote:
> Add few basic ACPI tables and DSDT with few devices in a
> new file virt-acpi-build.c.
> 
> These are mostly leveraged from arm64.

Quick rant that you've already heard: I don't really understand why there is so
much ACPI code duplication everywhere. I really don't. E.g. acpi_align_size() is
copied verbatim in hw/arm/virt-acpi-build.c, hw/i386/acpi-build.c and
hw/loongarch/acpi-build.c. I don't see why we can't have a common file in hw/acpi
with helpers and so on that every ACPI architecture can use, and then the
individual drivers for each arch can have its own magic sauce.

All this said, instead of mentioning "this is mostly leveraged from arm64", you
can make a brief summary of the changes you've done from the arm64 version. This
will help guide the review into focusing on the novel code you're adding and
ignore the copied bits.


Thanks,


Daniel


> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>   hw/riscv/virt-acpi-build.c | 285 +++++++++++++++++++++++++++++++++++++
>   include/hw/riscv/virt.h    |   1 +
>   2 files changed, 286 insertions(+)
>   create mode 100644 hw/riscv/virt-acpi-build.c
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> new file mode 100644
> index 0000000000..3c4da6c385
> --- /dev/null
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -0,0 +1,285 @@
> +/*
> + * 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/riscv/virt.h"
> +#include "hw/riscv/numa.h"
> +#include "hw/acpi/pci.h"
> +#include "hw/acpi/utils.h"
> +#include "sysemu/reset.h"
> +#include "hw/pci-host/gpex.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.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_align_size(GArray *blob, unsigned align)
> +{
> +    /*
> +     * Align size to multiple of given size. This reduces the chance
> +     * we need to change size in the future (breaking cross version migration).
> +     */
> +    g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
> +}
> +
> +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)
> +{
> +    /* ACPI v5.1 */
> +    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 RSDT */
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
> +
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_madt(tables_blob, tables->linker, s);
> +
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_rhct(tables_blob, tables->linker, s);
> +
> +    /* 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 CPUs, NUMA nodes, memory slots"
> +                     " or PCI bridges.");
> +    }
> +    acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> +
> +
> +    /* Clean up memory that's no longer used */
> +    g_array_free(table_offsets, true);
> +}
> +
> +static void acpi_ram_update(MemoryRegion *mr, GArray *data)
> +{
> +    uint32_t size = acpi_data_len(data);
> +
> +    /*
> +     * Make sure RAM size is correct - in case it got changed
> +     * e.g. by migration
> +     */
> +    memory_region_ram_resize(mr, size, &error_abort);
> +
> +    memcpy(memory_region_get_ram_ptr(mr), data->data, size);
> +    memory_region_set_dirty(mr, 0, size);
> +}
> +
> +static void virt_acpi_build_update(void *build_opaque)
> +{
> +    AcpiBuildState *build_state = build_opaque;
> +    AcpiBuildTables tables;
> +
> +    /* No state to update or already patched? Nothing to do. */
> +    if (!build_state || build_state->patched) {
> +        return;
> +    }
> +    build_state->patched = true;
> +
> +    acpi_build_tables_init(&tables);
> +
> +    virt_acpi_build(RISCV_VIRT_MACHINE(qdev_get_machine()), &tables);
> +
> +    acpi_ram_update(build_state->table_mr, tables.table_data);
> +    acpi_ram_update(build_state->rsdp_mr, tables.rsdp);
> +    acpi_ram_update(build_state->linker_mr, tables.linker->cmd_blob);
> +
> +    acpi_build_tables_cleanup(&tables, true);
> +}
> +
> +static void virt_acpi_build_reset(void *build_opaque)
> +{
> +    AcpiBuildState *build_state = build_opaque;
> +    build_state->patched = false;
> +}
> +
> +static const VMStateDescription vmstate_virt_acpi_build = {
> +    .name = "virt_acpi_build",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(patched, AcpiBuildState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +void virt_acpi_setup(RISCVVirtState *s)
> +{
> +    AcpiBuildTables tables;
> +    AcpiBuildState *build_state;
> +
> +    build_state = g_malloc0(sizeof *build_state);
> +
> +    acpi_build_tables_init(&tables);
> +    virt_acpi_build(s, &tables);
> +
> +    /* Now expose it all to Guest */
> +    build_state->table_mr = acpi_add_rom_blob(virt_acpi_build_update,
> +                                              build_state, tables.table_data,
> +                                              ACPI_BUILD_TABLE_FILE);
> +    assert(build_state->table_mr != NULL);
> +
> +    build_state->linker_mr = acpi_add_rom_blob(virt_acpi_build_update,
> +                                               build_state,
> +                                               tables.linker->cmd_blob,
> +                                               ACPI_BUILD_LOADER_FILE);
> +
> +    build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update,
> +                                             build_state, tables.rsdp,
> +                                             ACPI_BUILD_RSDP_FILE);
> +
> +    qemu_register_reset(virt_acpi_build_reset, build_state);
> +    virt_acpi_build_reset(build_state);
> +    vmstate_register(NULL, 0, &vmstate_virt_acpi_build, build_state);
> +
> +    /*
> +     * Clean up tables but don't free the memory: we track it
> +     * in build_state.
> +     */
> +    acpi_build_tables_cleanup(&tables, false);
> +}
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 379501edcc..e5c474b26e 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -126,4 +126,5 @@ enum {
>                                    1 + FDT_APLIC_INT_CELLS)
>   
>   bool virt_is_acpi_enabled(RISCVVirtState *s);
> +void virt_acpi_setup(RISCVVirtState *vms);
>   #endif

On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
> Sunil,
> 
> This patch is a bit confusing to me. You're using functions that doesn't exist
> in the code base yet (build_madt and build_rhct) because they are introduced
> in later patches. This also means that this patch is not being compiled tested,
> because otherwise it would throw a compile error. And the build of the file only
> happens after patch 8.
> 
My intention was to add the caller also in the same patch where the
function is added. I think I missed it when I split. Thanks!

> Now, there is no hard rule in QEMU that dictates that every patch must be compile
> tested, but nevertheless this is a good rule to follow that makes our lives easier
> when bisecting and cherry-pick individual patches.
> 
> My suggestion for this patch is:
> 
> - squash both patches 7 and 8 into this patch to allow the file to be built;
> 
Sure.

> - remove the code that is referring to stuff that you haven't add yet:
> 
> $ git diff
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 3c4da6c385..eb17029b64 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -156,12 +156,6 @@ virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
>      acpi_add_table(table_offsets, tables_blob);
>      build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
> -    acpi_add_table(table_offsets, tables_blob);
> -    build_madt(tables_blob, tables->linker, s);
> -
> -    acpi_add_table(table_offsets, tables_blob);
> -    build_rhct(tables_blob, tables->linker, s);
> -
>      /* XSDT is pointed to by RSDP */
>      xsdt = tables_blob->len;
>      build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
> 
> 
> - in patch 5, add back the lines in virt_acpi_build() that uses build_madt();
> 
> - in patch 6, add back the lines in virt_acpi_build() that uses build_rhct();
> 
> 
> This will make this patch to be compiled and built right away without interfering with
> the end result of the series.
>
Thanks!
 
> 
> One more suggestion:
> 
> 
> On 2/13/23 11:40, Sunil V L wrote:
> > Add few basic ACPI tables and DSDT with few devices in a
> > new file virt-acpi-build.c.
> > 
> > These are mostly leveraged from arm64.
> 
> Quick rant that you've already heard: I don't really understand why there is so
> much ACPI code duplication everywhere. I really don't. E.g. acpi_align_size() is
> copied verbatim in hw/arm/virt-acpi-build.c, hw/i386/acpi-build.c and
> hw/loongarch/acpi-build.c. I don't see why we can't have a common file in hw/acpi
> with helpers and so on that every ACPI architecture can use, and then the
> individual drivers for each arch can have its own magic sauce.
>
I completely agree that we better avoid duplication But I am bit
hesitant in this case because,
1) Low level functions which help in creating the namespace/static
tables are already common (ex: aml_append)

2) Using these basic routines, individual platforms can create the
namespace with appropriate devices and the methods.

ACPI name space is tightly coupled with a platform. While there may be
common devices like processors, uart etc, there can be difference in the
ACPI methods for each of those devices. For ex: CPU objects for one
platform may support _LPI method. uart may support _DSD for one platform
and other may use totally different UART. If we have to create common routines,
we will have to decide on all parameters the common function would need for
different platforms. Same concern with fw_cfg/virtio etc which look same
now but in future one platform can add a new method for these devices.

IMHO, even though it looks like we have the same function in each architecture
currently, this model allows us to have platforms with different devices with
different methods/features. Creating common routines now would probably make
them difficult to use in future. 

acpi_align_size() is a simple wrapper. We don't need it if we directly
use the common function.

Since I see insistence let me try moving few functions like fw_cfg (virtio, pci in
future) to a common file in hw/acpi.
 
> All this said, instead of mentioning "this is mostly leveraged from arm64", you
> can make a brief summary of the changes you've done from the arm64 version. This
> will help guide the review into focusing on the novel code you're adding and
> ignore the copied bits.
> 
Sure.

Thanks!
Sunil

On Tue, Feb 14, 2023 at 11:43 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
> > Sunil,
> >
> > This patch is a bit confusing to me. You're using functions that doesn't exist
> > in the code base yet (build_madt and build_rhct) because they are introduced
> > in later patches. This also means that this patch is not being compiled tested,
> > because otherwise it would throw a compile error. And the build of the file only
> > happens after patch 8.
> >
> My intention was to add the caller also in the same patch where the
> function is added. I think I missed it when I split. Thanks!
>
> > Now, there is no hard rule in QEMU that dictates that every patch must be compile
> > tested, but nevertheless this is a good rule to follow that makes our lives easier
> > when bisecting and cherry-pick individual patches.
> >
> > My suggestion for this patch is:
> >
> > - squash both patches 7 and 8 into this patch to allow the file to be built;
> >
> Sure.
>
> > - remove the code that is referring to stuff that you haven't add yet:
> >
> > $ git diff
> > diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> > index 3c4da6c385..eb17029b64 100644
> > --- a/hw/riscv/virt-acpi-build.c
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -156,12 +156,6 @@ virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
> >      acpi_add_table(table_offsets, tables_blob);
> >      build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
> > -    acpi_add_table(table_offsets, tables_blob);
> > -    build_madt(tables_blob, tables->linker, s);
> > -
> > -    acpi_add_table(table_offsets, tables_blob);
> > -    build_rhct(tables_blob, tables->linker, s);
> > -
> >      /* XSDT is pointed to by RSDP */
> >      xsdt = tables_blob->len;
> >      build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
> >
> >
> > - in patch 5, add back the lines in virt_acpi_build() that uses build_madt();
> >
> > - in patch 6, add back the lines in virt_acpi_build() that uses build_rhct();
> >
> >
> > This will make this patch to be compiled and built right away without interfering with
> > the end result of the series.
> >
> Thanks!
>
> >
> > One more suggestion:
> >
> >
> > On 2/13/23 11:40, Sunil V L wrote:
> > > Add few basic ACPI tables and DSDT with few devices in a
> > > new file virt-acpi-build.c.
> > >
> > > These are mostly leveraged from arm64.
> >
> > Quick rant that you've already heard: I don't really understand why there is so
> > much ACPI code duplication everywhere. I really don't. E.g. acpi_align_size() is
> > copied verbatim in hw/arm/virt-acpi-build.c, hw/i386/acpi-build.c and
> > hw/loongarch/acpi-build.c. I don't see why we can't have a common file in hw/acpi
> > with helpers and so on that every ACPI architecture can use, and then the
> > individual drivers for each arch can have its own magic sauce.
> >

Yes, I had the same concern with Daniel when looking at the v1 series.

I am hoping the ACPI maintainers could make a decision, whether we
allow another architecture to do the duplication in their arch tree,
or we spend some time refactoring the ACPI table generation stuff.

+Michael and Igor.

> I completely agree that we better avoid duplication But I am bit
> hesitant in this case because,
> 1) Low level functions which help in creating the namespace/static
> tables are already common (ex: aml_append)
>
> 2) Using these basic routines, individual platforms can create the
> namespace with appropriate devices and the methods.
>
> ACPI name space is tightly coupled with a platform. While there may be
> common devices like processors, uart etc, there can be difference in the
> ACPI methods for each of those devices. For ex: CPU objects for one
> platform may support _LPI method. uart may support _DSD for one platform
> and other may use totally different UART. If we have to create common routines,
> we will have to decide on all parameters the common function would need for
> different platforms. Same concern with fw_cfg/virtio etc which look same
> now but in future one platform can add a new method for these devices.
>
> IMHO, even though it looks like we have the same function in each architecture
> currently, this model allows us to have platforms with different devices with
> different methods/features. Creating common routines now would probably make
> them difficult to use in future.
>
> acpi_align_size() is a simple wrapper. We don't need it if we directly
> use the common function.
>
> Since I see insistence let me try moving few functions like fw_cfg (virtio, pci in
> future) to a common file in hw/acpi.
>
> > All this said, instead of mentioning "this is mostly leveraged from arm64", you
> > can make a brief summary of the changes you've done from the arm64 version. This
> > will help guide the review into focusing on the novel code you're adding and
> > ignore the copied bits.
> >
> Sure.

Regards,
Bin

On Tue, Feb 14, 2023 at 09:13:28AM +0530, Sunil V L wrote:
> On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
> > Sunil,
> > 
> > This patch is a bit confusing to me. You're using functions that doesn't exist
> > in the code base yet (build_madt and build_rhct) because they are introduced
> > in later patches. This also means that this patch is not being compiled tested,
> > because otherwise it would throw a compile error. And the build of the file only
> > happens after patch 8.
> > 
> My intention was to add the caller also in the same patch where the
> function is added. I think I missed it when I split. Thanks!
>

Before posting a series I try to remember to do the following check

$ ./configure ...
$ git rebase -i -x 'make -j' $BASE_COMMIT_FOR_SERIES

Thanks,
drew

On 2/14/23 00:43, Sunil V L wrote:
> On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
>> Sunil,
>>
>> This patch is a bit confusing to me. You're using functions that doesn't exist
>> in the code base yet (build_madt and build_rhct) because they are introduced
>> in later patches. This also means that this patch is not being compiled tested,
>> because otherwise it would throw a compile error. And the build of the file only
>> happens after patch 8.
>>
> My intention was to add the caller also in the same patch where the
> function is added. I think I missed it when I split. Thanks!
> 
>> Now, there is no hard rule in QEMU that dictates that every patch must be compile
>> tested, but nevertheless this is a good rule to follow that makes our lives easier
>> when bisecting and cherry-pick individual patches.
>>
>> My suggestion for this patch is:
>>
>> - squash both patches 7 and 8 into this patch to allow the file to be built;
>>
> Sure.
> 
>> - remove the code that is referring to stuff that you haven't add yet:
>>
>> $ git diff
>> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
>> index 3c4da6c385..eb17029b64 100644
>> --- a/hw/riscv/virt-acpi-build.c
>> +++ b/hw/riscv/virt-acpi-build.c
>> @@ -156,12 +156,6 @@ virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
>>       acpi_add_table(table_offsets, tables_blob);
>>       build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
>> -    acpi_add_table(table_offsets, tables_blob);
>> -    build_madt(tables_blob, tables->linker, s);
>> -
>> -    acpi_add_table(table_offsets, tables_blob);
>> -    build_rhct(tables_blob, tables->linker, s);
>> -
>>       /* XSDT is pointed to by RSDP */
>>       xsdt = tables_blob->len;
>>       build_xsdt(tables_blob, tables->linker, table_offsets, s->oem_id,
>>
>>
>> - in patch 5, add back the lines in virt_acpi_build() that uses build_madt();
>>
>> - in patch 6, add back the lines in virt_acpi_build() that uses build_rhct();
>>
>>
>> This will make this patch to be compiled and built right away without interfering with
>> the end result of the series.
>>
> Thanks!
>   
>>
>> One more suggestion:
>>
>>
>> On 2/13/23 11:40, Sunil V L wrote:
>>> Add few basic ACPI tables and DSDT with few devices in a
>>> new file virt-acpi-build.c.
>>>
>>> These are mostly leveraged from arm64.
>>
>> Quick rant that you've already heard: I don't really understand why there is so
>> much ACPI code duplication everywhere. I really don't. E.g. acpi_align_size() is
>> copied verbatim in hw/arm/virt-acpi-build.c, hw/i386/acpi-build.c and
>> hw/loongarch/acpi-build.c. I don't see why we can't have a common file in hw/acpi
>> with helpers and so on that every ACPI architecture can use, and then the
>> individual drivers for each arch can have its own magic sauce.
>>
> I completely agree that we better avoid duplication But I am bit
> hesitant in this case because,
> 1) Low level functions which help in creating the namespace/static
> tables are already common (ex: aml_append)
> 
> 2) Using these basic routines, individual platforms can create the
> namespace with appropriate devices and the methods.
> 
> ACPI name space is tightly coupled with a platform. While there may be
> common devices like processors, uart etc, there can be difference in the
> ACPI methods for each of those devices. For ex: CPU objects for one
> platform may support _LPI method. uart may support _DSD for one platform
> and other may use totally different UART. If we have to create common routines,
> we will have to decide on all parameters the common function would need for
> different platforms. Same concern with fw_cfg/virtio etc which look same
> now but in future one platform can add a new method for these devices.
> 
> IMHO, even though it looks like we have the same function in each architecture
> currently, this model allows us to have platforms with different devices with
> different methods/features. Creating common routines now would probably make
> them difficult to use in future.
> 
> acpi_align_size() is a simple wrapper. We don't need it if we directly
> use the common function.
> 
> Since I see insistence let me try moving few functions like fw_cfg (virtio, pci in
> future) to a common file in hw/acpi.

Nah. Doing that now will make this series rely on acks for every other ACPI arch to
push the RISC-V side.

Let's make this happen as is now to get ACPI in RISC-V working. We can think about
reducing overall ACPI duplication later. IMO it's enough for now to, mention in this
commit msg, which bits of the arm64 virt-acpi-build.c you changed for this RISC-V
version.


Thanks,

Daniel

>   
>> All this said, instead of mentioning "this is mostly leveraged from arm64", you
>> can make a brief summary of the changes you've done from the arm64 version. This
>> will help guide the review into focusing on the novel code you're adding and
>> ignore the copied bits.
>>
> Sure.
> 
> Thanks!
> Sunil

On Mon, Feb 13, 2023 at 08:10:32PM +0530, Sunil V L wrote:
> Add few basic ACPI tables and DSDT with few devices in a
> new file virt-acpi-build.c.
> 
> These are mostly leveraged from arm64.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt-acpi-build.c | 285 +++++++++++++++++++++++++++++++++++++
>  include/hw/riscv/virt.h    |   1 +
>  2 files changed, 286 insertions(+)
>  create mode 100644 hw/riscv/virt-acpi-build.c
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> new file mode 100644
> index 0000000000..3c4da6c385
> --- /dev/null
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -0,0 +1,285 @@
> +/*
> + * 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/riscv/virt.h"
> +#include "hw/riscv/numa.h"
> +#include "hw/acpi/pci.h"
> +#include "hw/acpi/utils.h"
> +#include "sysemu/reset.h"
> +#include "hw/pci-host/gpex.h"

We should only bring in includes as needed. Several of these are currently
unused.

> +#include "qapi/error.h"
> +#include "migration/vmstate.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_align_size(GArray *blob, unsigned align)
> +{
> +    /*
> +     * Align size to multiple of given size. This reduces the chance
> +     * we need to change size in the future (breaking cross version migration).
> +     */
> +    g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align));
> +}
> +
> +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)

Please avoid the use of this style. It's better to wrap parameters, e.g.

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)
> +{
> +    /* ACPI v5.1 */
> +    AcpiFadtData fadt = {
> +        .rev = 6,
> +        .minor_ver = 0,

Comment above says ACPI v5.1, but here it uses 6.0. I'd just drop the
comment.

> +        .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 RSDT */
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_fadt_rev6(tables_blob, tables->linker, s, dsdt);
> +
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_madt(tables_blob, tables->linker, s);
> +
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_rhct(tables_blob, tables->linker, s);
> +
> +    /* 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 CPUs, NUMA nodes, memory slots"
> +                     " or PCI bridges.");

This error mentions things for which we don't currently generate tables.

> +    }
> +    acpi_align_size(tables_blob, ACPI_BUILD_TABLE_SIZE);
> +
> +
> +    /* Clean up memory that's no longer used */
> +    g_array_free(table_offsets, true);
> +}
> +
> +static void acpi_ram_update(MemoryRegion *mr, GArray *data)
> +{
> +    uint32_t size = acpi_data_len(data);
> +
> +    /*
> +     * Make sure RAM size is correct - in case it got changed
> +     * e.g. by migration
> +     */
> +    memory_region_ram_resize(mr, size, &error_abort);
> +
> +    memcpy(memory_region_get_ram_ptr(mr), data->data, size);
> +    memory_region_set_dirty(mr, 0, size);
> +}
> +
> +static void virt_acpi_build_update(void *build_opaque)
> +{
> +    AcpiBuildState *build_state = build_opaque;
> +    AcpiBuildTables tables;
> +
> +    /* No state to update or already patched? Nothing to do. */
> +    if (!build_state || build_state->patched) {
> +        return;
> +    }
> +    build_state->patched = true;
> +
> +    acpi_build_tables_init(&tables);
> +
> +    virt_acpi_build(RISCV_VIRT_MACHINE(qdev_get_machine()), &tables);
> +
> +    acpi_ram_update(build_state->table_mr, tables.table_data);
> +    acpi_ram_update(build_state->rsdp_mr, tables.rsdp);
> +    acpi_ram_update(build_state->linker_mr, tables.linker->cmd_blob);
> +
> +    acpi_build_tables_cleanup(&tables, true);
> +}
> +
> +static void virt_acpi_build_reset(void *build_opaque)
> +{
> +    AcpiBuildState *build_state = build_opaque;
> +    build_state->patched = false;
> +}
> +
> +static const VMStateDescription vmstate_virt_acpi_build = {
> +    .name = "virt_acpi_build",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_BOOL(patched, AcpiBuildState),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +void virt_acpi_setup(RISCVVirtState *s)
> +{
> +    AcpiBuildTables tables;
> +    AcpiBuildState *build_state;
> +
> +    build_state = g_malloc0(sizeof *build_state);
> +
> +    acpi_build_tables_init(&tables);
> +    virt_acpi_build(s, &tables);
> +
> +    /* Now expose it all to Guest */
> +    build_state->table_mr = acpi_add_rom_blob(virt_acpi_build_update,
> +                                              build_state, tables.table_data,
> +                                              ACPI_BUILD_TABLE_FILE);
> +    assert(build_state->table_mr != NULL);
> +
> +    build_state->linker_mr = acpi_add_rom_blob(virt_acpi_build_update,
> +                                               build_state,
> +                                               tables.linker->cmd_blob,
> +                                               ACPI_BUILD_LOADER_FILE);
> +
> +    build_state->rsdp_mr = acpi_add_rom_blob(virt_acpi_build_update,
> +                                             build_state, tables.rsdp,
> +                                             ACPI_BUILD_RSDP_FILE);
> +
> +    qemu_register_reset(virt_acpi_build_reset, build_state);
> +    virt_acpi_build_reset(build_state);
> +    vmstate_register(NULL, 0, &vmstate_virt_acpi_build, build_state);
> +
> +    /*
> +     * Clean up tables but don't free the memory: we track it
> +     * in build_state.
> +     */
> +    acpi_build_tables_cleanup(&tables, false);
> +}
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 379501edcc..e5c474b26e 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -126,4 +126,5 @@ enum {
>                                   1 + FDT_APLIC_INT_CELLS)
>  
>  bool virt_is_acpi_enabled(RISCVVirtState *s);
> +void virt_acpi_setup(RISCVVirtState *vms);
>  #endif
> -- 
> 2.34.1
>

Thanks,
drew

On Tue, Feb 14, 2023 at 05:44:44AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> On 2/14/23 00:43, Sunil V L wrote:
> > On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
> 
> Nah. Doing that now will make this series rely on acks for every other ACPI arch to
> push the RISC-V side.
> 
> Let's make this happen as is now to get ACPI in RISC-V working. We can think about
> reducing overall ACPI duplication later. IMO it's enough for now to, mention in this
> commit msg, which bits of the arm64 virt-acpi-build.c you changed for this RISC-V
> version.
> 

Okay. Thanks!. Will update the commit message and send the V3 soon.

Thanks
Sunil

On Wed, 15 Feb 2023 06:08:10 PST (-0800), sunilvl@ventanamicro.com wrote:
> On Tue, Feb 14, 2023 at 05:44:44AM -0300, Daniel Henrique Barboza wrote:
>>
>>
>> On 2/14/23 00:43, Sunil V L wrote:
>> > On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
>>
>> Nah. Doing that now will make this series rely on acks for every other ACPI arch to
>> push the RISC-V side.
>>
>> Let's make this happen as is now to get ACPI in RISC-V working. We can think about
>> reducing overall ACPI duplication later. IMO it's enough for now to, mention in this
>> commit msg, which bits of the arm64 virt-acpi-build.c you changed for this RISC-V
>> version.
>>
>
> Okay. Thanks!. Will update the commit message and send the V3 soon.

I'm checking up on this one as I don't see a v3 on the lists.  No rush 
on my end, I'm just trying to make sure I don't drop the ball on 
anything from the backlog as I'm catching up.

Thanks!

>
> Thanks
> Sunil

On Thu, Feb 16, 2023 at 08:26:21AM -0800, Palmer Dabbelt wrote:
> On Wed, 15 Feb 2023 06:08:10 PST (-0800), sunilvl@ventanamicro.com wrote:
> > On Tue, Feb 14, 2023 at 05:44:44AM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > 
> > > On 2/14/23 00:43, Sunil V L wrote:
> > > > On Mon, Feb 13, 2023 at 03:48:04PM -0300, Daniel Henrique Barboza wrote:
> > > 
> > > Nah. Doing that now will make this series rely on acks for every other ACPI arch to
> > > push the RISC-V side.
> > > 
> > > Let's make this happen as is now to get ACPI in RISC-V working. We can think about
> > > reducing overall ACPI duplication later. IMO it's enough for now to, mention in this
> > > commit msg, which bits of the arm64 virt-acpi-build.c you changed for this RISC-V
> > > version.
> > > 
> > 
> > Okay. Thanks!. Will update the commit message and send the V3 soon.
> 
> I'm checking up on this one as I don't see a v3 on the lists.  No rush on my
> end, I'm just trying to make sure I don't drop the ball on anything from the
> backlog as I'm catching up.
> 
Thanks!, Plamer. I have sent V3 which is based on Alistair's
riscv-to-apply.next branch.

Thanks,
Sunil