Hi Xuemei Liu,
Thank you for the patch!
On Thu, Jun 19, 2025 at 03:56:26PM +0800, liu.xuem...@zte.com.cn wrote:
> From: Xuemei Liu <liu.xuem...@zte.com.cn>
>
> This adds powerdown support by implementing the ACPI GED.
>
> Signed-off-by: Xuemei Liu <liu.xuem...@zte.com.cn>
> Co-authored-by: Björn Töpel <bj...@rivosinc.com>
> ---
> hw/riscv/Kconfig | 1 +
> hw/riscv/virt-acpi-build.c | 12 ++++++++++++
> hw/riscv/virt.c | 32 ++++++++++++++++++++++++++++++++
> include/hw/riscv/virt.h | 4 ++++
> 4 files changed, 49 insertions(+)
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index e6a0ac1fa1..16837e0e8f 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -68,6 +68,7 @@ config RISCV_VIRT
> select PLATFORM_BUS
> select ACPI
> select ACPI_PCI
> + select ACPI_HW_REDUCED
>
> config SHAKTI_C
> bool
> diff --git a/hw/riscv/virt-acpi-build.c b/hw/riscv/virt-acpi-build.c
> index 8b5683dbde..89365c40b4 100644
> --- a/hw/riscv/virt-acpi-build.c
> +++ b/hw/riscv/virt-acpi-build.c
> @@ -29,6 +29,7 @@
> #include "hw/acpi/aml-build.h"
> #include "hw/acpi/pci.h"
> #include "hw/acpi/utils.h"
> +#include "hw/acpi/generic_event_device.h"
>
NIT: Could you include this after aml-build.h to keep it sorted?
> #include "hw/intc/riscv_aclint.h"
> #include "hw/nvram/fw_cfg_acpi.h"
> #include "hw/pci-host/gpex.h"
> @@ -224,6 +225,16 @@ static void acpi_dsdt_add_iommu_sys(Aml *scope, const
> MemMapEntry *iommu_memmap,
> aml_append(scope, dev);
> }
>
> +static void acpi_dsdt_add_ged(Aml *scope, RISCVVirtState *s)
> +{
> + build_ged_aml(scope, "\\_SB."GED_DEVICE,
> + HOTPLUG_HANDLER(s->acpi_ged),
> + ACPI_GED_IRQ, AML_SYSTEM_MEMORY,
> + s->memmap[VIRT_ACPI_GED].base);
> +
> + acpi_dsdt_add_power_button(scope);
>
Instead of this wrapper function to create both ged and power button,
please call them directly below where you are calling
acpi_dsdt_add_ged(). It will help in future to add some other eventing
mechanism to the power button.
> +}
> +
> /*
> * Serial Port Console Redirection Table (SPCR)
> * Rev: 1.10
> @@ -497,6 +508,7 @@ static void build_dsdt(GArray *table_data,
> VIRTIO_COUNT);
> acpi_dsdt_add_gpex_host(scope, PCIE_IRQ + VIRT_IRQCHIP_NUM_SOURCES *
> 2);
> }
> + acpi_dsdt_add_ged(scope, s);
>
This should be conditional - only if s->acpi_ged is set.
> aml_append(dsdt, scope);
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index cf280a92e5..aaa33b9740 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -51,10 +51,12 @@
> #include "system/kvm.h"
> #include "system/tpm.h"
> #include "system/qtest.h"
> +#include "system/runstate.h"
> #include "hw/pci/pci.h"
> #include "hw/pci-host/gpex.h"
> #include "hw/display/ramfb.h"
> #include "hw/acpi/aml-build.h"
> +#include "hw/acpi/generic_event_device.h"
> #include "qapi/qapi-visit-common.h"
> #include "hw/virtio/virtio-iommu.h"
> #include "hw/uefi/var-service-api.h"
> @@ -95,6 +97,7 @@ static const MemMapEntry virt_memmap[] = {
> [VIRT_UART0] = { 0x10000000, 0x100 },
> [VIRT_VIRTIO] = { 0x10001000, 0x1000 },
> [VIRT_FW_CFG] = { 0x10100000, 0x18 },
> + [VIRT_ACPI_GED] = { 0x10200000, ACPI_GED_EVT_SEL_LEN },
Do we need to align the base address at 1MB? It can create a larger hole
unnecessarily.
> [VIRT_FLASH] = { 0x20000000, 0x4000000 },
> [VIRT_IMSIC_M] = { 0x24000000, VIRT_IMSIC_MAX_SIZE },
> [VIRT_IMSIC_S] = { 0x28000000, VIRT_IMSIC_MAX_SIZE },
> @@ -1272,6 +1275,21 @@ static inline DeviceState *gpex_pcie_init(MemoryRegion
> *sys_mem,
> return dev;
> }
>
> +static DeviceState *create_acpi_ged(RISCVVirtState *s, DeviceState *irqchip)
> +{
> + DeviceState *dev;
> + uint32_t event = ACPI_GED_PWR_DOWN_EVT;
> +
> + dev = qdev_new(TYPE_ACPI_GED);
> + qdev_prop_set_uint32(dev, "ged-event", event);
> + sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> +
> + sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, s->memmap[VIRT_ACPI_GED].base);
> + sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, qdev_get_gpio_in(irqchip,
> ACPI_GED_IRQ));
> +
> + return dev;
> +}
> +
> static FWCfgState *create_fw_cfg(const MachineState *ms, hwaddr base)
> {
> FWCfgState *fw_cfg;
> @@ -1430,6 +1448,14 @@ static void virt_build_smbios(RISCVVirtState *s)
> }
> }
>
> +static void virt_powerdown_req(Notifier *notifier, void *opaque)
> +{
> + RISCVVirtState *s;
> +
> + s = container_of(notifier, RISCVVirtState, powerdown_notifier);
> + acpi_send_event(s->acpi_ged, ACPI_POWER_DOWN_STATUS);
Check for s->acpi_ged here.
> +}
> +
> static void virt_machine_done(Notifier *notifier, void *data)
> {
> RISCVVirtState *s = container_of(notifier, RISCVVirtState,
> @@ -1703,6 +1729,9 @@ static void virt_machine_init(MachineState *machine)
>
> create_platform_bus(s, mmio_irqchip);
>
> + /* acpi ged */
> + s->acpi_ged = create_acpi_ged(s, mmio_irqchip);
> +
>
I think we should create GED only if ACPI is enabled.
Thanks!
Sunil
