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

[BALATON Zoltan <balaton@eik.bme.hu>]

On Tue, 6 Jun 2023, Igor Mammedov wrote:
> 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
> )?

Since this will either reset or power off the machine it probably does not 
really matter. But to fix that more we'd need to rewrite 
acpi_pm1_cnt_write() to replace acpi_pm_cnt_write() and handle that 
internally. Or is there another way to handle this? Does that worth the 
effort when the only known usage is in pegasos2 firmware when it calls 
power off?

Regards,
BALATON Zoltan