[PATCH V2 00/10] Add basic ACPI support for risc-v virt

[PATCH V2 00/10] Add basic ACPI support for risc-v virt

[Sunil V L]

This series adds the basic ACPI support for the RISC-V virt machine. 
Currently only INTC interrupt controller specification is approved by the
UEFI forum. External interrupt controller support in ACPI is in progress.

The basic infrastructure changes are mostly leveraged from ARM.

This adds support for INTC and RHCT tables as specified in below ECR links
which are approved by UEFI forum.
RINTC - https://drive.google.com/file/d/1R6k4MshhN3WTT-hwqAquu5nX6xSEqK2l/view
RHCT - https://drive.google.com/file/d/1nP3nFiH4jkPMp6COOxP6123DCZKR-tia/view

These changes are also available @
https://github.com/vlsunil/qemu/tree/acpi_b1_us_review_V2

Changes since V1:
	1) Addressed comments from Bin Meng.
	2) Made acpi switch default AUTO similar to other architectures.
	3) Re-based and added RB and ACKs.

The series is tested using SBI HVC console and initrd.

Test instructions:
1) Build Qemu with ACPI support (this series)

2) Build EDK2 as per instructions in
https://github.com/vlsunil/riscv-uefi-edk2-docs/wiki/RISC-V-Qemu-Virt-support

3) Build Linux with ACPI support using below branch
https://github.com/vlsunil/linux/commits/acpi_b1_us_review_ipi17_V2
after enabling SBI HVC and SBI earlycon options.

CONFIG_RISCV_SBI_V01=y
CONFIG_SERIAL_EARLYCON_RISCV_SBI=y
CONFIG_HVC_RISCV_SBI=y

4) Build buildroot.

Run with below command.
qemu-system-riscv64   -nographic \
-drive file=Build/RiscVVirtQemu/RELEASE_GCC5/FV/RISCV_VIRT.fd,if=pflash,format=raw,unit=1 \
-machine virt -smp 16 -m 2G \
-kernel arch/riscv/boot/Image \
-initrd buildroot/output/images/rootfs.cpio \
-append "root=/dev/ram ro console=hvc0 earlycon=sbi"

Sunil V L (10):
  hw/riscv/virt: Add OEM_ID and OEM_TABLE_ID fields
  hw/riscv/virt: Add a switch to enable/disable ACPI
  hw/riscv/virt: Add memmap pointer to RiscVVirtState
  hw/riscv/virt: virt-acpi-build.c: Add basic ACPI tables
  hw/riscv/virt: virt-acpi-build.c: Add RINTC in MADT
  hw/riscv/virt: virt-acpi-build.c: Add RHCT Table
  hw/riscv: meson.build: Build virt-acpi-build.c
  hw/riscv/Kconfig: virt: Enable ACPI config option
  hw/riscv/virt.c: Initialize the ACPI tables
  MAINTAINERS: Add entry for RISC-V ACPI

 MAINTAINERS                |   6 +
 hw/riscv/Kconfig           |   1 +
 hw/riscv/meson.build       |   1 +
 hw/riscv/virt-acpi-build.c | 384 +++++++++++++++++++++++++++++++++++++
 hw/riscv/virt.c            |  45 +++++
 include/hw/riscv/virt.h    |   6 +
 6 files changed, 443 insertions(+)
 create mode 100644 hw/riscv/virt-acpi-build.c

-- 
2.34.1

[PATCH V2 01/10] hw/riscv/virt: Add OEM_ID and OEM_TABLE_ID fields

[Sunil V L]

ACPI needs OEM_ID and OEM_TABLE_ID for the machine. Add these fields
in the RISCVVirtState structure and initialize with default values.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c         | 4 ++++
 include/hw/riscv/virt.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 86c4adc0c9..fb68cf81e9 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -49,6 +49,7 @@
 #include "hw/pci/pci.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/display/ramfb.h"
