| Message ID | 5f12377f-b640-c4c5-1bcd-858c622c6c31@the-jedi.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | Allow acpi-tmr size=2 | expand |
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 > 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, > };
13.07.2020 10:20, Michael Tokarev пишет: > 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? Actually it is more twisted than that. We can't just change the size, we must update the corresponding code too. static uint64_t acpi_pm_tmr_read(void *opaque, hwaddr addr, unsigned width) { return acpi_pm_tmr_get(opaque); } note the actual read function does not even know neither the requested address nor the requested width, it assumes the min/max constraints are enforced and the read goes to all 4 bytes. If this pm timer can be read byte-by-byte, we should return the right byte of the value, not always the whole value. /mjt
On Mon, Jul 13, 2020 at 10:43:19AM +0300, Michael Tokarev wrote: > 13.07.2020 10:20, Michael Tokarev пишет: > > 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? > > Actually it is more twisted than that. We can't just change the size, > we must update the corresponding code too. > > > static uint64_t acpi_pm_tmr_read(void *opaque, hwaddr addr, unsigned width) > { > return acpi_pm_tmr_get(opaque); > } > > note the actual read function does not even know neither the requested > address nor the requested width, it assumes the min/max constraints > are enforced and the read goes to all 4 bytes. If this pm timer can > be read byte-by-byte, we should return the right byte of the value, > not always the whole value. > > /mjt I think that specifying .impl.min_access_size is a way to do that easily without major code changes.
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, > > Â };
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.
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,
>>> Â };
>
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
13.07.2020 15:17, 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? It's not about the security fix, it's about the piece in qemu code which behaved wrongly for several years, which finally started to actually work. >> 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: What this call will receive on a real HW? returning the same 4 bytes even when asked for 2 smells wrong, no? > .impl.min_access_size = 4, What does it mean? :) /mjt
On Mon, Jul 13, 2020 at 05:16:56PM +0300, Michael Tokarev wrote: > 13.07.2020 15:17, 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? > > It's not about the security fix, it's about the piece in qemu code which > behaved wrongly for several years, which finally started to actually work. > > >> 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: > > What this call will receive on a real HW? returning the same 4 bytes > even when asked for 2 smells wrong, no? > > > .impl.min_access_size = 4, > > What does it mean? :) > > /mjt This will allow you to return a 4 byte value and will shift it accordingly. See: docs/devel/memory.rst : - .impl.min_access_size, .impl.max_access_size define the access sizes (in bytes) supported by the *implementation*; other access sizes will be emulated using the ones available. For example a 4-byte write will be emulated using four 1-byte writes, if .impl.max_access_size = 1.
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 And looking at that: - case 0x08: - val = acpi_pm_tmr_get(&s->ar); - break; default: val = 0; break; So what was going on is reads from 0x10 would just give you 0. It looks like Mac OSX does not care much about the value it gets, as long as it does not crash :). > > 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, > > Â };
+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. 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 <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, >>>  }; > >
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, > >>>  }; > > > >
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,
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 Signed-off-by: Simon John <git@the-jedi.co.uk> --- hw/acpi/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) };