1. Patchew
  2. QEMU
  3. [PULL 00/53] virtio,pc,pci: fixes, features, cleanups
  4. View patch

[PULL 30/53] hw/acpi: Fix PM control register access

Michael S. Tsirkin posted 53 patches 2 years, 7 months ago
Maintainers: "Gonglei (Arei)" <arei.gonglei@huawei.com>, Zhenwei Pi <pizhenwei@bytedance.com>, Coiby Xu <Coiby.Xu@gmail.com>, Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Raphael Norwitz <raphael.norwitz@nutanix.com>, Igor Mammedov <imammedo@redhat.com>, Ani Sinha <anisinha@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Jonathan Cameron <jonathan.cameron@huawei.com>, Fan Ni <fan.ni@samsung.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Gerd Hoffmann <kraxel@redhat.com>, Peter Xu <peterx@redhat.com>, Jason Wang <jasowang@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Cornelia Huck <cohuck@redhat.com>, Halil Pasic <pasic@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, Thomas Huth <thuth@redhat.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Fam Zheng <fam@euphon.net>, "Eugenio Pérez" <eperezma@redhat.com>, Eric Auger <eric.auger@redhat.com>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>
[PULL 30/53] hw/acpi: Fix PM control register access
Posted by Michael S. Tsirkin 2 years, 7 months ago 
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>
---
 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,
-- 
MST
Re: [PULL 30/53] hw/acpi: Fix PM control register access
Posted by Igor Mammedov 2 years, 7 months ago 
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,
Re: [PULL 30/53] hw/acpi: Fix PM control register access
Posted by Michael S. Tsirkin 2 years, 7 months ago 
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,

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.