[PATCH] Allow acpi-tmr size=2

[PATCH] Allow acpi-tmr size=2

[Simon John]

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(-)

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

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

[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?

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

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

[Michael Tokarev]

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

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

[Michael S. Tsirkin]

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.

-- 
MST

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

[Michael S. Tsirkin]

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

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

[Simon John]

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


-- 
Simon John

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

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

[Michael Tokarev]

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

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

[Michael S. Tsirkin]

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.


-- 
MST

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

[Philippe Mathieu-Daudé]

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

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

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

[Michael S. Tsirkin]

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

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

[Simon John]

On Mon, 13 Jul 2020 08:17:41 -0400, Michael S. Tsirkin wrote:

> Sounds good. And how about also adding:
> 
>       .impl.min_access_size = 4,
> 
> ?

Yes, this works too - what does that do?

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,
     .impl.min_access_size = 4,
     .impl.max_access_size = 4,
     .endianness = DEVICE_LITTLE_ENDIAN,
};

Regards.

-- 
Simon John