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

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

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.
> > 
> >