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

[Michael S. Tsirkin]

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 <kraxel@redhat.com>
> Date:   Thu Nov 22 12:12:30 2012 +0100
> Subject: apci: switch timer to memory api
> Signed-off-by: Gerd Hoffmann <kraxel@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

And looking at that:

-    case 0x08:
-        val = acpi_pm_tmr_get(&s->ar);
-        break;
     default:
         val = 0;
         break;

So what was going on is reads from 0x10 would just give you 0.
It looks like Mac OSX does not care much about the value it gets,
as long as it does not crash :).



> > Signed-off-by: Simon John <git@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,
> > Â };