+#include "hw/acpi/aml-build.h"
 
 /*
  * The virt machine physical address space used by some of the devices
@@ -1504,6 +1505,9 @@ static void virt_machine_init(MachineState *machine)
     }
     virt_flash_map(s, system_memory);
 
+    s->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
+    s->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
+
     /* create device tree */
     create_fdt(s, memmap);
 
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index b3d26135c0..6c7885bf89 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -56,6 +56,8 @@ struct RISCVVirtState {
     bool have_aclint;
     RISCVVirtAIAType aia_type;
     int aia_guests;
+    char *oem_id;
+    char *oem_table_id;
 };
 
 enum {
-- 
2.34.1

[PATCH V2 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

[Sunil V L]

ACPI is enabled by default. Add a switch to turn off
for testing and debug purposes.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/virt.c         | 35 +++++++++++++++++++++++++++++++++++
 include/hw/riscv/virt.h |  2 ++
 2 files changed, 37 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index fb68cf81e9..58d3765b6d 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -50,6 +50,7 @@
 #include "hw/pci-host/gpex.h"
 #include "hw/display/ramfb.h"
 #include "hw/acpi/aml-build.h"
+#include "qapi/qapi-visit-common.h"
 
 /*
  * The virt machine physical address space used by some of the devices
@@ -1517,6 +1518,9 @@ static void virt_machine_init(MachineState *machine)
 
 static void virt_machine_instance_init(Object *obj)
 {
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
+
+    s->acpi = ON_OFF_AUTO_AUTO;
 }
 
 static char *virt_get_aia_guests(Object *obj, Error **errp)
@@ -1591,6 +1595,32 @@ static void virt_set_aclint(Object *obj, bool value, Error **errp)
     s->have_aclint = value;
 }
 
+bool virt_is_acpi_enabled(RISCVVirtState *s)
+{
+    if (s->acpi == ON_OFF_AUTO_OFF) {
+        return false;
+    }
+    return true;
+}
+
+static void virt_get_acpi(Object *obj, Visitor *v, const char *name,
+                          void *opaque, Error **errp)
+{
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
+
+    OnOffAuto acpi = s->acpi;
+
+    visit_type_OnOffAuto(v, name, &acpi, errp);
+}
+
+static void virt_set_acpi(Object *obj, Visitor *v, const char *name,
+                          void *opaque, Error **errp)
+{
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
+
+    visit_type_OnOffAuto(v, name, &s->acpi, errp);
+}
+
 static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
                                                         DeviceState *dev)
 {
@@ -1662,6 +1692,11 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value "
                  "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS);
     object_class_property_set_description(oc, "aia-guests", str);
+    object_class_property_add(oc, "acpi", "OnOffAuto",
+                              virt_get_acpi, virt_set_acpi,
+                              NULL, NULL);
+    object_class_property_set_description(oc, "acpi",
+                                          "Enable ACPI");
 }
 
 static const TypeInfo virt_machine_typeinfo = {
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 6c7885bf89..62efebaa32 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -58,6 +58,7 @@ struct RISCVVirtState {
     int aia_guests;
     char *oem_id;
     char *oem_table_id;
+    OnOffAuto acpi;
 };
 
 enum {
@@ -123,4 +124,5 @@ enum {
 #define FDT_APLIC_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
                                  1 + FDT_APLIC_INT_CELLS)
 
+bool virt_is_acpi_enabled(RISCVVirtState *s);
 #endif
-- 
2.34.1

[PATCH V2 03/10] hw/riscv/virt: Add memmap pointer to RiscVVirtState

[Sunil V L]

memmap needs to be exported outside of virt.c so that
modules like acpi can use it. Hence, add a pointer field
in RiscVVirtState structure and initialize it with the
memorymap.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt.c         | 2 ++
 include/hw/riscv/virt.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 58d3765b6d..5017ba62ec 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1451,6 +1451,8 @@ static void virt_machine_init(MachineState *machine)
             ROUND_UP(virt_high_pcie_memmap.base, virt_high_pcie_memmap.size);
     }
 
+    s->memmap = virt_memmap;
+
     /* register system main memory (actual RAM) */
     memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base,
         machine->ram);
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 62efebaa32..379501edcc 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -59,6 +59,7 @@ struct RISCVVirtState {
     char *oem_id;
     char *oem_table_id;
     OnOffAuto acpi;
+    const MemMapEntry *memmap;
 };
 
 enum {
-- 
2.34.1

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

[Sunil V L]

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"
+#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
-- 
2.34.1

[PATCH V2 05/10] hw/riscv/virt: virt-acpi-build.c: Add RINTC in MADT

[Sunil V L]

Add Multiple APIC Description Table (MADT) with the
INTC structure for each cpu.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/virt-acpi-build.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index 3c4da6c385..f54e3fb731 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -134,6 +134,43 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
     free_aml_allocator();
 }
 
+/* MADT */
+static void
+build_madt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
+{
+    MachineState *mc = MACHINE(s);
+    int socket;
+    uint16_t base_hartid = 0;
+    uint32_t cpu_id = 0;
+
+    AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id,
+                        .oem_table_id = s->oem_table_id };
+
+    acpi_table_begin(&table, table_data);
+    /* Local Interrupt Controller Address */
+    build_append_int_noprefix(table_data, 0, 4);
+    build_append_int_noprefix(table_data, 0, 4);   /* MADT Flags */
+
+    /* RISC-V Local INTC structures per HART */
+    for (socket = 0; socket < riscv_socket_count(mc); socket++) {
+        base_hartid = riscv_socket_first_hartid(mc, socket);
+
+        for (int i = 0; i < s->soc[socket].num_harts; i++) {
+            build_append_int_noprefix(table_data, 0x18, 1);    /* Type     */
+            build_append_int_noprefix(table_data, 20, 1);      /* Length   */
+            build_append_int_noprefix(table_data, 1, 1);       /* Version  */
+            build_append_int_noprefix(table_data, 0, 1);       /* Reserved */
+            build_append_int_noprefix(table_data, 1, 4);       /* Flags    */
+            build_append_int_noprefix(table_data,
+                                      (base_hartid + i), 8);   /* hartid   */
+            build_append_int_noprefix(table_data, cpu_id, 4);  /* ACPI ID  */
+            cpu_id++;
+        }
+    }
+
+    acpi_table_end(linker, &table);
+}
+
 static void
 virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
 {
-- 
2.34.1

[PATCH V2 06/10] hw/riscv/virt: virt-acpi-build.c: Add RHCT Table

[Sunil V L]

RISC-V ACPI platforms need to provide RISC-V Hart Capabilities
Table (RHCT). Add this to the ACPI tables.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/virt-acpi-build.c | 62 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
index f54e3fb731..a2054f79a8 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -35,6 +35,7 @@
 #include "hw/pci-host/gpex.h"
 #include "qapi/error.h"
 #include "migration/vmstate.h"
+#include "hw/intc/riscv_aclint.h"
 
 #define ACPI_BUILD_TABLE_SIZE             0x20000
 
@@ -85,6 +86,67 @@ acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
     aml_append(scope, dev);
 }
 
