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