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

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

[BALATON Zoltan]

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>
---
 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 = {
-- 
2.30.9

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 = {

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

[Mark Cave-Ayland]

On 07/06/2023 21:01, BALATON Zoltan 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>
> ---
>   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;

This shift here...

>   }
>   
>   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);
> +    }

and this shift here look similar to my workaround in 
https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/ 
which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.

Whilst there is no imminent fix for the above issue, it may be worth a few mins to 
determine if this is the same issue and if so document it with comments accordingly 
as I did so that the workaround can be removed at a later date.

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


ATB,

Mark.

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

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
> On 07/06/2023 21:01, BALATON Zoltan 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>
> > ---
> >   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;
> 
> This shift here...
> 
> >   }
> >   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);
> > +    }
> 
> and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
> which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
> 
> Whilst there is no imminent fix for the above issue, it may be worth a few
> mins to determine if this is the same issue and if so document it with
> comments accordingly as I did so that the workaround can be removed at a
> later date.

So I will add
this triggers a but in memory core,
(see
https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)

?

> > +    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 = {
> 
> 
> ATB,
> 
> Mark.

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

[BALATON Zoltan]

On Thu, 8 Jun 2023, Michael S. Tsirkin wrote:
> On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
>> On 07/06/2023 21:01, BALATON Zoltan 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>
>>> ---
>>>   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;
>>
>> This shift here...
>>
>>>   }
>>>   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);
>>> +    }
>>
>> and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
>> which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
>>
>> Whilst there is no imminent fix for the above issue, it may be worth a few
>> mins to determine if this is the same issue and if so document it with
>> comments accordingly as I did so that the workaround can be removed at a
>> later date.
>
> So I will add
> this triggers a but in memory core,
> (see
> https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
>
> ?

Apart from the typo but -> bug I'm not sure this is related to that issue 
but in any case this does not trigger but works around some possible bug 
so maybe "This work around may be related to issue URL" or something like 
that maybe? I'm also not sure what comment to add where so I'd appreciate 
if you can handle this on merging.

Regards,
BALATON Zoltan

>>> +    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 = {
>>
>>
>> ATB,
>>
>> Mark.
>
>

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

[Mark Cave-Ayland]

On 08/06/2023 16:13, Michael S. Tsirkin wrote:

> On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
>> On 07/06/2023 21:01, BALATON Zoltan 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>
>>> ---
>>>    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;
>>
>> This shift here...
>>
>>>    }
>>>    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);
>>> +    }
>>
>> and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
>> which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
>>
>> Whilst there is no imminent fix for the above issue, it may be worth a few
>> mins to determine if this is the same issue and if so document it with
>> comments accordingly as I did so that the workaround can be removed at a
>> later date.
> 
> So I will add
> this triggers a but in memory core,
> (see
> https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
> 
> ?

Well it was just an observation based on its similarity to my patch, but it would 
need a quick check with the debugger to step back up from the access to confirm if it 
were caused by the same issue (see the above issue #360 for detail).

As per my previous message no objection to the patch, but if it does have the same 
underlying cause then it could explain why this has been a tricky problem to solve 
(as in general the memory API "just works") and also provide another test case for a 
potential fix.

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


ATB,

Mark.

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

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 08:35:42PM +0100, Mark Cave-Ayland wrote:
> On 08/06/2023 16:13, Michael S. Tsirkin wrote:
> 
> > On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
> > > On 07/06/2023 21:01, BALATON Zoltan 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>
> > > > ---
> > > >    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;
> > > 
> > > This shift here...
> > > 
> > > >    }
> > > >    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);
> > > > +    }
> > > 
> > > and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
> > > which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
> > > 
> > > Whilst there is no imminent fix for the above issue, it may be worth a few
> > > mins to determine if this is the same issue and if so document it with
> > > comments accordingly as I did so that the workaround can be removed at a
> > > later date.
> > 
> > So I will add
> > this triggers a but in memory core,
> > (see
> > https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
> > 
> > ?
> 
> Well it was just an observation based on its similarity to my patch, but it
> would need a quick check with the debugger to step back up from the access
> to confirm if it were caused by the same issue (see the above issue #360 for
> detail).
> 
> As per my previous message no objection to the patch, but if it does have
> the same underlying cause then it could explain why this has been a tricky
> problem to solve (as in general the memory API "just works") and also
> provide another test case for a potential fix.
> 
> > > > +    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 = {
> 
> 
> ATB,
> 
> Mark.

Yea look at e.g. discussion around
https://lore.kernel.org/all/20211108130934.59B48748F52%40zero.eik.bme.hu/t.mbox.gz

-- 
MST

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

[BALATON Zoltan]

On Thu, 8 Jun 2023, BALATON Zoltan wrote:
> On Thu, 8 Jun 2023, Michael S. Tsirkin wrote:
>> On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
>>> On 07/06/2023 21:01, BALATON Zoltan 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>
>>>> ---
>>>>   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;
>>> 
>>> This shift here...
>>>
>>>>   }
>>>>   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);
>>>> +    }
>>> 
>>> and this shift here look similar to my workaround in 
>>> https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
>>> which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
>>> 
>>> Whilst there is no imminent fix for the above issue, it may be worth a few
>>> mins to determine if this is the same issue and if so document it with
>>> comments accordingly as I did so that the workaround can be removed at a
>>> later date.
>> 
>> So I will add
>> this triggers a but in memory core,
>> (see
>> https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
>> 
>> ?
>
> Apart from the typo but -> bug I'm not sure this is related to that issue but 
> in any case this does not trigger but works around some possible bug so maybe 
> "This work around may be related to issue URL" or something like that maybe? 
> I'm also not sure what comment to add where so I'd appreciate if you can 
> handle this on merging.

Ping? Is this queued somewhere or will it be merged? Maybe Adding Buglink 
tag to commit message could be sufficient or just mentioning the link in 
the commit message. I'm still not sure what the best way to do that so I 
hope you could take care of that.

Regards,
BALATON Zoltan

>>>> +    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 = {
>>> 
>>> 
>>> ATB,
>>> 
>>> Mark.
>> 
>> 
>
>

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

[Michael S. Tsirkin]

On Tue, Jun 20, 2023 at 02:50:55AM +0200, BALATON Zoltan wrote:
> On Thu, 8 Jun 2023, BALATON Zoltan wrote:
> > On Thu, 8 Jun 2023, Michael S. Tsirkin wrote:
> > > On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
> > > > On 07/06/2023 21:01, BALATON Zoltan 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>
> > > > > ---
> > > > >   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;
> > > > 
> > > > This shift here...
> > > > 
> > > > >   }
> > > > >   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);
> > > > > +    }
> > > > 
> > > > and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
> > > > which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
> > > > 
> > > > Whilst there is no imminent fix for the above issue, it may be worth a few
> > > > mins to determine if this is the same issue and if so document it with
> > > > comments accordingly as I did so that the workaround can be removed at a
> > > > later date.
> > > 
> > > So I will add
> > > this triggers a but in memory core,
> > > (see
> > > https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
> > > 
> > > ?
> > 
> > Apart from the typo but -> bug I'm not sure this is related to that
> > issue but in any case this does not trigger but works around some
> > possible bug so maybe "This work around may be related to issue URL" or
> > something like that maybe? I'm also not sure what comment to add where
> > so I'd appreciate if you can handle this on merging.
> 
> Ping? Is this queued somewhere or will it be merged? Maybe Adding Buglink
> tag to commit message could be sufficient or just mentioning the link in the
> commit message. I'm still not sure what the best way to do that so I hope
> you could take care of that.
> 
> Regards,
> BALATON Zoltan

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/360

?


> > > > > +    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 = {
> > > > 
> > > > 
> > > > ATB,
> > > > 
> > > > Mark.
> > > 
> > > 
> > 
> > 

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

[Michael S. Tsirkin]

On Thu, Jun 08, 2023 at 05:20:37PM +0200, BALATON Zoltan wrote:
> On Thu, 8 Jun 2023, Michael S. Tsirkin wrote:
> > On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
> > > On 07/06/2023 21:01, BALATON Zoltan 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>
> > > > ---
> > > >   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;
> > > 
> > > This shift here...
> > > 
> > > >   }
> > > >   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);
> > > > +    }
> > > 
> > > and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
> > > which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
> > > 
> > > Whilst there is no imminent fix for the above issue, it may be worth a few
> > > mins to determine if this is the same issue and if so document it with
> > > comments accordingly as I did so that the workaround can be removed at a
> > > later date.
> > 
> > So I will add
> > this triggers a but in memory core,
> > (see
> > https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
> > 
> > ?
> 
> Apart from the typo but -> bug I'm not sure this is related to that issue
> but in any case this does not trigger but works around some possible bug so
> maybe "This work around may be related to issue URL" or something like that
> maybe? I'm also not sure what comment to add where so I'd appreciate if you
> can handle this on merging.
> 
> Regards,
> BALATON Zoltan

I'll merge, just got back from travel.


> > > > +    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 = {
> > > 
> > > 
> > > ATB,
> > > 
> > > Mark.
> > 
> > 

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

[BALATON Zoltan]

On Tue, 20 Jun 2023, Michael S. Tsirkin wrote:
> On Tue, Jun 20, 2023 at 02:50:55AM +0200, BALATON Zoltan wrote:
>> On Thu, 8 Jun 2023, BALATON Zoltan wrote:
>>> On Thu, 8 Jun 2023, Michael S. Tsirkin wrote:
>>>> On Thu, Jun 08, 2023 at 12:37:08PM +0100, Mark Cave-Ayland wrote:
>>>>> On 07/06/2023 21:01, BALATON Zoltan 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>
>>>>>> ---
>>>>>>   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;
>>>>>
>>>>> This shift here...
>>>>>
>>>>>>   }
>>>>>>   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);
>>>>>> +    }
>>>>>
>>>>> and this shift here look similar to my workaround in https://patchew.org/QEMU/20230524211104.686087-1-mark.cave-ayland@ilande.co.uk/20230524211104.686087-31-mark.cave-ayland@ilande.co.uk/
>>>>> which is a symptom of https://gitlab.com/qemu-project/qemu/-/issues/360.
>>>>>
>>>>> Whilst there is no imminent fix for the above issue, it may be worth a few
>>>>> mins to determine if this is the same issue and if so document it with
>>>>> comments accordingly as I did so that the workaround can be removed at a
>>>>> later date.
>>>>
>>>> So I will add
>>>> this triggers a but in memory core,
>>>> (see
>>>> https://gitlab.com/qemu-project/qemu/-/issues/360 for more detail)
>>>>
>>>> ?
>>>
>>> Apart from the typo but -> bug I'm not sure this is related to that
>>> issue but in any case this does not trigger but works around some
>>> possible bug so maybe "This work around may be related to issue URL" or
>>> something like that maybe? I'm also not sure what comment to add where
>>> so I'd appreciate if you can handle this on merging.
>>
>> Ping? Is this queued somewhere or will it be merged? Maybe Adding Buglink
>> tag to commit message could be sufficient or just mentioning the link in the
>> commit message. I'm still not sure what the best way to do that so I hope
>> you could take care of that.
>>
>> Regards,
>> BALATON Zoltan
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/360
>
> ?

Fixes means it would close the ticket but this patch does not fix that 
issue just may be related to it so that's why Buglink may be more 
appropriate but I'm not sure either. So maybe just add a sentence to the 
commit message saying:

"This may be related to URL.."

Regards,
BALATON Zoltan

>
>>>>>> +    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 = {
>>>>>
>>>>>
>>>>> ATB,
>>>>>
>>>>> Mark.
>>>>
>>>>
>>>
>>>
>
>