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.