On Mon, Jul 13, 2020 at 12:46:00PM +0100, Simon John wrote: > 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. >
Sounds good. And how about also adding: .impl.min_access_size = 4, ? > > 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