Re: [PATCH 1/1] hw/loongarch/virt: add system_powerdown hmp command support

[Philippe Mathieu-Daudé]

On 12/1/23 07:11, Song Gao wrote:
> For loongarch virt machine, add powerdown notification callback
> and send ACPI_POWER_DOWN_STATUS event by acpi ged. Also add
> acpi dsdt table for ACPI_POWER_BUTTON_DEVICE device in this
> patch.
>
> Signed-off-by: Song Gao <gaosong@loongson.cn>
> ---
>   hw/loongarch/acpi-build.c   |  1 +
>   hw/loongarch/virt.c         | 14 ++++++++++++++
>   include/hw/loongarch/virt.h |  1 +
>   3 files changed, 16 insertions(+)

Cc'ing ACPI maintainers:

$ git grep acpi_send_event
include/hw/acpi/acpi_dev_interface.h:30:void acpi_send_event(DeviceState 
*dev, AcpiEventStatusBits event);

$ ./scripts/get_maintainer.pl -f include/hw/acpi/acpi_dev_interface.h
"Michael S. Tsirkin" <mst@redhat.com> (supporter:ACPI/SMBIOS)
Igor Mammedov <imammedo@redhat.com> (supporter:ACPI/SMBIOS)
Ani Sinha <ani@anisinha.ca> (reviewer:ACPI/SMBIOS)
qemu-devel@nongnu.org (open list:All patches CC here)

> diff --git a/hw/loongarch/acpi-build.c b/hw/loongarch/acpi-build.c
> index c2b237736d..b7601cb284 100644
> --- a/hw/loongarch/acpi-build.c
> +++ b/hw/loongarch/acpi-build.c
> @@ -261,6 +261,7 @@ build_la_ged_aml(Aml *dsdt, MachineState *machine)
>                                    AML_SYSTEM_MEMORY,
>                                    VIRT_GED_MEM_ADDR);
>       }
> +    acpi_dsdt_add_power_button(dsdt);
>   }
>   
>   static void build_pci_device_aml(Aml *scope, LoongArchMachineState *lams)
> diff --git a/hw/loongarch/virt.c b/hw/loongarch/virt.c
> index 66be925068..a4998599d3 100644
> --- a/hw/loongarch/virt.c
> +++ b/hw/loongarch/virt.c
> @@ -316,6 +316,16 @@ static void virt_machine_done(Notifier *notifier, void 
> *data)
>       loongarch_acpi_setup(lams);
>   }
>   
> +static void virt_powerdown_req(Notifier *notifier, void *opaque)
> +{
> +    LoongArchMachineState *s = container_of(notifier,
> +                                   LoongArchMachineState, powerdown_notifier);
> +
> +    if (s->acpi_ged) {

Why check for acpi_ged? AFAICT it is always set.

Otherwise LGTM.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

> +        acpi_send_event(s->acpi_ged, ACPI_POWER_DOWN_STATUS);
> +    }
> +}
> +
>   struct memmap_entry {
>       uint64_t address;
>       uint64_t length;
> @@ -859,6 +869,10 @@ static void loongarch_init(MachineState *machine)
>                                      VIRT_PLATFORM_BUS_IRQ);
>       lams->machine_done.notify = virt_machine_done;
>       qemu_add_machine_init_done_notifier(&lams->machine_done);
> +     /* connect powerdown request */
> +    lams->powerdown_notifier.notify = virt_powerdown_req;
> +    qemu_register_powerdown_notifier(&lams->powerdown_notifier);
> +
>       fdt_add_pcie_node(lams);
>       /*
>        * Since lowmem region starts from 0 and Linux kernel legacy start address
> diff --git a/include/hw/loongarch/virt.h b/include/hw/loongarch/virt.h
> index f5f818894e..7ae8a91229 100644
> --- a/include/hw/loongarch/virt.h
> +++ b/include/hw/loongarch/virt.h
> @@ -45,6 +45,7 @@ struct LoongArchMachineState {
>       /* State for other subsystems/APIs: */
>       FWCfgState  *fw_cfg;
>       Notifier     machine_done;
> +    Notifier     powerdown_notifier;
>       OnOffAuto    acpi;
>       char         *oem_id;
>       char         *oem_table_id;