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

[Michael S. Tsirkin]

On Tue, Jul 14, 2020 at 12:55:44PM +0200, Philippe Mathieu-Daudé wrote:
> +Peter/Paolo
> 
> On 7/13/20 1:14 PM, 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.
> 
> Now reading this thread, I guess understand this register is
> accessed via the I/O address space, where 8/16/32-bit accesses
> are always valid if the CPU supports an I/O bus.

They are valid from bus POV, but not from the device POV.


> We have 3 different devices providing this register:
> - ICH9
> - PIIX4 (abused in PIIX3)
> - VT82C686
> 
> All are PCI devices, exposing this register via an ISA function.
> 
> The ISA MemoryRegion should allow 8/16/32-bit accesses.
> 
> For these devices we use:
> 
> MemoryRegion *pci_address_space_io(PCIDevice *dev)
> {
>     return pci_get_bus(dev)->address_space_io;
> }
> 
> Which comes from:
> 
> static void pci_root_bus_init(PCIBus *bus, DeviceState *parent,
>                               MemoryRegion *address_space_mem,
>                               MemoryRegion *address_space_io,
>                               uint8_t devfn_min)
> {
>     ...
>     bus->address_space_mem = address_space_mem;
>     bus->address_space_io = address_space_io;
>     ...
> 
> 
> In i440fx_init():
> 
>     b = pci_root_bus_new(dev, NULL, pci_address_space,
>                          address_space_io, 0, TYPE_PCI_BUS);
> 
> q35_host_initfn() uses get_system_io() from pc_q35_init().
> 
> If the guest did a 16-bit read, it should work ...:
> 
> uint16_t cpu_inw(uint32_t addr)
> {
>     uint8_t buf[2];
>     uint16_t val;
> 
>     address_space_read(&address_space_io, addr, MEMTXATTRS_UNSPECIFIED,
> buf, 2);
>     val = lduw_p(buf);
>     trace_cpu_in(addr, 'w', val);
>     return val;
> }
> 
> ... but it is indeed prevented by min_access_size=4.
> 
> Maybe we should have the ISA MemoryRegion accepts min_access_size=1
> and adjust the access sizes.

What started all this is that device code isn't really prepared
to handle such accesses.


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