|
From: | Simon John |
Subject: | Re: [PATCH] Allow acpi-tmr size=2 |
Date: | Mon, 13 Jul 2020 12:46:00 +0100 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 |
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/1886318Actually 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? /mjtWell 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
[Prev in Thread] | Current Thread | [Next in Thread] |