On Tue, Jun 20, 2023 at 02:50:55AM +0200, BALATON Zoltan wrote: > 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 <bala...@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-ayl...@ilande.co.uk/20230524211104.686087-31-mark.cave-ayl...@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
Fixes: https://gitlab.com/qemu-project/qemu/-/issues/360 ? > > > > > + 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. > > > > > > > > > >