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

[Michael S. Tsirkin]

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 <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
> > 
> > 
> > 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 <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,
> > > >  };
> > 
> 
> 
> -- 
> Simon John