Re: [PATCH v2] hw/acpi: Fix PM control register access

[Igor Mammedov]

On Wed,  7 Jun 2023 22:01:25 +0200 (CEST)
BALATON Zoltan <balaton@eik.bme.hu> wrote:

> On pegasos2 which has ACPI as part of VT8231 south bridge the board
> firmware writes PM control register by accessing the second byte so
> addr will be 1. This wasn't handled correctly and the write went to
> addr 0 instead. Remove the acpi_pm1_cnt_write() function which is used
> only once and does not take addr into account and handle non-zero
> address in acpi_pm_cnt_{read|write}. This fixes ACPI shutdown with
> pegasos2 firmware.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/core.c | 52 +++++++++++++++++++++++++-------------------------
>  1 file changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 6da275c599..00b1e79a30 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -551,30 +551,6 @@ void acpi_pm_tmr_reset(ACPIREGS *ar)
>  }
>  
>  /* ACPI PM1aCNT */
> -static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
> -{
> -    ar->pm1.cnt.cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
> -
> -    if (val & ACPI_BITMASK_SLEEP_ENABLE) {
> -        /* change suspend type */
> -        uint16_t sus_typ = (val >> 10) & 7;
> -        switch (sus_typ) {
> -        case 0: /* soft power off */
> -            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> -            break;
> -        case 1:
> -            qemu_system_suspend_request();
> -            break;
> -        default:
> -            if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
> -                qapi_event_send_suspend_disk();
> -                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> -            }
> -            break;
> -        }
> -    }
> -}
> -
>  void acpi_pm1_cnt_update(ACPIREGS *ar,
>                           bool sci_enable, bool sci_disable)
>  {
> @@ -593,13 +569,37 @@ void acpi_pm1_cnt_update(ACPIREGS *ar,
>  static uint64_t acpi_pm_cnt_read(void *opaque, hwaddr addr, unsigned width)
>  {
>      ACPIREGS *ar = opaque;
> -    return ar->pm1.cnt.cnt;
> +    return ar->pm1.cnt.cnt >> addr * 8;
>  }
>  
>  static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>                                unsigned width)
>  {
> -    acpi_pm1_cnt_write(opaque, val);
> +    ACPIREGS *ar = opaque;
> +
> +    if (addr == 1) {
> +        val = val << 8 | (ar->pm1.cnt.cnt & 0xff);
> +    }
> +    ar->pm1.cnt.cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
> +
> +    if (val & ACPI_BITMASK_SLEEP_ENABLE) {
> +        /* change suspend type */
> +        uint16_t sus_typ = (val >> 10) & 7;
> +        switch (sus_typ) {
> +        case 0: /* soft power off */
> +            qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +            break;
> +        case 1:
> +            qemu_system_suspend_request();
> +            break;
> +        default:
> +            if (sus_typ == ar->pm1.cnt.s4_val) { /* S4 request */
> +                qapi_event_send_suspend_disk();
> +                qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
> +            }
> +            break;
> +        }
> +    }
>  }
>  
>  static const MemoryRegionOps acpi_pm_cnt_ops = {