Re: [PATCH] hw/acpi: Fix PM control register access

[Igor Mammedov <imammedo@redhat.com>]

On Sun, 28 May 2023 15:57:50 +0200 (CEST)
BALATON Zoltan <balaton@eik.bme.hu> 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. This fixes ACPI shutdown with pegasos2 firmware.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> This is replacing the previous attempt which changed enduanness to
> NATIVE_ENDIAN that was found to be wrong. I'm still not sure what's
> happening as these functions are called with addr = 1 and size = 2 but
> maybe the guest really does word access to addr 1 when wanting to
> write 1 byte. This fixes the problem and should not break anything
> else but please review.
> 
>  hw/acpi/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/acpi/core.c b/hw/acpi/core.c
> index 6da275c599..bbc599a252 100644
> --- a/hw/acpi/core.c
> +++ b/hw/acpi/core.c
> @@ -593,13 +593,13 @@ 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;
>  }
that looks fine

>  
>  static void acpi_pm_cnt_write(void *opaque, hwaddr addr, uint64_t val,
>                                unsigned width)
>  {
> -    acpi_pm1_cnt_write(opaque, val);
> +    acpi_pm1_cnt_write(opaque, val << addr * 8);
>  }
however, if this is 1 byte write at offset 1,
wouldn't this wipe out lower byte
(aka:
#define ACPI_BITMASK_SCI_ENABLE                 0x0001                           
#define ACPI_BITMASK_BUS_MASTER_RLD             0x0002                           
#define ACPI_BITMASK_GLOBAL_LOCK_RELEASE        0x0004
)? 


>  
>  static const MemoryRegionOps acpi_pm_cnt_ops = {