+#define RHCT_NODE_ARRAY_OFFSET 56
+static void
+build_rhct(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
+{
+    MachineState *ms = MACHINE(s);
+    uint32_t acpi_proc_id = 0;
+    int i, socket;
+    RISCVCPU *cpu;
+    char *isa;
+    size_t len, aligned_len;
+    uint32_t isa_offset, num_rhct_nodes;
+
+    AcpiTable table = { .sig = "RHCT", .rev = 1, .oem_id = s->oem_id,
+                        .oem_table_id = s->oem_table_id };
+
+    acpi_table_begin(&table, table_data);
+
+    build_append_int_noprefix(table_data, 0x0, 4);   /* Reserved */
+    build_append_int_noprefix(table_data,
+                              RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, 8);
+
+    /* ISA + N hart info */
+    num_rhct_nodes = 1 + ms->smp.cpus;
+    build_append_int_noprefix(table_data, num_rhct_nodes, 4);
+    build_append_int_noprefix(table_data, RHCT_NODE_ARRAY_OFFSET, 4);
+
+    /* ISA string node */
+    isa_offset = table_data->len - table.table_offset;
+    build_append_int_noprefix(table_data, 0, 2);   /* Type 0*/
+
+    cpu = &s->soc[0].harts[0];
+    isa = riscv_isa_string(cpu);
+    len = 8 + strlen(isa) + 1;
+    aligned_len = (len % 2) ? (len + 1) : len;
+
+    build_append_int_noprefix(table_data, aligned_len, 2);   /* Total length */
+    build_append_int_noprefix(table_data, 0x1, 2);           /* Revision */
+
+    /* ISA string length including NUL */
+    build_append_int_noprefix(table_data, strlen(isa) + 1, 2);
+    g_array_append_vals(table_data, isa, strlen(isa) + 1);   /* ISA string */
+
+    if (aligned_len != len) {
+        build_append_int_noprefix(table_data, 0x0, 1);   /* pad */
+    }
+
+    for (socket = 0; socket < riscv_socket_count(ms); socket++) {
+        for (i = 0; i < s->soc[socket].num_harts; i++) {
+            build_append_int_noprefix(table_data, 0xFFFF, 2);  /* Type */
+            build_append_int_noprefix(table_data, 16, 2);      /* Length */
+            build_append_int_noprefix(table_data, 0x1, 2);     /* Revision */
+            build_append_int_noprefix(table_data, 1, 2); /* number of offsets */
+            build_append_int_noprefix(table_data, acpi_proc_id, 4); /* UID */
+            build_append_int_noprefix(table_data, isa_offset, 4);
+            acpi_proc_id++;
+        }
+    }
+
+    acpi_table_end(linker, &table);
+}
+
 /* FADT */
 static void
 build_fadt_rev6(GArray *table_data, BIOSLinker *linker,
-- 
2.34.1

[PATCH V2 07/10] hw/riscv: meson.build: Build virt-acpi-build.c

[Sunil V L]

ACPI functions are defined in new file virt-acpi-build.c. Enable
it to be built as part of virt machine if CONFIG_ACPI is set.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/meson.build | 1 +
 1 file changed, 1 insertion(+)

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}
-- 
2.34.1

[PATCH V2 08/10] hw/riscv/Kconfig: virt: Enable ACPI config option

[Sunil V L]

Enable ACPI related config option to build ACPI subsystem
for virt machine.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/Kconfig | 1 +
 1 file changed, 1 insertion(+)

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
-- 
2.34.1

[PATCH V2 09/10] hw/riscv/virt.c: Initialize the ACPI tables

[Sunil V L]

When the "acpi=on", ACPI tables need to be added. Detect the option
and initialize the ACPI tables.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
---
 hw/riscv/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 5017ba62ec..43c201c8cf 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1316,6 +1316,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
     if (kvm_enabled()) {
         riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
     }
+
+    if (virt_is_acpi_enabled(s)) {
+        virt_acpi_setup(s);
+    }
 }
 
 static void virt_machine_init(MachineState *machine)
