[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] hw/acpi: Fix PM control register access
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH v2] hw/acpi: Fix PM control register access |
Date: |
Tue, 20 Jun 2023 09:27:26 -0400 |
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.
> >
> >