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,
> > Â };
