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

[Paolo Bonzini]

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