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.