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

["Michael S. Tsirkin" <mst@redhat.com>]

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.