Re: [PATCH] hw/acpi: Set memory regions to native endian as a work around

[Paolo Bonzini <pbonzini@redhat.com>]

On 2/21/23 00:25, BALATON Zoltan wrote:
>> I think fundamentally you need to check for the condition
>> Size < mr->ops->impl.min_access_size in memory_region_dispatch_write
>> and then make a read, combine the result with
>> the value and make a write.
> 
> I neither know that part nor feel confident enough breaking such low 
> level stuff so I think setting the affected regions NATIVE_ENDIAN for 
> now until somebody takes care of this is safer and not likely to break 
> anyting (or if it does, much less widely and I'm more likely to be able 
> to fix that than your proposed changes). So I'd rather let you do that 
> but I'd like this fixed one way or another at last.

Sorry about not replying.

The case of impl.min_access_size < valid.min_access_size is not
supported in the memory core.  Until that is done, the correct fix is to
fix acpi_pm_evt_ops to have impl.min_access_size == 1, something like
this:

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index 6da275c599c6..96eb88fa7e27 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -429,20 +429,35 @@ void acpi_pm1_evt_reset(ACPIREGS *ar)
  static uint64_t acpi_pm_evt_read(void *opaque, hwaddr addr, unsigned width)
  {
      ACPIREGS *ar = opaque;
+    uint16_t val;
      switch (addr) {
      case 0:
-        return acpi_pm1_evt_get_sts(ar);
+        val = acpi_pm1_evt_get_sts(ar);
      case 2:
-        return ar->pm1.evt.en;
+        val = ar->pm1.evt.en;
      default:
          return 0;
      }
+
+    if (width == 1) {
+        int bitofs = (addr & 1) * 8;
+        val >>= bitofs;
+    }
+    return val;
  }
  
  static void acpi_pm_evt_write(void *opaque, hwaddr addr, uint64_t val,
                                unsigned width)
  {
      ACPIREGS *ar = opaque;
+    if (width == 1) {
+        int bitofs = (addr & 1) * 8;
+        uint16_t old_val = acpi_pm_evt_read(ar, addr, val & ~1);
+        uint16_t mask = 0xFF << bitofs;
+        val = (old_val & ~mask) | (val << bitofs);
+        addr &= ~1;
+    }
+
      switch (addr) {
      case 0:
          acpi_pm1_evt_write_sts(ar, val);
@@ -458,7 +473,7 @@ static void acpi_pm_evt_write(void *opaque, hwaddr addr, uint64_t val,
  static const MemoryRegionOps acpi_pm_evt_ops = {
      .read = acpi_pm_evt_read,
      .write = acpi_pm_evt_write,
-    .impl.min_access_size = 2,
+    .impl.min_access_size = 1,
      .valid.min_access_size = 1,
      .valid.max_access_size = 2,
      .endianness = DEVICE_LITTLE_ENDIAN,


This assumes that the bus is little-endian, i.e. reading the byte at PM_EVT returns
bits 0..7 and reading the byte at PM_EVT+1 returns bits 8..15.

If this is incorrect, endianness needs to be changed as well.

Paolo