Re: [PATCH] Allow acpi-tmr size=2

[Simon John]

I don't profess to understand most of this, I am just a user who found 
something didn't work and tracked down the cause with help from the 
people on the bugtracker.

the min=1 and max=4 was chosen as it seems to be set that way in most 
other places in the source, and 2 fits in that range.

so as macos seems to require 2 bytes but spec says 4 (32 bits) would it 
be better to set min=2 max=4, given that the original revert seems to be 
a security fix?

this works equally well:

static const MemoryRegionOps acpi_pm_tmr_ops = {
    .read = acpi_pm_tmr_read,
    .write = acpi_pm_tmr_write,
    .valid.min_access_size = 2,
    .valid.max_access_size = 4,
    .endianness = DEVICE_LITTLE_ENDIAN,
};

regards.



On 13/07/2020 12:14, Michael S. Tsirkin wrote:
On Mon, Jul 13, 2020 at 10:20:12AM +0300, Michael Tokarev wrote:
12.07.2020 15:00, Simon John wrote:
macos guests no longer boot after commit 
5d971f9e672507210e77d020d89e0e89165c8fc9

acpi-tmr needs 2 byte memory accesses, so breaks as that commit only allows 4 
bytes.

Fixes: 5d971f9e672507210e7 (memory: Revert "memory: accept mismatching sizes in 
memory_region_access_valid")
Buglink: https://bugs.launchpad.net/qemu/+bug/1886318

Actually this fixes 77d58b1e47c8d1c661f98f12b47ab519d3561488
Author: Gerd Hoffmann <kra...@redhat.com>
Date:   Thu Nov 22 12:12:30 2012 +0100
Subject: apci: switch timer to memory api
Signed-off-by: Gerd Hoffmann <kra...@redhat.com>

because this is the commit which put min_access_size = 4 in there
(5d971f9e672507210e7 is just a messenger, actual error were here
earlier but it went unnoticed).

While min_access_size=4 was most likely an error, I wonder why
we use 1 now, while the subject says it needs 2? What real min
size is here for ACPI PM timer?

/mjt


Well the ACPI spec 1.0b says

4.7.3.3 Power Management Timer (PM_TMR)

...

This register is accessed as 32 bits.

and this text is still there in 6.2.


So it's probably worth it to cite this in the commit log
and explain it's a spec violation.
I think it's better to be restrictive and only allow the
minimal variation from spec - in this case I guess this means 2 byte
reads.

In any case pls do include an explanation for why you picked
one over the other.


Signed-off-by: Simon John <g...@the-jedi.co.uk>
---
 hw/acpi/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/core.c b/hw/acpi/core.c
index f6d9ec4f13..05ff29b9d7 100644
--- a/hw/acpi/core.c
+++ b/hw/acpi/core.c
@@ -527,7 +527,7 @@ static void acpi_pm_tmr_write(void *opaque, hwaddr addr, 
uint64_t val,
 static const MemoryRegionOps acpi_pm_tmr_ops = {
     .read = acpi_pm_tmr_read,
     .write = acpi_pm_tmr_write,
-    .valid.min_access_size = 4,
+    .valid.min_access_size = 1,
     .valid.max_access_size = 4,
     .endianness = DEVICE_LITTLE_ENDIAN,
 };



--
Simon John