-- 
2.34.1

[PATCH V2 10/10] MAINTAINERS: Add entry for RISC-V ACPI

[Sunil V L]

RISC-V ACPI related functionality for virt machine is added in
virt-acpi-build.c. Add the maintainer entry.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
Reviewed-by: Bin Meng <bmeng@tinylab.org>
Acked-by: Alistair Francis <alistair.francis@wdc.com>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 847bc7f131..7fb0f1052d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1003,6 +1003,12 @@ L: qemu-arm@nongnu.org
 S: Maintained
 F: hw/arm/virt-acpi-build.c
 
+RISC-V ACPI Subsystem
+M: Sunil V L <sunilvl@ventanamicro.com>
+L: qemu-riscv@nongnu.org
+S: Maintained
+F: hw/riscv/virt-acpi-build.c
+
 STM32F100
 M: Alexandre Iooss <erdnaxe@crans.org>
 L: qemu-arm@nongnu.org
-- 
2.34.1

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

[Daniel Henrique Barboza]

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

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

[Sunil V L]

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

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

[Bin Meng]

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

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

[Andrew Jones]

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

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

[Daniel Henrique Barboza]

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

Re: [PATCH V2 01/10] hw/riscv/virt: Add OEM_ID and OEM_TABLE_ID fields

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:29PM +0530, Sunil V L wrote:
> ACPI needs OEM_ID and OEM_TABLE_ID for the machine. Add these fields
> in the RISCVVirtState structure and initialize with default values.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt.c         | 4 ++++
>  include/hw/riscv/virt.h | 2 ++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 86c4adc0c9..fb68cf81e9 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -49,6 +49,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> +#include "hw/acpi/aml-build.h"
>  
>  /*
>   * The virt machine physical address space used by some of the devices
> @@ -1504,6 +1505,9 @@ static void virt_machine_init(MachineState *machine)
>      }
>      virt_flash_map(s, system_memory);
>  
> +    s->oem_id = g_strndup(ACPI_BUILD_APPNAME6, 6);
> +    s->oem_table_id = g_strndup(ACPI_BUILD_APPNAME8, 8);
> +

arm and x86 do this at instance init time, because then it can be
overridden in machine class init. loongarch also does this at instance
init time, probably because arm and x86 do. If riscv doesn't plan to
allow overriding, then it could be done in acpi setup. Otherwise, it
should probably follow the other architectures' example.

>      /* create device tree */
>      create_fdt(s, memmap);
>  
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index b3d26135c0..6c7885bf89 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -56,6 +56,8 @@ struct RISCVVirtState {
>      bool have_aclint;
>      RISCVVirtAIAType aia_type;
>      int aia_guests;
> +    char *oem_id;
> +    char *oem_table_id;
>  };

