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

[PATCH 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 infrstructure 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

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
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,acpi=on -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 options
  hw/riscv/virt.c: Initialize the ACPI tables
  MAINTAINERS: Add entry for RISC-V ACPI

 MAINTAINERS                |   6 +
 hw/riscv/Kconfig           |   3 +
 hw/riscv/meson.build       |   1 +
 hw/riscv/virt-acpi-build.c | 391 +++++++++++++++++++++++++++++++++++++
 hw/riscv/virt.c            |  48 +++++
 include/hw/riscv/virt.h    |   6 +
 6 files changed, 455 insertions(+)
 create mode 100644 hw/riscv/virt-acpi-build.c

-- 
2.38.0

[PATCH 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>
---
 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 a061151a6f..7ad9fda20c 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
@@ -1512,6 +1513,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.38.0

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

[Sunil V L]

ACPI is optional. So, add a switch to toggle.

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

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 7ad9fda20c..84962962ff 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
@@ -1525,6 +1526,10 @@ static void virt_machine_init(MachineState *machine)
 
 static void virt_machine_instance_init(Object *obj)
 {
+    MachineState *ms = MACHINE(obj);
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
+
+    s->acpi = ON_OFF_AUTO_OFF;
 }
 
 static char *virt_get_aia_guests(Object *obj, Error **errp)
@@ -1601,6 +1606,34 @@ 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)
+{
+    MachineState *ms = MACHINE(obj);
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
+
+    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)
+{
+    MachineState *ms = MACHINE(obj);
+    RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
+
+    visit_type_OnOffAuto(v, name, &s->acpi, errp);
+}
+
 static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
                                                         DeviceState *dev)
 {
@@ -1672,6 +1705,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.38.0

[PATCH 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>
---
 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 84962962ff..26caea59ff 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1459,6 +1459,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.38.0

[PATCH 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 | 292 +++++++++++++++++++++++++++++++++++++
 include/hw/riscv/virt.h    |   1 +
 2 files changed, 293 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..0410b955bd
--- /dev/null
+++ b/hw/riscv/virt-acpi-build.c
@@ -0,0 +1,292 @@
+/*
+ * 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 *vms)
+{
+    MachineState *ms = MACHINE(vms);
+    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 *vms, 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, vms->oem_id, vms->oem_table_id);
+}
+
+/* DSDT */
+static void
+build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
+{
+    Aml *scope, *dsdt;
+    const MemMapEntry *memmap = vms->memmap;
+    AcpiTable table = { .sig = "DSDT", .rev = 2, .oem_id = vms->oem_id,
+                        .oem_table_id = vms->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, vms);
+
+    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 *vms, 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, vms);
+
+    /* FADT and others pointed to by RSDT */
+    acpi_add_table(table_offsets, tables_blob);
+    build_fadt_rev6(tables_blob, tables->linker, vms, dsdt);
+
+    acpi_add_table(table_offsets, tables_blob);
+    build_madt(tables_blob, tables->linker, vms);
+
+    acpi_add_table(table_offsets, tables_blob);
+    build_rhct(tables_blob, tables->linker, vms);
+
+    /* XSDT is pointed to by RSDP */
+    xsdt = tables_blob->len;
+    build_xsdt(tables_blob, tables->linker, table_offsets, vms->oem_id,
+                vms->oem_table_id);
+
+    /* RSDP is in FSEG memory, so allocate it separately */
+    {
+        AcpiRsdpData rsdp_data = {
+            .revision = 2,
+            .oem_id = vms->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);
+
+
+    /* Cleanup 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 *vms)
+{
+    AcpiBuildTables tables;
+    AcpiBuildState *build_state;
+
+    build_state = g_malloc0(sizeof *build_state);
+
+    acpi_build_tables_init(&tables);
+    virt_acpi_build(vms, &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);
+
+    /*
+     * Cleanup tables but don't free the memory: we track it
+     * in build_state.
+     */
+    acpi_build_tables_cleanup(&tables, false);
+}
diff --git a/include/hw/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.38.0

[PATCH 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>
---
 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 0410b955bd..2f65f1e2e5 100644
--- a/hw/riscv/virt-acpi-build.c
+++ b/hw/riscv/virt-acpi-build.c
@@ -137,6 +137,43 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
     free_aml_allocator();
 }
 
+/* MADT */
+static void
+build_madt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
+{
+    MachineState *mc = MACHINE(vms);
+    int socket;
+    uint16_t base_hartid = 0;
+    uint32_t cpu_id = 0;
+
+    AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = vms->oem_id,
+                        .oem_table_id = vms->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 < vms->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 *vms, AcpiBuildTables *tables)
 {
-- 
2.38.0

[PATCH 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 2f65f1e2e5..b04f179cb5 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
 
@@ -88,6 +89,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 *vms)
+{
+    MachineState *ms = MACHINE(vms);
+    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 = vms->oem_id,
+                        .oem_table_id = vms->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 = &vms->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 < vms->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.38.0

[PATCH 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>
---
 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.38.0

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

[Sunil V L]

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

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

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index 4550b3b938..92b1a9eb64 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -44,6 +44,9 @@ config RISCV_VIRT
     select VIRTIO_MMIO
     select FW_CFG_DMA
     select PLATFORM_BUS
+    select ACPI
+    select ACPI_HW_REDUCED
+    select ACPI_PCI
 
 config SHAKTI_C
     bool
-- 
2.38.0

[PATCH 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>
---
 hw/riscv/virt.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 26caea59ff..f1f5612c69 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -1324,6 +1324,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.38.0

[PATCH 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>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c581c11a64..23fcaaf54a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -995,6 +995,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.38.0

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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>
> ---
>  hw/riscv/virt.c         | 4 ++++
>  include/hw/riscv/virt.h | 2 ++
>  2 files changed, 6 insertions(+)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> ACPI is optional. So, add a switch to toggle.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt.c         | 38 ++++++++++++++++++++++++++++++++++++++
>  include/hw/riscv/virt.h |  2 ++
>  2 files changed, 40 insertions(+)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 7ad9fda20c..84962962ff 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
> @@ -1525,6 +1526,10 @@ static void virt_machine_init(MachineState *machine)
>
>  static void virt_machine_instance_init(Object *obj)
>  {
> +    MachineState *ms = MACHINE(obj);

Drop this

> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
> +
> +    s->acpi = ON_OFF_AUTO_OFF;

Is this needed? I believe the purpose of an auto/on/off property is to
have an "auto" value, which is ON_OFF_AUTO_AUTO.

>  }
>
>  static char *virt_get_aia_guests(Object *obj, Error **errp)
> @@ -1601,6 +1606,34 @@ 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)
> +{
> +    MachineState *ms = MACHINE(obj);

ditto

> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
> +
> +    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)
> +{
> +    MachineState *ms = MACHINE(obj);

ditto

> +    RISCVVirtState *s = RISCV_VIRT_MACHINE(ms);
> +
> +    visit_type_OnOffAuto(v, name, &s->acpi, errp);
> +}
> +
>  static HotplugHandler *virt_machine_get_hotplug_handler(MachineState *machine,
>                                                          DeviceState *dev)
>  {
> @@ -1672,6 +1705,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);

I am not sure about "OnOffAuto" vs. "bool" type. It seems the only
difference is that with "OnOffAuto" type we may silently change the
interpretation of "auto" value across different QEMU versions?

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

Regards,
Bin

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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>
> ---
>  hw/riscv/virt.c         | 2 ++
>  include/hw/riscv/virt.h | 1 +
>  2 files changed, 3 insertions(+)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.

There are lots of same ACPI codes existing in x86/arm/riscv. I believe
some refactoring work is needed before ACPI support fully lands on
RISC-V.
For example, we can extract the common part among x86/arm/riscv into a
separate file, like hw/acpi/acpi-build.c?

>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt-acpi-build.c | 292 +++++++++++++++++++++++++++++++++++++
>  include/hw/riscv/virt.h    |   1 +
>  2 files changed, 293 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..0410b955bd
> --- /dev/null
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -0,0 +1,292 @@
> +/*
> + * 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). */

nits: removing the ending .

> +    MemoryRegion *table_mr;
> +    MemoryRegion *rsdp_mr;
> +    MemoryRegion *linker_mr;
> +    /* Is table patched? */
> +    bool patched;
> +} AcpiBuildState;
> +
> +static void

nits: please put above in the same line

> +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 *vms)

QEMU convention is to use 's' for the model state, so:

s/vms/s/g

> +{
> +    MachineState *ms = MACHINE(vms);
> +    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 *vms, 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, vms->oem_id, vms->oem_table_id);
> +}
> +
> +/* DSDT */
> +static void
> +build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
> +{
> +    Aml *scope, *dsdt;
> +    const MemMapEntry *memmap = vms->memmap;
> +    AcpiTable table = { .sig = "DSDT", .rev = 2, .oem_id = vms->oem_id,
> +                        .oem_table_id = vms->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, vms);
> +
> +    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 *vms, 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, vms);
> +
> +    /* FADT and others pointed to by RSDT */
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_fadt_rev6(tables_blob, tables->linker, vms, dsdt);
> +
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_madt(tables_blob, tables->linker, vms);
> +
> +    acpi_add_table(table_offsets, tables_blob);
> +    build_rhct(tables_blob, tables->linker, vms);
> +
> +    /* XSDT is pointed to by RSDP */
> +    xsdt = tables_blob->len;
> +    build_xsdt(tables_blob, tables->linker, table_offsets, vms->oem_id,
> +                vms->oem_table_id);
> +
> +    /* RSDP is in FSEG memory, so allocate it separately */
> +    {
> +        AcpiRsdpData rsdp_data = {
> +            .revision = 2,
> +            .oem_id = vms->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);
> +
> +
> +    /* Cleanup memory that's no longer used. */

Clean up

nits: removing the ending .

> +    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 *vms)
> +{
> +    AcpiBuildTables tables;
> +    AcpiBuildState *build_state;
> +
> +    build_state = g_malloc0(sizeof *build_state);
> +
> +    acpi_build_tables_init(&tables);
> +    virt_acpi_build(vms, &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);
> +
> +    /*
> +     * Cleanup tables but don't free the memory: we track it

s/Cleanup/Clean up

> +     * 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
> --

Regards,
Bin

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:53 PM Sunil V L <sunilvl@ventanamicro.com> 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>
> ---
>  hw/riscv/meson.build | 1 +
>  1 file changed, 1 insertion(+)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> Enable ACPI related config options to build ACPI subsystem
> for virt machine.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/Kconfig | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index 4550b3b938..92b1a9eb64 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -44,6 +44,9 @@ config RISCV_VIRT
>      select VIRTIO_MMIO
>      select FW_CFG_DMA
>      select PLATFORM_BUS
> +    select ACPI
> +    select ACPI_HW_REDUCED

I don't see APIs in ACPI_HW_REDUCED get called by RISC-V virt codes.

> +    select ACPI_PCI

Regards,
Bin

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:53 PM Sunil V L <sunilvl@ventanamicro.com> 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>
> ---
>  hw/riscv/virt.c | 4 ++++
>  1 file changed, 4 insertions(+)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

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

[Bin Meng]

On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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>
> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
>

Reviewed-by: Bin Meng <bmeng@tinylab.org>

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

[Andrea Bolognani]

On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> +    object_class_property_add(oc, "acpi", "OnOffAuto",
> +                              virt_get_acpi, virt_set_acpi,
> +                              NULL, NULL);
> +    object_class_property_set_description(oc, "acpi",
> +                                          "Enable ACPI");

The way this works on other architectures (x86_64, aarch64) is that
you get ACPI by default and can use -no-acpi to disable it if
desired. Can we have the same on RISC-V, for consistency?

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Philippe Mathieu-Daudé]

On 6/2/23 11:54, Andrea Bolognani wrote:
> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
>> +    object_class_property_add(oc, "acpi", "OnOffAuto",
>> +                              virt_get_acpi, virt_set_acpi,
>> +                              NULL, NULL);
>> +    object_class_property_set_description(oc, "acpi",
>> +                                          "Enable ACPI");
> 
> The way this works on other architectures (x86_64, aarch64) is that
> you get ACPI by default and can use -no-acpi to disable it if
> desired. Can we have the same on RISC-V, for consistency?

-no-acpi rather seems a x86-specific hack for the ISA PC machine, and
has a high maintenance cost / burden.

If hardware provides ACPI support, QEMU should expose it to the guest.

Actually, what is the value added by '-no-acpi'?

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

[Andrew Jones]

On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 11:54, Andrea Bolognani wrote:
> > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > +                              virt_get_acpi, virt_set_acpi,
> > > +                              NULL, NULL);
> > > +    object_class_property_set_description(oc, "acpi",
> > > +                                          "Enable ACPI");
> > 
> > The way this works on other architectures (x86_64, aarch64) is that
> > you get ACPI by default and can use -no-acpi to disable it if
> > desired. Can we have the same on RISC-V, for consistency?

Default on, with a user control to turn off, can be done with a boolean.
I'm not sure why/if Auto is needed for acpi. Auto is useful when a
configuration doesn't support a default setting for a feature. If the
user hasn't explicitly requested the feature to be on or off, then the
configuration can silently select what works. If, however, the user
explicitly chooses what doesn't work, then qemu will fail with an error
instead.

> 
> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> has a high maintenance cost / burden.
> 
> If hardware provides ACPI support, QEMU should expose it to the guest.
> 
> Actually, what is the value added by '-no-acpi'?

IIRC, when booting, at least arm guests, with edk2 and ACPI tables,
then edk2 will provide the guest ACPI tables instead of DT. To ensure
we can boot with edk2, but still allow the guest to boot with DT, we
need a way to disable the generation of ACPI tables.

Thanks,
drew

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

[Gerd Hoffmann]

On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 11:54, Andrea Bolognani wrote:
> > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > +                              virt_get_acpi, virt_set_acpi,
> > > +                              NULL, NULL);
> > > +    object_class_property_set_description(oc, "acpi",
> > > +                                          "Enable ACPI");
> > 
> > The way this works on other architectures (x86_64, aarch64) is that
> > you get ACPI by default and can use -no-acpi to disable it if
> > desired. Can we have the same on RISC-V, for consistency?
> 
> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> has a high maintenance cost / burden.

Under the hood it is actually a OnOffAuto machine property and -no-acpi
is just a shortcut to set that.

> Actually, what is the value added by '-no-acpi'?

On arm(64) the linux kernel can use either device trees or ACPI to find
the hardware.  Historical reasons mostly, when the platform started
there was no ACPI support.  Also the edk2 firmware uses Device Trees
for hardware discovery, likewise for historical reasons.

When ACPI is available for a platform right from the start I see little
reason to offer an option to turn it off though ...

take care,
  Gerd

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

[Sunil V L]

On Mon, Feb 06, 2023 at 01:35:20PM +0100, Andrew Jones wrote:
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > +                              virt_get_acpi, virt_set_acpi,
> > > > +                              NULL, NULL);
> > > > +    object_class_property_set_description(oc, "acpi",
> > > > +                                          "Enable ACPI");
> > > 
> > > The way this works on other architectures (x86_64, aarch64) is that
> > > you get ACPI by default and can use -no-acpi to disable it if
> > > desired. Can we have the same on RISC-V, for consistency?
> 
> Default on, with a user control to turn off, can be done with a boolean.
> I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> configuration doesn't support a default setting for a feature. If the
> user hasn't explicitly requested the feature to be on or off, then the
> configuration can silently select what works. If, however, the user
> explicitly chooses what doesn't work, then qemu will fail with an error
> instead.
> 

Since all other architectures use Auto instead of a simple bool, I opted
for the same to keep it consistent.

However, default AUTO looked ambiguous to me. Since we still need to
support external interrupt controllers (IMSIC/APLIC/PLIC), I chose to
keep it OFF by default for now.

Thanks
Sunil

> > 
> > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > has a high maintenance cost / burden.
> > 
> > If hardware provides ACPI support, QEMU should expose it to the guest.
> > 
> > Actually, what is the value added by '-no-acpi'?
> 
> IIRC, when booting, at least arm guests, with edk2 and ACPI tables,
> then edk2 will provide the guest ACPI tables instead of DT. To ensure
> we can boot with edk2, but still allow the guest to boot with DT, we
> need a way to disable the generation of ACPI tables.
> 
> Thanks,
> drew

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

[Sunil V L]

On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.
> 
> There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> some refactoring work is needed before ACPI support fully lands on
> RISC-V.
> For example, we can extract the common part among x86/arm/riscv into a
> separate file, like hw/acpi/acpi-build.c?
> 

While it appears like there is same code in multiple places, those
functions take architecture specific MachineState parameter. Some tables
like MADT though with same name, will have different contents for
different architectures.

Only one function which Daniel also pointed is the wrapper
acpi_align_size() which can be made common. I am not
sure whether it is worth of refactoring.

> >
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/riscv/virt-acpi-build.c | 292 +++++++++++++++++++++++++++++++++++++
> >  include/hw/riscv/virt.h    |   1 +
> >  2 files changed, 293 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..0410b955bd
> > --- /dev/null
> > +++ b/hw/riscv/virt-acpi-build.c
> > @@ -0,0 +1,292 @@
> > +/*
> > + * 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). */
> 
> nits: removing the ending .
> 
> > +    MemoryRegion *table_mr;
> > +    MemoryRegion *rsdp_mr;
> > +    MemoryRegion *linker_mr;
> > +    /* Is table patched? */
> > +    bool patched;
> > +} AcpiBuildState;
> > +
> > +static void
> 
> nits: please put above in the same line
> 
> > +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 *vms)
> 
> QEMU convention is to use 's' for the model state, so:
> 
> s/vms/s/g
> 
Sure, Thanks!. Will update it when I send the next revision.

> > +{
> > +    MachineState *ms = MACHINE(vms);
> > +    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 *vms, 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, vms->oem_id, vms->oem_table_id);
> > +}
> > +
> > +/* DSDT */
> > +static void
> > +build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
> > +{
> > +    Aml *scope, *dsdt;
> > +    const MemMapEntry *memmap = vms->memmap;
> > +    AcpiTable table = { .sig = "DSDT", .rev = 2, .oem_id = vms->oem_id,
> > +                        .oem_table_id = vms->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, vms);
> > +
> > +    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 *vms, 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, vms);
> > +
> > +    /* FADT and others pointed to by RSDT */
> > +    acpi_add_table(table_offsets, tables_blob);
> > +    build_fadt_rev6(tables_blob, tables->linker, vms, dsdt);
> > +
> > +    acpi_add_table(table_offsets, tables_blob);
> > +    build_madt(tables_blob, tables->linker, vms);
> > +
> > +    acpi_add_table(table_offsets, tables_blob);
> > +    build_rhct(tables_blob, tables->linker, vms);
> > +
> > +    /* XSDT is pointed to by RSDP */
> > +    xsdt = tables_blob->len;
> > +    build_xsdt(tables_blob, tables->linker, table_offsets, vms->oem_id,
> > +                vms->oem_table_id);
> > +
> > +    /* RSDP is in FSEG memory, so allocate it separately */
> > +    {
> > +        AcpiRsdpData rsdp_data = {
> > +            .revision = 2,
> > +            .oem_id = vms->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);
> > +
> > +
> > +    /* Cleanup memory that's no longer used. */
> 
> Clean up
> 
> nits: removing the ending .
> 
Okay.

> > +    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 *vms)
> > +{
> > +    AcpiBuildTables tables;
> > +    AcpiBuildState *build_state;
> > +
> > +    build_state = g_malloc0(sizeof *build_state);
> > +
> > +    acpi_build_tables_init(&tables);
> > +    virt_acpi_build(vms, &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);
> > +
> > +    /*
> > +     * Cleanup tables but don't free the memory: we track it
> 
> s/Cleanup/Clean up

Okay.

Thanks!
Sunil
> 
> > +     * 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
> > --
> 
> Regards,
> Bin

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

[Sunil V L]

On Mon, Feb 06, 2023 at 06:29:46PM +0800, Bin Meng wrote:
> On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > Enable ACPI related config options to build ACPI subsystem
> > for virt machine.
> >
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/riscv/Kconfig | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > index 4550b3b938..92b1a9eb64 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -44,6 +44,9 @@ config RISCV_VIRT
> >      select VIRTIO_MMIO
> >      select FW_CFG_DMA
> >      select PLATFORM_BUS
> > +    select ACPI
> > +    select ACPI_HW_REDUCED
> 
> I don't see APIs in ACPI_HW_REDUCED get called by RISC-V virt codes.
> 
Yes, this and PCI are not required at this time. Will remove them when I
send the next revision of this series.

Thanks
Sunil
> > +    select ACPI_PCI
> 
> Regards,
> Bin

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

[Bernhard Beschow]

Am 6. Februar 2023 11:18:06 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 6/2/23 11:54, Andrea Bolognani wrote:
>> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
>>> +    object_class_property_add(oc, "acpi", "OnOffAuto",
>>> +                              virt_get_acpi, virt_set_acpi,
>>> +                              NULL, NULL);
>>> +    object_class_property_set_description(oc, "acpi",
>>> +                                          "Enable ACPI");
>> 
>> The way this works on other architectures (x86_64, aarch64) is that
>> you get ACPI by default and can use -no-acpi to disable it if
>> desired. Can we have the same on RISC-V, for consistency?
>
>-no-acpi rather seems a x86-specific hack for the ISA PC machine,

... for the i440FX/PIIX machine, to be precise. There it controls the presence of the PIIX ACPI controller and surely also the generation of ACPI tables. ACPI wasn't a thing in pure ISA times, so the switch doesn't make much sense for the ISA machine.

Here, for RISC-V, the "acpi" switch seems to just control the generation of ACPI tables which has quite different semantics than -no-acpi.

>and
>has a high maintenance cost / burden.
>
>If hardware provides ACPI support, QEMU should expose it to the guest.

Yes, I fully agree with both points.

>
>Actually, what is the value added by '-no-acpi'?

IIUC, it allows the PC machine to emulate a PCI PC without an ACPI bios. Unfortunately, it also omits the instantiation of the... erm... Frankenstein PIIX4_ACPI device which seems quite unnecessary to achieve that goal. Just always instantiating it seems much simpler.

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

[Bin Meng]

On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > +                              virt_get_acpi, virt_set_acpi,
> > > > +                              NULL, NULL);
> > > > +    object_class_property_set_description(oc, "acpi",
> > > > +                                          "Enable ACPI");
> > >
> > > The way this works on other architectures (x86_64, aarch64) is that
> > > you get ACPI by default and can use -no-acpi to disable it if
> > > desired. Can we have the same on RISC-V, for consistency?
>
> Default on, with a user control to turn off, can be done with a boolean.
> I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> configuration doesn't support a default setting for a feature. If the
> user hasn't explicitly requested the feature to be on or off, then the
> configuration can silently select what works. If, however, the user
> explicitly chooses what doesn't work, then qemu will fail with an error
> instead.

I have a confusion about "OnOffAuto" vs. "bool" type.

Both "OnOffAuto" vs. "bool" type property can have a default value if
user does not assign a value to it from command line. The default
value is:

- ON_OFF_AUTO_AUTO for "OnOffAuto"
- false for "bool"

But the default value can be overridden in the machine's init
function, like in this patch.

So I am not really sure how these 2 types of properties are different.
Why did we introduce a "OnOffAuto" type, and how is that type supposed
to be used in which scenario?

>
> >
> > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > has a high maintenance cost / burden.
> >
> > If hardware provides ACPI support, QEMU should expose it to the guest.
> >
> > Actually, what is the value added by '-no-acpi'?
>
> IIRC, when booting, at least arm guests, with edk2 and ACPI tables,
> then edk2 will provide the guest ACPI tables instead of DT. To ensure
> we can boot with edk2, but still allow the guest to boot with DT, we
> need a way to disable the generation of ACPI tables.
>

Regards,
Bin

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

[Andrew Jones]

On Tue, Feb 07, 2023 at 11:57:29AM +0800, Bin Meng wrote:
> On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +                              virt_get_acpi, virt_set_acpi,
> > > > > +                              NULL, NULL);
> > > > > +    object_class_property_set_description(oc, "acpi",
> > > > > +                                          "Enable ACPI");
> > > >
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> >
> > Default on, with a user control to turn off, can be done with a boolean.
> > I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> > configuration doesn't support a default setting for a feature. If the
> > user hasn't explicitly requested the feature to be on or off, then the
> > configuration can silently select what works. If, however, the user
> > explicitly chooses what doesn't work, then qemu will fail with an error
> > instead.
> 
> I have a confusion about "OnOffAuto" vs. "bool" type.
> 
> Both "OnOffAuto" vs. "bool" type property can have a default value if
> user does not assign a value to it from command line. The default
> value is:
> 
> - ON_OFF_AUTO_AUTO for "OnOffAuto"
> - false for "bool"
> 
> But the default value can be overridden in the machine's init
> function, like in this patch.
> 
> So I am not really sure how these 2 types of properties are different.
> Why did we introduce a "OnOffAuto" type, and how is that type supposed
> to be used in which scenario?

Auto makes sense for options which have dependencies on other options.
For example, if we have two options, A and B, where A is an Auto and
B is a boolean, then, when A is initialized to AUTO and has a dependency
on B being X, then have the following

	B=X	A=AUTO -> T	(works)
	B=!X	A=AUTO -> F	(works)

(This is the same whether B was set to X by default or explicitly by the
user.)

Now, if the user explicitly sets A, we have

	B=X	A=T		(works)
	B=X	A=F		(works)
	B=!X	A=T		(emit error about B!=X with A=T and fail)
	B=!X	A=F		(works)

We can't have that behavior with A just being a boolean, defaulting to
true, because we don't know if it's true because of the default or
because it was explicitly set

	B=X	A=T		(works, by default or by user)
	B=X	A=F		(works)
	B=!X	A=T		(doesn't work, but if the user didn't
				 select A=T, then we could have silently
				 flipped it to F and continued)
	B=!X	A=F		(works)


IOW, Auto just adds one more bit of info, allowing default vs. user
selection to be determined, which can then be used for different
behaviors.

Back to the "acpi" property, I'm not sure what it depends on that
requires it to be an Auto vs. a boolean.

Thanks,
drew

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

[Philippe Mathieu-Daudé]

On 6/2/23 13:56, Gerd Hoffmann wrote:
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
>> On 6/2/23 11:54, Andrea Bolognani wrote:
>>> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
>>>> +    object_class_property_add(oc, "acpi", "OnOffAuto",
>>>> +                              virt_get_acpi, virt_set_acpi,
>>>> +                              NULL, NULL);
>>>> +    object_class_property_set_description(oc, "acpi",
>>>> +                                          "Enable ACPI");
>>>
>>> The way this works on other architectures (x86_64, aarch64) is that
>>> you get ACPI by default and can use -no-acpi to disable it if
>>> desired. Can we have the same on RISC-V, for consistency?
>>
>> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
>> has a high maintenance cost / burden.
> 
> Under the hood it is actually a OnOffAuto machine property and -no-acpi
> is just a shortcut to set that.
> 
>> Actually, what is the value added by '-no-acpi'?
> 
> On arm(64) the linux kernel can use either device trees or ACPI to find
> the hardware.  Historical reasons mostly, when the platform started
> there was no ACPI support.  Also the edk2 firmware uses Device Trees
> for hardware discovery, likewise for historical reasons.
> 
> When ACPI is available for a platform right from the start I see little
> reason to offer an option to turn it off though ...

Yeah I concur. There is no point in disabling ACPI on the RISCV virt
machine IMO.

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

[Sunil V L]

On Tue, Feb 07, 2023 at 09:50:29AM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 13:56, Gerd Hoffmann wrote:
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +                              virt_get_acpi, virt_set_acpi,
> > > > > +                              NULL, NULL);
> > > > > +    object_class_property_set_description(oc, "acpi",
> > > > > +                                          "Enable ACPI");
> > > > 
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> > > 
> > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > > has a high maintenance cost / burden.
> > 
> > Under the hood it is actually a OnOffAuto machine property and -no-acpi
> > is just a shortcut to set that.
> > 
> > > Actually, what is the value added by '-no-acpi'?
> > 
> > On arm(64) the linux kernel can use either device trees or ACPI to find
> > the hardware.  Historical reasons mostly, when the platform started
> > there was no ACPI support.  Also the edk2 firmware uses Device Trees
> > for hardware discovery, likewise for historical reasons.
> > 
> > When ACPI is available for a platform right from the start I see little
> > reason to offer an option to turn it off though ...
> 
> Yeah I concur. There is no point in disabling ACPI on the RISCV virt
> machine IMO.

Thank you all for the inputs. I am fine to keep it enabled by default.
Do you mean we don't need the switch at all even for some
testing/debugging purpose?

Thanks,
Sunil

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

[Andrew Jones]

On Tue, Feb 07, 2023 at 09:50:29AM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 13:56, Gerd Hoffmann wrote:
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +                              virt_get_acpi, virt_set_acpi,
> > > > > +                              NULL, NULL);
> > > > > +    object_class_property_set_description(oc, "acpi",
> > > > > +                                          "Enable ACPI");
> > > > 
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> > > 
> > > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > > has a high maintenance cost / burden.
> > 
> > Under the hood it is actually a OnOffAuto machine property and -no-acpi
> > is just a shortcut to set that.
> > 
> > > Actually, what is the value added by '-no-acpi'?
> > 
> > On arm(64) the linux kernel can use either device trees or ACPI to find
> > the hardware.  Historical reasons mostly, when the platform started
> > there was no ACPI support.  Also the edk2 firmware uses Device Trees
> > for hardware discovery, likewise for historical reasons.
> > 
> > When ACPI is available for a platform right from the start I see little
> > reason to offer an option to turn it off though ...
> 
> Yeah I concur. There is no point in disabling ACPI on the RISCV virt
> machine IMO.

edk2 will only present DT or ACPI to the guest, not both. However, RISCV
Linux supports both. If we don't offer '-no-acpi' as a way to switch to
DT, then edk2+DT users will need to configure the varstore to select it.
And, since testing needs to be done with both, that varstore change will
need to be added to all the testcases which need DT (or a varstore with
DT already selected will need to be maintained and used by the testsuites)

IMO, the generation of the ACPI tables should be 'on' by default, but then
the, already present, '-no-acpi' command line option should be made
available in order to easily inform edk2 to present DT instead of ACPI.

Thanks,
drew

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

[Andrew Jones]

On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> On 6/2/23 11:54, Andrea Bolognani wrote:
> > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > +                              virt_get_acpi, virt_set_acpi,
> > > +                              NULL, NULL);
> > > +    object_class_property_set_description(oc, "acpi",
> > > +                                          "Enable ACPI");
> > 
> > The way this works on other architectures (x86_64, aarch64) is that
> > you get ACPI by default and can use -no-acpi to disable it if
> > desired. Can we have the same on RISC-V, for consistency?
> 
> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> has a high maintenance cost / burden.

Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically.
If -no-acpi is problematic for some reason, then something like
'-machine virt,acpi=off' would be sufficient for switching to DT too.

Thanks,
drew

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

[Sunil V L]

On Tue, Feb 07, 2023 at 11:57:29AM +0800, Bin Meng wrote:
> On Mon, Feb 6, 2023 at 8:36 PM Andrew Jones <ajones@ventanamicro.com> wrote:
> >
> > On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > > +                              virt_get_acpi, virt_set_acpi,
> > > > > +                              NULL, NULL);
> > > > > +    object_class_property_set_description(oc, "acpi",
> > > > > +                                          "Enable ACPI");
> > > >
> > > > The way this works on other architectures (x86_64, aarch64) is that
> > > > you get ACPI by default and can use -no-acpi to disable it if
> > > > desired. Can we have the same on RISC-V, for consistency?
> >
> > Default on, with a user control to turn off, can be done with a boolean.
> > I'm not sure why/if Auto is needed for acpi. Auto is useful when a
> > configuration doesn't support a default setting for a feature. If the
> > user hasn't explicitly requested the feature to be on or off, then the
> > configuration can silently select what works. If, however, the user
> > explicitly chooses what doesn't work, then qemu will fail with an error
> > instead.
> 
> I have a confusion about "OnOffAuto" vs. "bool" type.
> 
> Both "OnOffAuto" vs. "bool" type property can have a default value if
> user does not assign a value to it from command line. The default
> value is:
> 
> - ON_OFF_AUTO_AUTO for "OnOffAuto"
> - false for "bool"
> 
> But the default value can be overridden in the machine's init
> function, like in this patch.
> 
> So I am not really sure how these 2 types of properties are different.
> Why did we introduce a "OnOffAuto" type, and how is that type supposed
> to be used in which scenario?
> 

I don't know either. Since it is the same property across architectures,
I used the OnOffAuto instead of a bool. 

May be Gerd and other qemu experts can help us to understand better.
https://github.com/qemu/qemu/commit/17e89077b7e3bc1d96540e21ddc7451c3e077049

Thanks,
Sunil

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

[Andrea Bolognani]

On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote:
> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
> > On 6/2/23 11:54, Andrea Bolognani wrote:
> > > On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
> > > > +    object_class_property_add(oc, "acpi", "OnOffAuto",
> > > > +                              virt_get_acpi, virt_set_acpi,
> > > > +                              NULL, NULL);
> > > > +    object_class_property_set_description(oc, "acpi",
> > > > +                                          "Enable ACPI");
> > >
> > > The way this works on other architectures (x86_64, aarch64) is that
> > > you get ACPI by default and can use -no-acpi to disable it if
> > > desired. Can we have the same on RISC-V, for consistency?
> >
> > -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
> > has a high maintenance cost / burden.
>
> Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically.
> If -no-acpi is problematic for some reason, then something like
> '-machine virt,acpi=off' would be sufficient for switching to DT too.

I would greatly prefer it if the command line interface could be kept
consistent across architectures.

It looks like i440fx and q35 both have an 'acpi' machine property. Is
-no-acpi just sugar for acpi=off? Is it considered desirable to get
rid of -no-acpi? If so, we should follow the usual deprecation
process and get rid of it after libvirt has had a chance to adapt.

In the scenario described above, it would make sense for RISC-V to
only offer the machine type option (assuming that -no-acpi doesn't
come for free with that) instead of putting additional effort into
implementing an interface that is already on its way out.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Thomas Huth]

On 07/02/2023 14.56, Andrea Bolognani wrote:
> On Tue, Feb 07, 2023 at 11:23:53AM +0100, Andrew Jones wrote:
>> On Mon, Feb 06, 2023 at 12:18:06PM +0100, Philippe Mathieu-Daudé wrote:
>>> On 6/2/23 11:54, Andrea Bolognani wrote:
>>>> On Thu, Feb 02, 2023 at 10:22:15AM +0530, Sunil V L wrote:
>>>>> +    object_class_property_add(oc, "acpi", "OnOffAuto",
>>>>> +                              virt_get_acpi, virt_set_acpi,
>>>>> +                              NULL, NULL);
>>>>> +    object_class_property_set_description(oc, "acpi",
>>>>> +                                          "Enable ACPI");
>>>>
>>>> The way this works on other architectures (x86_64, aarch64) is that
>>>> you get ACPI by default and can use -no-acpi to disable it if
>>>> desired. Can we have the same on RISC-V, for consistency?
>>>
>>> -no-acpi rather seems a x86-specific hack for the ISA PC machine, and
>>> has a high maintenance cost / burden.
>>
>> Can you elaborate on this? RISCV doesn't need '-no-acpi' specifically.
>> If -no-acpi is problematic for some reason, then something like
>> '-machine virt,acpi=off' would be sufficient for switching to DT too.
> 
> I would greatly prefer it if the command line interface could be kept
> consistent across architectures.
> 
> It looks like i440fx and q35 both have an 'acpi' machine property. Is
> -no-acpi just sugar for acpi=off?

Yes, it is, see softmmu/vl.c:

             case QEMU_OPTION_no_acpi:
                 qdict_put_str(machine_opts_dict, "acpi", "off");
                 break;

> Is it considered desirable to get rid of -no-acpi?

Sounds like a good idea, indeed!

> If so, we should follow the usual deprecation
> process and get rid of it after libvirt has had a chance to adapt.
> 
> In the scenario described above, it would make sense for RISC-V to
> only offer the machine type option (assuming that -no-acpi doesn't
> come for free with that) instead of putting additional effort into
> implementing an interface that is already on its way out.

I agree. IMHO the machine parameter should be enough, no need to introduce 
"-no-acpi" here.

  Thomas

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

[Andrea Bolognani]

On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote:
> On 07/02/2023 14.56, Andrea Bolognani wrote:
> > It looks like i440fx and q35 both have an 'acpi' machine property. Is
> > -no-acpi just sugar for acpi=off?
>
> Yes, it is, see softmmu/vl.c:
>
>             case QEMU_OPTION_no_acpi:
>                 qdict_put_str(machine_opts_dict, "acpi", "off");
>                 break;
>
> > Is it considered desirable to get rid of -no-acpi?
>
> Sounds like a good idea, indeed!
>
> > If so, we should follow the usual deprecation
> > process and get rid of it after libvirt has had a chance to adapt.
> >
> > In the scenario described above, it would make sense for RISC-V to
> > only offer the machine type option (assuming that -no-acpi doesn't
> > come for free with that) instead of putting additional effort into
> > implementing an interface that is already on its way out.
>
> I agree. IMHO the machine parameter should be enough, no need to introduce
> "-no-acpi" here.

Well, it looks like -no-acpi will come for free after all, thanks to
the code you pasted above, as long as the machine property follows
the convention established by x86, arm and (I just noticed this one)
loongarch.

So I guess the -no-acpi deprecation can be handled separately, and
the only thing that needs changing in the current patch is making
sure that ACPI is opt-out rather than opt-in, as is the case for
other architectures :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Bin Meng]

On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.
> >
> > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > some refactoring work is needed before ACPI support fully lands on
> > RISC-V.
> > For example, we can extract the common part among x86/arm/riscv into a
> > separate file, like hw/acpi/acpi-build.c?
> >
>
> While it appears like there is same code in multiple places, those
> functions take architecture specific MachineState parameter. Some tables
> like MADT though with same name, will have different contents for
> different architectures.
>
> Only one function which Daniel also pointed is the wrapper
> acpi_align_size() which can be made common. I am not
> sure whether it is worth of refactoring.
>

It's more than that. For example,

With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
of cpus, so there is no need to pass the architecture specific
MachineState as the parameter.

Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.

Regards,
Bin

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

[Sunil V L]

On Wed, Feb 08, 2023 at 12:10:55AM +0800, Bin Meng wrote:
> On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> > > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.
> > >
> > > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > > some refactoring work is needed before ACPI support fully lands on
> > > RISC-V.
> > > For example, we can extract the common part among x86/arm/riscv into a
> > > separate file, like hw/acpi/acpi-build.c?
> > >
> >
> > While it appears like there is same code in multiple places, those
> > functions take architecture specific MachineState parameter. Some tables
> > like MADT though with same name, will have different contents for
> > different architectures.
> >
> > Only one function which Daniel also pointed is the wrapper
> > acpi_align_size() which can be made common. I am not
> > sure whether it is worth of refactoring.
> >
> 
> It's more than that. For example,
> 
> With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
> of cpus, so there is no need to pass the architecture specific
> MachineState as the parameter.
> 
I would not make this function common. The reason is, an architecture may
choose to attach different ACPI objects under the CPU node.

> Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.
> 
The issue is, these things are not exactly common across all architectures.
x86 has bit different data in these objects. While today it appears they
are same for riscv and arm, in future things may change for an architecture.
It doesn't look like it is a standard practice to build files under
hw/acpi for specific architectures. Hence, IMO, it is better to keep these
things in architecture specific folders to allow them to do differently in
future.

But I look forward for the feedback from other architecture maintainers on
this topic. My experience in qemu is very limited. So, I need help from
experts.

Thanks!
Sunil

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

[Andrew Jones]

On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:
> On Tue, Feb 07, 2023 at 03:02:19PM +0100, Thomas Huth wrote:
> > On 07/02/2023 14.56, Andrea Bolognani wrote:
> > > It looks like i440fx and q35 both have an 'acpi' machine property. Is
> > > -no-acpi just sugar for acpi=off?
> >
> > Yes, it is, see softmmu/vl.c:
> >
> >             case QEMU_OPTION_no_acpi:
> >                 qdict_put_str(machine_opts_dict, "acpi", "off");
> >                 break;
> >
> > > Is it considered desirable to get rid of -no-acpi?
> >
> > Sounds like a good idea, indeed!
> >
> > > If so, we should follow the usual deprecation
> > > process and get rid of it after libvirt has had a chance to adapt.
> > >
> > > In the scenario described above, it would make sense for RISC-V to
> > > only offer the machine type option (assuming that -no-acpi doesn't
> > > come for free with that) instead of putting additional effort into
> > > implementing an interface that is already on its way out.
> >
> > I agree. IMHO the machine parameter should be enough, no need to introduce
> > "-no-acpi" here.
> 
> Well, it looks like -no-acpi will come for free after all, thanks to
> the code you pasted above, as long as the machine property follows
> the convention established by x86, arm and (I just noticed this one)
> loongarch.

Not quite, because we also have

$ grep -A1 '"no-acpi"' qemu-options.hx
DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
           "-no-acpi        disable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)

So that means neither riscv nor loongarch get -no-acpi just by adding
the "acpi" machine property.

If the plan is to deprecate -no-acpi, then riscv can be like loongarch
and only have the "acpi" property from the start.

Thanks,
drew

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

[Bin Meng]

On Wed, Feb 8, 2023 at 2:15 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> On Wed, Feb 08, 2023 at 12:10:55AM +0800, Bin Meng wrote:
> > On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> > >
> > > On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> > > > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.
> > > >
> > > > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > > > some refactoring work is needed before ACPI support fully lands on
> > > > RISC-V.
> > > > For example, we can extract the common part among x86/arm/riscv into a
> > > > separate file, like hw/acpi/acpi-build.c?
> > > >
> > >
> > > While it appears like there is same code in multiple places, those
> > > functions take architecture specific MachineState parameter. Some tables
> > > like MADT though with same name, will have different contents for
> > > different architectures.
> > >
> > > Only one function which Daniel also pointed is the wrapper
> > > acpi_align_size() which can be made common. I am not
> > > sure whether it is worth of refactoring.
> > >
> >
> > It's more than that. For example,
> >
> > With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
> > of cpus, so there is no need to pass the architecture specific
> > MachineState as the parameter.
> >
> I would not make this function common. The reason is, an architecture may
> choose to attach different ACPI objects under the CPU node.
>

Is it possible to insert architect specific CPU nodes after this
common API builds the basic CPU node in DSDT?

> > Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.
> >
> The issue is, these things are not exactly common across all architectures.
> x86 has bit different data in these objects. While today it appears they
> are same for riscv and arm, in future things may change for an architecture.
> It doesn't look like it is a standard practice to build files under
> hw/acpi for specific architectures. Hence, IMO, it is better to keep these
> things in architecture specific folders to allow them to do differently in
> future.
>

It looks like hw/acpi/ghes.c is for Arm from MAINTAINERS.

> But I look forward for the feedback from other architecture maintainers on
> this topic. My experience in qemu is very limited. So, I need help from
> experts.

+ Michael and Igor

Regards,
Bin

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

[Sunil V L]

On Wed, Feb 08, 2023 at 09:06:48AM +0800, Bin Meng wrote:
> On Wed, Feb 8, 2023 at 2:15 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Wed, Feb 08, 2023 at 12:10:55AM +0800, Bin Meng wrote:
> > > On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
> > > >
> > > > On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:
> > > > > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.
> > > > >
> > > > > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > > > > some refactoring work is needed before ACPI support fully lands on
> > > > > RISC-V.
> > > > > For example, we can extract the common part among x86/arm/riscv into a
> > > > > separate file, like hw/acpi/acpi-build.c?
> > > > >
> > > >
> > > > While it appears like there is same code in multiple places, those
> > > > functions take architecture specific MachineState parameter. Some tables
> > > > like MADT though with same name, will have different contents for
> > > > different architectures.
> > > >
> > > > Only one function which Daniel also pointed is the wrapper
> > > > acpi_align_size() which can be made common. I am not
> > > > sure whether it is worth of refactoring.
> > > >
> > >
> > > It's more than that. For example,
> > >
> > > With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
> > > of cpus, so there is no need to pass the architecture specific
> > > MachineState as the parameter.
> > >
> > I would not make this function common. The reason is, an architecture may
> > choose to attach different ACPI objects under the CPU node.
> >
> 
> Is it possible to insert architect specific CPU nodes after this
> common API builds the basic CPU node in DSDT?
> 
No. They need to be added in the same loop. Otherwise, it will cause
issues to the AML interpreter.

> > > Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.
> > >
> > The issue is, these things are not exactly common across all architectures.
> > x86 has bit different data in these objects. While today it appears they
> > are same for riscv and arm, in future things may change for an architecture.
> > It doesn't look like it is a standard practice to build files under
> > hw/acpi for specific architectures. Hence, IMO, it is better to keep these
> > things in architecture specific folders to allow them to do differently in
> > future.
> >
> 
> It looks like hw/acpi/ghes.c is for Arm from MAINTAINERS.
> 
APEI is a standard feature but it is up to the architecture to use it or
not. Probably, it is maintained by ARM since they adopted first. But if
you look at hw/acpi/meson.build, it is not architecture specific.

> > But I look forward for the feedback from other architecture maintainers on
> > this topic. My experience in qemu is very limited. So, I need help from
> > experts.
> 
> + Michael and Igor
> 
Thanks!
Sunil

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

[Andrea Bolognani]

On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote:
> On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:
> > Well, it looks like -no-acpi will come for free after all, thanks to
> > the code you pasted above, as long as the machine property follows
> > the convention established by x86, arm and (I just noticed this one)
> > loongarch.
>
> Not quite, because we also have
>
> $ grep -A1 '"no-acpi"' qemu-options.hx
> DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
>            "-no-acpi        disable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)
>
> So that means neither riscv nor loongarch get -no-acpi just by adding
> the "acpi" machine property.
>
> If the plan is to deprecate -no-acpi, then riscv can be like loongarch
> and only have the "acpi" property from the start.

Makes sense.


For the libvirt integration, do I understand correctly that the
machine property was added by commit

  commit 17e89077b7e3bc1d96540e21ddc7451c3e077049
  Author: Gerd Hoffmann <kraxel@redhat.com>
  Date:   Fri Mar 20 11:01:36 2020 +0100

    acpi: add acpi=OnOffAuto machine property to x86 and arm virt

    Remove the global acpi_enabled bool and replace it with an
    acpi OnOffAuto machine property.

    qemu throws an error now if you use -no-acpi while the machine
    type you are using doesn't support acpi in the first place.

    Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
    Message-Id: <20200320100136.11717-1-kraxel@redhat.com>
    Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
    Acked-by: Paolo Bonzini <pbonzini@redhat.com>
    Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so
we'll have to make the use of the machine property conditional.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Alistair Francis]

On Thu, Feb 2, 2023 at 2:54 PM Sunil V L <sunilvl@ventanamicro.com> 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>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 a061151a6f..7ad9fda20c 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
> @@ -1512,6 +1513,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.38.0
>
>

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

[Alistair Francis]

On Thu, Feb 2, 2023 at 2:54 PM Sunil V L <sunilvl@ventanamicro.com> 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>

Acked-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 84962962ff..26caea59ff 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -1459,6 +1459,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.38.0
>
>

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

[Alistair Francis]

On Thu, Feb 2, 2023 at 2:54 PM Sunil V L <sunilvl@ventanamicro.com> 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>

Alistair

> ---
>  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 0410b955bd..2f65f1e2e5 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -137,6 +137,43 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
>      free_aml_allocator();
>  }
>
> +/* MADT */
> +static void
> +build_madt(GArray *table_data, BIOSLinker *linker, RISCVVirtState *vms)
> +{
> +    MachineState *mc = MACHINE(vms);
> +    int socket;
> +    uint16_t base_hartid = 0;
> +    uint32_t cpu_id = 0;
> +
> +    AcpiTable table = { .sig = "APIC", .rev = 6, .oem_id = vms->oem_id,
> +                        .oem_table_id = vms->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 < vms->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 *vms, AcpiBuildTables *tables)
>  {
> --
> 2.38.0
>
>

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

[Alistair Francis]

On Thu, Feb 2, 2023 at 2:54 PM Sunil V L <sunilvl@ventanamicro.com> 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: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  MAINTAINERS | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c581c11a64..23fcaaf54a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -995,6 +995,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.38.0
>
>

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

[Thomas Huth]

On 08/02/2023 17.48, Andrea Bolognani wrote:
> On Tue, Feb 07, 2023 at 08:20:31PM +0100, Andrew Jones wrote:
>> On Tue, Feb 07, 2023 at 06:38:15AM -0800, Andrea Bolognani wrote:
>>> Well, it looks like -no-acpi will come for free after all, thanks to
>>> the code you pasted above, as long as the machine property follows
>>> the convention established by x86, arm and (I just noticed this one)
>>> loongarch.
>>
>> Not quite, because we also have
>>
>> $ grep -A1 '"no-acpi"' qemu-options.hx
>> DEF("no-acpi", 0, QEMU_OPTION_no_acpi,
>>             "-no-acpi        disable ACPI\n", QEMU_ARCH_I386 | QEMU_ARCH_ARM)
>>
>> So that means neither riscv nor loongarch get -no-acpi just by adding
>> the "acpi" machine property.
>>
>> If the plan is to deprecate -no-acpi, then riscv can be like loongarch
>> and only have the "acpi" property from the start.
> 
> Makes sense.
> 
> 
> For the libvirt integration, do I understand correctly that the
> machine property was added by commit
> 
>    commit 17e89077b7e3bc1d96540e21ddc7451c3e077049
>    Author: Gerd Hoffmann <kraxel@redhat.com>
>    Date:   Fri Mar 20 11:01:36 2020 +0100
> 
>      acpi: add acpi=OnOffAuto machine property to x86 and arm virt
> 
>      Remove the global acpi_enabled bool and replace it with an
>      acpi OnOffAuto machine property.
> 
>      qemu throws an error now if you use -no-acpi while the machine
>      type you are using doesn't support acpi in the first place.
> 
>      Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>      Message-Id: <20200320100136.11717-1-kraxel@redhat.com>
>      Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>      Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>      Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
>      Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> included in QEMU 5.0? libvirt still officially supports QEMU 4.2, so
> we'll have to make the use of the machine property conditional.

Sounds right, and testing for the machine option should be possible with the 
upcoming QEMU 8.0. FWIW, I assume this is similar to the -no-hpet option 
which has been turned into a machine property, too:

https://gitlab.com/libvirt/libvirt/-/commit/3c508e7d4310aeb8

  Thomas

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

[Igor Mammedov]

On Wed, 8 Feb 2023 09:06:48 +0800
Bin Meng <bmeng.cn@gmail.com> wrote:

> On Wed, Feb 8, 2023 at 2:15 AM Sunil V L <sunilvl@ventanamicro.com> wrote:
> >
> > On Wed, Feb 08, 2023 at 12:10:55AM +0800, Bin Meng wrote:  
> > > On Mon, Feb 6, 2023 at 9:24 PM Sunil V L <sunilvl@ventanamicro.com> wrote:  
> > > >
> > > > On Mon, Feb 06, 2023 at 06:17:50PM +0800, Bin Meng wrote:  
> > > > > On Thu, Feb 2, 2023 at 12:54 PM Sunil V L <sunilvl@ventanamicro.com> 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.  
> > > > >
> > > > > There are lots of same ACPI codes existing in x86/arm/riscv. I believe
> > > > > some refactoring work is needed before ACPI support fully lands on
> > > > > RISC-V.
> > > > > For example, we can extract the common part among x86/arm/riscv into a
> > > > > separate file, like hw/acpi/acpi-build.c?
> > > > >  
> > > >
> > > > While it appears like there is same code in multiple places, those
> > > > functions take architecture specific MachineState parameter. Some tables
> > > > like MADT though with same name, will have different contents for
> > > > different architectures.
> > > >
> > > > Only one function which Daniel also pointed is the wrapper
> > > > acpi_align_size() which can be made common. I am not
> > > > sure whether it is worth of refactoring.
> > > >  
> > >
> > > It's more than that. For example,
> > >
> > > With acpi_dsdt_add_cpus(), the only meaningful parameter is the number
> > > of cpus, so there is no need to pass the architecture specific
> > > MachineState as the parameter.
> > >  
> > I would not make this function common. The reason is, an architecture may
> > choose to attach different ACPI objects under the CPU node.
> >  
> 
> Is it possible to insert architect specific CPU nodes after this
> common API builds the basic CPU node in DSDT?

What do you mean by "architect specific CPU", any examples?

> 
> > > Likewise, acpi_dsdt_add_fw_cfg() can be made generic as well.
> > >  
> > The issue is, these things are not exactly common across all architectures.
> > x86 has bit different data in these objects. While today it appears they
> > are same for riscv and arm, in future things may change for an architecture.
> > It doesn't look like it is a standard practice to build files under
> > hw/acpi for specific architectures. Hence, IMO, it is better to keep these
> > things in architecture specific folders to allow them to do differently in
> > future.
> >  
> 
> It looks like hw/acpi/ghes.c is for Arm from MAINTAINERS.
> 
> > But I look forward for the feedback from other architecture maintainers on
> > this topic. My experience in qemu is very limited. So, I need help from
> > experts.  
> 
> + Michael and Igor
> 
> Regards,
> Bin
>