Message ID | 3d563fc68732179b86eff2b87f2d7dbef150315f.1687782442.git.mst@redhat.com |
---|---|
State | New |
Headers | show |
Series | [PULL,01/53] bswap: Add the ability to store to an unaligned 24 bit field | expand |
On Mon, 26 Jun 2023 08:29:19 -0400 "Michael S. Tsirkin" <mst@redhat.com> wrote: > From: BALATON Zoltan <balaton@eik.bme.hu> > > 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 <balaton@eik.bme.hu> > Message-Id: <20230607200125.A9988746377@zero.eik.bme.hu> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Somewhere you lost mine Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > 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,
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" <mst@redhat.com> wrote: > > > From: BALATON Zoltan <balaton@eik.bme.hu> > > > > 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 <balaton@eik.bme.hu> > > Message-Id: <20230607200125.A9988746377@zero.eik.bme.hu> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Somewhere you lost mine > Reviewed-by: Igor Mammedov <imammedo@redhat.com> 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,
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,