Otherwise

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH V2 02/10] hw/riscv/virt: Add a switch to enable/disable ACPI

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:30PM +0530, Sunil V L wrote:
> ACPI is enabled by default. Add a switch to turn off
> for testing and debug purposes.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt.c         | 35 +++++++++++++++++++++++++++++++++++
>  include/hw/riscv/virt.h |  2 ++
>  2 files changed, 37 insertions(+)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index fb68cf81e9..58d3765b6d 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -50,6 +50,7 @@
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
>  #include "hw/acpi/aml-build.h"
> +#include "qapi/qapi-visit-common.h"
>  
>  /*
>   * The virt machine physical address space used by some of the devices
> @@ -1517,6 +1518,9 @@ static void virt_machine_init(MachineState *machine)
>  
>  static void virt_machine_instance_init(Object *obj)
>  {
> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> +
> +    s->acpi = ON_OFF_AUTO_AUTO;

Might as well put the oem stuff here too now like the other architectures.

>  }
>  
>  static char *virt_get_aia_guests(Object *obj, Error **errp)
> @@ -1591,6 +1595,32 @@ static void virt_set_aclint(Object *obj, bool value, Error **errp)
>      s->have_aclint = value;
>  }
>  
> +bool virt_is_acpi_enabled(RISCVVirtState *s)
> +{
> +    if (s->acpi == ON_OFF_AUTO_OFF) {
> +        return false;
> +    }
> +    return true;

  return s->acpi != ON_OFF_AUTO_OFF;

> +}
> +
> +static void virt_get_acpi(Object *obj, Visitor *v, const char *name,
> +                          void *opaque, Error **errp)
> +{
> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> +

extra blank line here

> +    OnOffAuto acpi = s->acpi;
> +
> +    visit_type_OnOffAuto(v, name, &acpi, errp);
> +}
> +
> +static void virt_set_acpi(Object *obj, Visitor *v, const char *name,
> +                          void *opaque, Error **errp)
> +{
> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(obj);
> +
> +    visit_type_OnOffAuto(v, name, &s->acpi, errp);
> +}
> +
>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>                                                          DeviceState *dev)
>  {
> @@ -1662,6 +1692,11 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
>      sprintf(str, "Set number of guest MMIO pages for AIA IMSIC. Valid value "
>                   "should be between 0 and %d.", VIRT_IRQCHIP_MAX_GUESTS);
>      object_class_property_set_description(oc, "aia-guests", str);
> +    object_class_property_add(oc, "acpi", "OnOffAuto",
> +                              virt_get_acpi, virt_set_acpi,
> +                              NULL, NULL);
> +    object_class_property_set_description(oc, "acpi",
> +                                          "Enable ACPI");
>  }
>  
>  static const TypeInfo virt_machine_typeinfo = {
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 6c7885bf89..62efebaa32 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -58,6 +58,7 @@ struct RISCVVirtState {
>      int aia_guests;
>      char *oem_id;
>      char *oem_table_id;
> +    OnOffAuto acpi;
>  };
>  
>  enum {
> @@ -123,4 +124,5 @@ enum {
>  #define FDT_APLIC_INT_MAP_WIDTH (FDT_PCI_ADDR_CELLS + FDT_PCI_INT_CELLS + \
>                                   1 + FDT_APLIC_INT_CELLS)
>  
> +bool virt_is_acpi_enabled(RISCVVirtState *s);
>  #endif
> -- 
> 2.34.1
>

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH V2 03/10] hw/riscv/virt: Add memmap pointer to RiscVVirtState

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:31PM +0530, Sunil V L wrote:
> memmap needs to be exported outside of virt.c so that
> modules like acpi can use it. Hence, add a pointer field
> in RiscVVirtState structure and initialize it with the
> memorymap.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt.c         | 2 ++
>  include/hw/riscv/virt.h | 1 +
>  2 files changed, 3 insertions(+)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 58d3765b6d..5017ba62ec 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1451,6 +1451,8 @@ static void virt_machine_init(MachineState *machine)
>              ROUND_UP(virt_high_pcie_memmap.base, virt_high_pcie_memmap.size);
>      }
>  
> +    s->memmap = virt_memmap;
> +
>      /* register system main memory (actual RAM) */
>      memory_region_add_subregion(system_memory, memmap[VIRT_DRAM].base,
>          machine->ram);
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 62efebaa32..379501edcc 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -59,6 +59,7 @@ struct RISCVVirtState {
>      char *oem_id;
>      char *oem_table_id;
>      OnOffAuto acpi;
> +    const MemMapEntry *memmap;
>  };
>  
>  enum {
> -- 
> 2.34.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

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

[Andrew Jones]

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

Re: [PATCH V2 05/10] hw/riscv/virt: virt-acpi-build.c: Add RINTC in MADT

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:33PM +0530, Sunil V L wrote:
> Add Multiple APIC Description Table (MADT) with the
> INTC structure for each cpu.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/virt-acpi-build.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 3c4da6c385..f54e3fb731 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -134,6 +134,43 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
>      free_aml_allocator();
>  }
>  
> +/* MADT */
> +static void
> +build_madt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
> +{
> +    MachineState *mc = MACHINE(s);

Please use 'ms' for MachineState and 'mc' for MachineClass

> +    int socket;
> +    uint16_t base_hartid = 0;
> +    uint32_t cpu_id = 0;
> +
> +    AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = s->oem_id,
> +                        .oem_table_id = s->oem_table_id };
> +
> +    acpi_table_begin(&table, table_data);
> +    /* Local Interrupt Controller Address */
> +    build_append_int_noprefix(table_data, 0, 4);
> +    build_append_int_noprefix(table_data, 0, 4);   /* MADT Flags */
> +
> +    /* RISC-V Local INTC structures per HART */
> +    for (socket = 0; socket < riscv_socket_count(mc); socket++) {
> +        base_hartid = riscv_socket_first_hartid(mc, socket);
> +
> +        for (int i = 0; i < s->soc[socket].num_harts; i++) {
> +            build_append_int_noprefix(table_data, 0x18, 1);    /* Type     */
> +            build_append_int_noprefix(table_data, 20, 1);      /* Length   */
> +            build_append_int_noprefix(table_data, 1, 1);       /* Version  */
> +            build_append_int_noprefix(table_data, 0, 1);       /* Reserved */
> +            build_append_int_noprefix(table_data, 1, 4);       /* Flags    */
> +            build_append_int_noprefix(table_data,
> +                                      (base_hartid + i), 8);   /* hartid   */

The spec calls this field "Hart ID of the hart" (which is redundant), but
we should at least use "Hart ID" for the comment here. We want the text
in the comments to be directly searchable in the specs.

> +            build_append_int_noprefix(table_data, cpu_id, 4);  /* ACPI ID  */

This one should be "ACPI Processor UID"

> +            cpu_id++;
> +        }
> +    }
> +
> +    acpi_table_end(linker, &table);
> +}
> +
>  static void
>  virt_acpi_build(RISCVVirtState *s, AcpiBuildTables *tables)
>  {
> -- 
> 2.34.1
>

Thanks,
drew

Re: [PATCH V2 06/10] hw/riscv/virt: virt-acpi-build.c: Add RHCT Table

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:34PM +0530, Sunil V L wrote:
> RISC-V ACPI platforms need to provide RISC-V Hart Capabilities
> Table (RHCT). Add this to the ACPI tables.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt-acpi-build.c | 62 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index f54e3fb731..a2054f79a8 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -35,6 +35,7 @@
>  #include "hw/pci-host/gpex.h"
>  #include "qapi/error.h"
>  #include "migration/vmstate.h"
> +#include "hw/intc/riscv_aclint.h"
>  
>  #define ACPI_BUILD_TABLE_SIZE             0x20000
>  
> @@ -85,6 +86,67 @@ acpi_dsdt_add_fw_cfg(Aml *scope, const MemMapEntry *fw_cfg_memmap)
>      aml_append(scope, dev);
>  }
>  
> +#define RHCT_NODE_ARRAY_OFFSET 56
> +static void
> +build_rhct(GArray *table_data, BIOSLinker *linker, RISCVVirtState *s)
> +{
> +    MachineState *ms = MACHINE(s);
> +    uint32_t acpi_proc_id = 0;
> +    int i, socket;
> +    RISCVCPU *cpu;
> +    char *isa;
> +    size_t len, aligned_len;
> +    uint32_t isa_offset, num_rhct_nodes;
> +
> +    AcpiTable table = { .sig = "RHCT", .rev = 1, .oem_id = s->oem_id,
> +                        .oem_table_id = s->oem_table_id };
> +
> +    acpi_table_begin(&table, table_data);
> +
> +    build_append_int_noprefix(table_data, 0x0, 4);   /* Reserved */
> +    build_append_int_noprefix(table_data,
> +                              RISCV_ACLINT_DEFAULT_TIMEBASE_FREQ, 8);

Need "Time Base Frequency" comment.

> +
> +    /* ISA + N hart info */
> +    num_rhct_nodes = 1 + ms->smp.cpus;
> +    build_append_int_noprefix(table_data, num_rhct_nodes, 4);

/* Number of RHCT nodes */

> +    build_append_int_noprefix(table_data, RHCT_NODE_ARRAY_OFFSET, 4);

/* Offset to the RHCT node array */

> +
> +    /* ISA string node */
> +    isa_offset = table_data->len - table.table_offset;
> +    build_append_int_noprefix(table_data, 0, 2);   /* Type 0*/
                                                               ^ need
							       space
> +
> +    cpu = &s->soc[0].harts[0];
> +    isa = riscv_isa_string(cpu);
> +    len = 8 + strlen(isa) + 1;
> +    aligned_len = (len % 2) ? (len + 1) : len;
> +
> +    build_append_int_noprefix(table_data, aligned_len, 2);   /* Total length */

s/Total length/Length/

> +    build_append_int_noprefix(table_data, 0x1, 2);           /* Revision */
> +
> +    /* ISA string length including NUL */

/* ISA Length including NUL */

> +    build_append_int_noprefix(table_data, strlen(isa) + 1, 2);
> +    g_array_append_vals(table_data, isa, strlen(isa) + 1);   /* ISA string */
> +
> +    if (aligned_len != len) {
> +        build_append_int_noprefix(table_data, 0x0, 1);   /* pad */

s/pad/Optional Padding/

> +    }
> +
> +    for (socket = 0; socket < riscv_socket_count(ms); socket++) {
> +        for (i = 0; i < s->soc[socket].num_harts; i++) {
> +            build_append_int_noprefix(table_data, 0xFFFF, 2);  /* Type */
> +            build_append_int_noprefix(table_data, 16, 2);      /* Length */
> +            build_append_int_noprefix(table_data, 0x1, 2);     /* Revision */
> +            build_append_int_noprefix(table_data, 1, 2); /* number of offsets */

s/number/Number/

> +            build_append_int_noprefix(table_data, acpi_proc_id, 4); /* UID */

ACPI Processor UID

> +            build_append_int_noprefix(table_data, isa_offset, 4);

/* Offsets */

> +            acpi_proc_id++;
> +        }
> +    }
> +
> +    acpi_table_end(linker, &table);
> +}
> +
>  /* FADT */
>  static void
>  build_fadt_rev6(GArray *table_data, BIOSLinker *linker,
> -- 
> 2.34.1
>

Other than getting the comments to match the spec fields,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew

Re: [PATCH V2 07/10] hw/riscv: meson.build: Build virt-acpi-build.c

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:35PM +0530, Sunil V L wrote:
> ACPI functions are defined in new file virt-acpi-build.c. Enable
> it to be built as part of virt machine if CONFIG_ACPI is set.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  hw/riscv/meson.build | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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}
> -- 
> 2.34.1
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH V2 08/10] hw/riscv/Kconfig: virt: Enable ACPI config option

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:36PM +0530, Sunil V L wrote:
> Enable ACPI related config option to build ACPI subsystem
> for virt machine.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> 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
> -- 
> 2.34.1
>

