On Mon, Jun 26, 2023 at 03:20:09PM +0200, Igor Mammedov wrote:
> On Mon, 26 Jun 2023 08:29:19 -0400
> "Michael S. Tsirkin" <[email protected]> wrote:
>
> > From: BALATON Zoltan <[email protected]>
> >
> > 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.
> >
> > The issue below is possibly related to the same memory core bug.
> >
> > Link: https://gitlab.com/qemu-project/qemu/-/issues/360
> > Signed-off-by: BALATON Zoltan <[email protected]>
> > Message-Id: <[email protected]>
> > Reviewed-by: Michael S. Tsirkin <[email protected]>
> > Signed-off-by: Michael S. Tsirkin <[email protected]>
>
> Somewhere you lost mine
> Reviewed-by: Igor Mammedov <[email protected]>
You are right, sorry. fixed now.
> > ---
> > hw/acpi/core.c | 56 +++++++++++++++++++++++++-------------------------
> > 1 file changed, 28 insertions(+), 28 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,8 +551,35 @@ void acpi_pm_tmr_reset(ACPIREGS *ar)
> > }
> >
> > /* ACPI PM1aCNT */
> > -static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t val)
> > +void acpi_pm1_cnt_update(ACPIREGS *ar,
> > + bool sci_enable, bool sci_disable)
> > {
> > + /* ACPI specs 3.0, 4.7.2.5 */
> > + if (ar->pm1.cnt.acpi_only) {
> > + return;
> > + }
> > +
> > + if (sci_enable) {
> > + ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
> > + } else if (sci_disable) {
> > + ar->pm1.cnt.cnt &= ~ACPI_BITMASK_SCI_ENABLE;
> > + }
> > +}
> > +
> > +static uint64_t acpi_pm_cnt_read(void *opaque, hwaddr addr, unsigned width)
> > +{
> > + ACPIREGS *ar = opaque;
> > + return ar->pm1.cnt.cnt >> addr * 8;
> > +}
> > +
> > +static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
> > + unsigned width)
> > +{
> > + ACPIREGS *ar = opaque;
> > +
> > + if (addr == 1) {
> > + val = val << 8 | (ar->pm1.cnt.cnt & 0xff);
> > + }
> > ar->pm1.cnt.cnt = val & ~(ACPI_BITMASK_SLEEP_ENABLE);
> >
> > if (val & ACPI_BITMASK_SLEEP_ENABLE) {
> > @@ -575,33 +602,6 @@ static void acpi_pm1_cnt_write(ACPIREGS *ar, uint16_t
> > val)
> > }
> > }
> >
> > -void acpi_pm1_cnt_update(ACPIREGS *ar,
> > - bool sci_enable, bool sci_disable)
> > -{
> > - /* ACPI specs 3.0, 4.7.2.5 */
> > - if (ar->pm1.cnt.acpi_only) {
> > - return;
> > - }
> > -
> > - if (sci_enable) {
> > - ar->pm1.cnt.cnt |= ACPI_BITMASK_SCI_ENABLE;
> > - } else if (sci_disable) {
> > - ar->pm1.cnt.cnt &= ~ACPI_BITMASK_SCI_ENABLE;
> > - }
> > -}
> > -
> > -static uint64_t acpi_pm_cnt_read(void *opaque, hwaddr addr, unsigned width)
> > -{
> > - ACPIREGS *ar = opaque;
> > - return ar->pm1.cnt.cnt;
> > -}
> > -
> > -static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
> > - unsigned width)
> > -{
> > - acpi_pm1_cnt_write(opaque, val);
> > -}
> > -
> > static const MemoryRegionOps acpi_pm_cnt_ops = {
> > .read = acpi_pm_cnt_read,
> > .write = acpi_pm_cnt_write,
