Re: [PULL 30/53] hw/acpi: Fix PM control register access

[Igor Mammedov]

On Mon, 26 Jun 2023 08:29:19 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> From: BALATON Zoltan <balaton@eik.bme.hu>
> 
> 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.
> 
> The issue below is possibly related to the same memory core bug.
> 
> Link: https://gitlab.com/qemu-project/qemu/-/issues/360
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Message-Id: <20230607200125.A9988746377@zero.eik.bme.hu>
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Somewhere you lost mine
Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> ---
>  hw/acpi/core.c | 56 +++++++++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 28 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,8 +551,35 @@ void acpi_pm_tmr_reset(ACPIREGS *ar)
>  }
>  
>  /* ACPI PM1aCNT */
> -static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
> +void acpi_pm1_cnt_update(ACPIREGS *ar,
> +                         bool sci_enable, bool sci_disable)
>  {
> +    /* ACPI specs 3.0, 4.7.2.5 */
> +    if (ar->pm1.cnt.acpi_only) {
> +        return;
> +    }
> +
> +    if (sci_enable) {
> +        ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
> +    } else if (sci_disable) {
> +        ar->pm1.cnt.cnt &= ~ACPI_BITMASK_SCI_ENABLE;
> +    }
> +}
> +
> +static uint64_t acpi_pm_cnt_read(void *opaque, hwaddr addr, unsigned width)
> +{
> +    ACPIREGS *ar = opaque;
> +    return ar->pm1.cnt.cnt >> addr * 8;
> +}
> +
> +static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
> +                              unsigned width)
> +{
> +    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) {
> @@ -575,33 +602,6 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t 
> val)
>      }
>  }
>  
> -void acpi_pm1_cnt_update(ACPIREGS *ar,
> -                         bool sci_enable, bool sci_disable)
> -{
> -    /* ACPI specs 3.0, 4.7.2.5 */
> -    if (ar->pm1.cnt.acpi_only) {
> -        return;
> -    }
> -
> -    if (sci_enable) {
> -        ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
> -    } else if (sci_disable) {
> -        ar->pm1.cnt.cnt &= ~ACPI_BITMASK_SCI_ENABLE;
> -    }
> -}
> -
> -static uint64_t acpi_pm_cnt_read(void *opaque, hwaddr addr, unsigned width)
> -{
> -    ACPIREGS *ar = opaque;
> -    return ar->pm1.cnt.cnt;
> -}
> -
> -static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
> -                              unsigned width)
> -{
> -    acpi_pm1_cnt_write(opaque, val);
> -}
> -
>  static const MemoryRegionOps acpi_pm_cnt_ops = {
>      .read = acpi_pm_cnt_read,
>      .write = acpi_pm_cnt_write,