This could probably squashed together with the previous patch.

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH V2 09/10] hw/riscv/virt.c: Initialize the ACPI tables

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:37PM +0530, Sunil V L wrote:
> When the "acpi=on", ACPI tables need to be added. Detect the option
> and initialize the ACPI tables.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> ---
>  hw/riscv/virt.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 5017ba62ec..43c201c8cf 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1316,6 +1316,10 @@ static void virt_machine_done(Notifier *notifier, void *data)
>      if (kvm_enabled()) {
>          riscv_setup_direct_kernel(kernel_entry, fdt_load_addr);
>      }
> +
> +    if (virt_is_acpi_enabled(s)) {
> +        virt_acpi_setup(s);
> +    }
>  }
>  
>  static void virt_machine_init(MachineState *machine)
> -- 
> 2.34.1
> 

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH V2 10/10] MAINTAINERS: Add entry for RISC-V ACPI

[Andrew Jones]

On Mon, Feb 13, 2023 at 08:10:38PM +0530, Sunil V L wrote:
> RISC-V ACPI related functionality for virt machine is added in
> virt-acpi-build.c. Add the maintainer entry.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> Reviewed-by: Bin Meng <bmeng@tinylab.org>
> Acked-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 847bc7f131..7fb0f1052d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1003,6 +1003,12 @@ L: qemu-arm@nongnu.org
>  S: Maintained
>  F: hw/arm/virt-acpi-build.c
>  
> +RISC-V ACPI Subsystem
> +M: Sunil V L <sunilvl@ventanamicro.com>
> +L: qemu-riscv@nongnu.org
> +S: Maintained
> +F: hw/riscv/virt-acpi-build.c
> +
>  STM32F100
>  M: Alexandre Iooss <erdnaxe@crans.org>
>  L: qemu-arm@nongnu.org
> -- 
> 2.34.1
>

Please move the ARM ACPI entry down under the main ACPI entry and then
add the RISC-V one there too.

Otherwise,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH V2 08/10] hw/riscv/Kconfig: virt: Enable ACPI config option

[Sunil V L]

On Wed, Feb 15, 2023 at 02:24:24PM +0100, Andrew Jones wrote:
> On Mon, Feb 13, 2023 at 08:10:36PM +0530, Sunil V L wrote:
> > Enable ACPI related config option to build ACPI subsystem
> > for virt machine.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/riscv/Kconfig | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > 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
> > -- 
> > 2.34.1
> >
> 
> This could probably squashed together with the previous patch.
> 
Thank you!, Drew. Yes, I am going to squash these patches as per
feedback from Daniel. Will address your comments on other patches and
send V3 soon.

Thanks!
Sunil

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

[Sunil V L]

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

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

[Palmer Dabbelt]

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

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

[Sunil V L]

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