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