Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 3/2/23 09:32, Christian Ehrhardt wrote: > good ~instant > [08:14:37.267336194] Select Item: 0xE > [08:14:37.268346995] [Bds]RegisterKeyNotify: 000C/ 8000/00 Success > > bad ~8s > [08:15:43.561054490] Select Item: 0xE > [08:15:51.291039364] [Bds]RegisterKeyNotify: 000C/ 8000/00 Success Yes, this is consistent with my hypothesis. PlatformBootManagerBeforeConsole() GetFrontPageTimeoutFromQemu() QemuFwCfgSelectItem (QemuFwCfgItemBootMenu) // "Select Item: 0xE" gRT->SetVariable() PlatformRegisterOptionsAndKeys() EfiBootManagerAddKeyOptionVariable() gRT->SetVariable() BmProcessKeyOption() BmRegisterHotkeyNotify() // "[Bds]RegisterKeyNotify: 000C/ 8000/00 Success" IOW, there are at least two gRT->SetVariable() calls in OVMF (with EFI_VARIABLE_NON_VOLATILE attribute) between the two adjacent log lines you quoted. The other functions listed in the call tree may contain further gRT->SetVariable() calls. Laszlo
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Wed, Mar 1, 2023 at 9:04 AM Laszlo Ersek wrote: > > Hello Christian, > > On 3/1/23 08:17, Christian Ehrhardt wrote: > > On Thu, Jan 5, 2023 at 8:14 AM Laszlo Ersek wrote: > >> > >> On 1/4/23 13:35, Michael S. Tsirkin wrote: > >>> On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote: > The modern ACPI CPU hotplug interface was introduced in the following > series (aa1dd39ca307..679dd1a957df), released in v2.7.0: > > 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol > 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property > 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method > 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook > 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug > interface > 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug > interface > 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling > 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine > type > > > ... > >> > >> The solution to the riddle > > > > Hi, > > just to add to this nicely convoluted case an FYI to everyone involved > > back then, > > the fix seems to have caused a regression [1] in - as far as I've > > found - an edge case. > > > > [1]: https://gitlab.com/qemu-project/qemu/-/issues/1520 > > After reading the gitlab case, here's my theory on it: > > - Without the patch applied, the CPU hotplug register block in QEMU is > broken. Effectively, it has *always* been broken; to put it differently, > you have most likely *never* seen a QEMU in which the CPU hotplug > register block was not broken. The reason is that the only QEMU release > without the breakage (as far as a guest could see it!) was v5.0.0, but > it got exposed to the guest as early as v5.1.0 (IOW, in the 5.* series, > the first stable release already exposed the issue), and the symptom has > existed since (up to and including 7.2). > > - With the register block broken, OVMF's multiprocessing is broken, and > the random chaos just happens to play out in a way that makes OVMF think > it's running on a uniprocessor system. > > - With the register block *fixed* (commit dab30fbe applied), OVMF > actually boots up your VCPUs. With MT-TCG, this translates to as many > host-side VCPU threads running in your QEMU process as you have VCPUs. > > - Furthermore, if your OVMF build includes the SMM driver stack, then > each UEFI variable update will require all VCPUs to enter SMM. All VCPUs > entering SMM is a "thundering herd" event, so it seriously spins up all > your host-side threads. (I assume the SMM-enabled binaries are what you > refer to as "signed OVMF cases" in the gitlab ticket.) > > - If you overcommit the VCPUs (#vcpus > #pcpus), then your host-side > threads will be competing for PCPUs. On s390x, there is apparently some > bottleneck in QEMU's locking or in the host kernel or wherever else that > penalizes (#threads > #pcpus) heavily, while on other host arches, the > penalty is (apparently) not as severe. > > So, the QEMU fix actually "only exposes" the high penalty of the MT-TCG > VCPU thread overcommit that appears characteristic of s390x hosts. > You've not seen this symptom before because, regardless of how many > VCPUs you've specified in the past, OVMF has never actually attempted to > bring those up, due to the hotplug regblock breakage "masking" the > actual VCPU counts (the present-at-boot VCPU count and the possible max > VCPU count). Thank you for the detailed thoughts - if we can confirm this we can close the case as "it is odd that there is so much penalty, but => Won't Fix / Works as Intended" > Here's a test you could try: go back to QEMU v5.0.0 *precisely*, and try > to reproduce the symptom. I expect that it should reproduce. v5.0.0 - 1 host cpu vs 2 vcpu - 58.47s v5.0.0 - 1 host cpu vs 1 vcpu - 5.33s v5.0.0 - 2 host cpu vs 2 vcpu - 5.27s v5.1.0 - 1 host cpu vs 2 vcpu - 7.18s v5.1.0 - 1 host cpu vs 1 vcpu - 5.22s v5.1.0 - 2 host cpu vs 2 vcpu - 5.40s Yes, v5.0.0 behaves exactly like the recent master branch does since your fix. And v5.1.0 does no more, just as you predicted > Here's another test you can try: with latest QEMU, boot an x86 Linux > guest, but using SeaBIOS, not OVMF, on your s390x host. Then, in the > Linux guest, run as many busy loops (e.g. in the shell) as there are > VCPUs. Compare the behavior between #vcpus = #pcpus vs. #vcpus > #pcpus. > The idea here is of course to show that the impact of overcommitting x86 > VCPUs on s390x is not specific to OVMF. Note that I don't *fully* expect > this test to confirm the expectation, because the guest workload will be > very different: in the Linux guest case, your VCPUs will not be > attempting to enter SMM *or* to access pflash, so the paths exercised in > QEMU will be very different. But, the test may still be worth a try.
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
Hello Christian, On 3/1/23 08:17, Christian Ehrhardt wrote: > On Thu, Jan 5, 2023 at 8:14 AM Laszlo Ersek wrote: >> >> On 1/4/23 13:35, Michael S. Tsirkin wrote: >>> On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote: The modern ACPI CPU hotplug interface was introduced in the following series (aa1dd39ca307..679dd1a957df), released in v2.7.0: 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug interface 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug interface 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type > ... >> >> The solution to the riddle > > Hi, > just to add to this nicely convoluted case an FYI to everyone involved > back then, > the fix seems to have caused a regression [1] in - as far as I've > found - an edge case. > > [1]: https://gitlab.com/qemu-project/qemu/-/issues/1520 After reading the gitlab case, here's my theory on it: - Without the patch applied, the CPU hotplug register block in QEMU is broken. Effectively, it has *always* been broken; to put it differently, you have most likely *never* seen a QEMU in which the CPU hotplug register block was not broken. The reason is that the only QEMU release without the breakage (as far as a guest could see it!) was v5.0.0, but it got exposed to the guest as early as v5.1.0 (IOW, in the 5.* series, the first stable release already exposed the issue), and the symptom has existed since (up to and including 7.2). - With the register block broken, OVMF's multiprocessing is broken, and the random chaos just happens to play out in a way that makes OVMF think it's running on a uniprocessor system. - With the register block *fixed* (commit dab30fbe applied), OVMF actually boots up your VCPUs. With MT-TCG, this translates to as many host-side VCPU threads running in your QEMU process as you have VCPUs. - Furthermore, if your OVMF build includes the SMM driver stack, then each UEFI variable update will require all VCPUs to enter SMM. All VCPUs entering SMM is a "thundering herd" event, so it seriously spins up all your host-side threads. (I assume the SMM-enabled binaries are what you refer to as "signed OVMF cases" in the gitlab ticket.) - If you overcommit the VCPUs (#vcpus > #pcpus), then your host-side threads will be competing for PCPUs. On s390x, there is apparently some bottleneck in QEMU's locking or in the host kernel or wherever else that penalizes (#threads > #pcpus) heavily, while on other host arches, the penalty is (apparently) not as severe. So, the QEMU fix actually "only exposes" the high penalty of the MT-TCG VCPU thread overcommit that appears characteristic of s390x hosts. You've not seen this symptom before because, regardless of how many VCPUs you've specified in the past, OVMF has never actually attempted to bring those up, due to the hotplug regblock breakage "masking" the actual VCPU counts (the present-at-boot VCPU count and the possible max VCPU count). Here's a test you could try: go back to QEMU v5.0.0 *precisely*, and try to reproduce the symptom. I expect that it should reproduce. Here's another test you can try: with latest QEMU, boot an x86 Linux guest, but using SeaBIOS, not OVMF, on your s390x host. Then, in the Linux guest, run as many busy loops (e.g. in the shell) as there are VCPUs. Compare the behavior between #vcpus = #pcpus vs. #vcpus > #pcpus. The idea here is of course to show that the impact of overcommitting x86 VCPUs on s390x is not specific to OVMF. Note that I don't *fully* expect this test to confirm the expectation, because the guest workload will be very different: in the Linux guest case, your VCPUs will not be attempting to enter SMM *or* to access pflash, so the paths exercised in QEMU will be very different. But, the test may still be worth a try. Yet another test (or more like, information gathering): re-run the problematic case, while printing the OVMF debug log (the x86 debug console) to stdout, and visually determine at what part(s) the slowdown hits. (I guess you can also feed the debug console log through some timestamping utility like "logger".) I suspect it's going to be those log sections that relate to SMM entry -- initial SMBASE relocation, and then whenever UEFI variables are modified. Preliminary advice: don't overcommit VCPUs in the setup at hand, or else please increase the timeout. :) In edk2, a way to mitigate said "thundering herd" problem *supposedly* exists (using unicast SMIs rather than broadcast ones), but that configuration of the core SMM components in
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Thu, Jan 5, 2023 at 8:14 AM Laszlo Ersek wrote: > > On 1/4/23 13:35, Michael S. Tsirkin wrote: > > On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote: > >> The modern ACPI CPU hotplug interface was introduced in the following > >> series (aa1dd39ca307..679dd1a957df), released in v2.7.0: > >> > >> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol > >> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property > >> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method > >> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook > >> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug > >> interface > >> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug > >> interface > >> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling > >> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type > >> ... > > The solution to the riddle Hi, just to add to this nicely convoluted case an FYI to everyone involved back then, the fix seems to have caused a regression [1] in - as far as I've found - an edge case. [1]: https://gitlab.com/qemu-project/qemu/-/issues/1520 ... > Laszlo > > -- Christian Ehrhardt Senior Staff Engineer, Ubuntu Server Canonical Ltd
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
sigh, I've been at it for almost 12 hours today, and am in a rush to
finish up -- so I forgot to pass "-v2" to git-format patch.
So this is actually "v2" (as the Notes section shows); I'm sorry about
the mistake in the subject-line versioning.
Michael, please *do* queue this patch.
Thanks
Laszlo
On 1/5/23 17:18, Laszlo Ersek wrote:
> The modern ACPI CPU hotplug interface was introduced in the following
> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>
> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
> interface
> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
> interface
> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>
> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
> accesses for the hotplug register block. Patch#1 preserved the same
> restriction for the legacy register block, but:
>
> - it specified DWORD accesses for some of the modern registers,
>
> - in particular, the switch from the legacy block to the modern block
> would require a DWORD write to the *legacy* block.
>
> The latter functionality was then implemented in cpu_status_write()
> [hw/acpi/cpu_hotplug.c], in patch#8.
>
> Unfortunately, all DWORD accesses depended on a dormant bug: the one
> introduced in earlier commit a014ed07bd5a ("memory: accept mismatching
> sizes in memory_region_access_valid", 2013-05-29); first released in
> v1.6.0. Due to commit a014ed07bd5a, the DWORD accesses to the *legacy*
> CPU hotplug register block would work in spite of the above series *not*
> relaxing "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>
>> static const MemoryRegionOps AcpiCpuHotplug_ops = {
>> .read = cpu_status_read,
>> .write = cpu_status_write,
>> .endianness = DEVICE_LITTLE_ENDIAN,
>> .valid = {
>> .min_access_size = 1,
>> .max_access_size = 1,
>> },
>> };
>
> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
> interface (including the documentation) was extended with another DWORD
> *read* access, namely to the "Command data 2" register, which would be
> important for the guest to confirm whether it managed to switch the
> register block from legacy to modern.
>
> This functionality too silently depended on the bug from commit
> a014ed07bd5a.
>
> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
> the v2.7.0 series quoted at the top -- namely the fact that
> "valid.max_access_size = 1" didn't match what the guest was supposed to
> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>
> The symptom is that the "modern interface negotiation protocol"
> described in commit ae340aa3d256:
>
>> + Use following steps to detect and enable modern CPU hotplug interface:
>> +1. Store 0x0 to the 'CPU selector' register,
>> + attempting to switch to modern mode
>> +2. Store 0x0 to the 'CPU selector' register,
>> + to ensure valid selector value
>> +3. Store 0x0 to the 'Command field' register,
>> +4. Read the 'Command data 2' register.
>> + If read value is 0x0, the modern interface is enabled.
>> + Otherwise legacy or no CPU hotplug interface available
>
> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
> writes; so no switching happens. Step 3 (a single-byte write) is not
> lost, but it has no effect; see the condition in cpu_status_write() in
> patch#8. And step 4 *misleads* the guest into thinking that the switch
> worked: the DWORD read is lost again -- it returns zero to the guest
> without ever reaching the device model, so the guest never learns the
> switch didn't work.
>
> This means that guest behavior centered on the "Command data 2" register
> worked *only* in the v5.0.0 release; it got effectively regressed in
> v5.1.0.
>
> To make things *even more* complicated, the breakage was (and remains, as
> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
> no difference with KVM acceleration -- the DWORD accesses still work,
> despite "valid.max_access_size = 1".
>
> As commit 5d971f9e6725 suggests, fix the problem by raising
>
[PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
The modern ACPI CPU hotplug interface was introduced in the following
series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
interface
6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
interface
7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
accesses for the hotplug register block. Patch#1 preserved the same
restriction for the legacy register block, but:
- it specified DWORD accesses for some of the modern registers,
- in particular, the switch from the legacy block to the modern block
would require a DWORD write to the *legacy* block.
The latter functionality was then implemented in cpu_status_write()
[hw/acpi/cpu_hotplug.c], in patch#8.
Unfortunately, all DWORD accesses depended on a dormant bug: the one
introduced in earlier commit a014ed07bd5a ("memory: accept mismatching
sizes in memory_region_access_valid", 2013-05-29); first released in
v1.6.0. Due to commit a014ed07bd5a, the DWORD accesses to the *legacy*
CPU hotplug register block would work in spite of the above series *not*
relaxing "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
> static const MemoryRegionOps AcpiCpuHotplug_ops = {
> .read = cpu_status_read,
> .write = cpu_status_write,
> .endianness = DEVICE_LITTLE_ENDIAN,
> .valid = {
> .min_access_size = 1,
> .max_access_size = 1,
> },
> };
Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
interface (including the documentation) was extended with another DWORD
*read* access, namely to the "Command data 2" register, which would be
important for the guest to confirm whether it managed to switch the
register block from legacy to modern.
This functionality too silently depended on the bug from commit
a014ed07bd5a.
In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
the bug from commit a014ed07bd5a was fixed (the commit was reverted).
That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
the v2.7.0 series quoted at the top -- namely the fact that
"valid.max_access_size = 1" didn't match what the guest was supposed to
do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
The symptom is that the "modern interface negotiation protocol"
described in commit ae340aa3d256:
> + Use following steps to detect and enable modern CPU hotplug interface:
> +1. Store 0x0 to the 'CPU selector' register,
> + attempting to switch to modern mode
> +2. Store 0x0 to the 'CPU selector' register,
> + to ensure valid selector value
> +3. Store 0x0 to the 'Command field' register,
> +4. Read the 'Command data 2' register.
> + If read value is 0x0, the modern interface is enabled.
> + Otherwise legacy or no CPU hotplug interface available
falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
writes; so no switching happens. Step 3 (a single-byte write) is not
lost, but it has no effect; see the condition in cpu_status_write() in
patch#8. And step 4 *misleads* the guest into thinking that the switch
worked: the DWORD read is lost again -- it returns zero to the guest
without ever reaching the device model, so the guest never learns the
switch didn't work.
This means that guest behavior centered on the "Command data 2" register
worked *only* in the v5.0.0 release; it got effectively regressed in
v5.1.0.
To make things *even more* complicated, the breakage was (and remains, as
of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
no difference with KVM acceleration -- the DWORD accesses still work,
despite "valid.max_access_size = 1".
As commit 5d971f9e6725 suggests, fix the problem by raising
"valid.max_access_size" to 4 -- the spec now clearly instructs the guest
to perform DWORD accesses to the legacy register block too, for enabling
(and verifying!) the modern block. In order to keep compatibility for the
device model implementation though, set "impl.max_access_size = 1", so
that wide accesses be split before they reach the legacy read/write
handlers, like they always have been on KVM, and like they were on TCG
before 5d971f9e6725 (v5.1.0).
Tested with:
- OVMF IA32 + qemu-system-i386, CPU
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Thu, Jan 05, 2023 at 10:00:00AM +0100, Philippe Mathieu-Daudé wrote:
> On 5/1/23 08:13, Laszlo Ersek wrote:
> > On 1/4/23 13:35, Michael S. Tsirkin wrote:
> > > On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote:
> [...]
>
> > > > To make things *even more* complicated, the breakage was (and remains,
> > > > as
> > > > of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
> > > > no difference with KVM acceleration -- the DWORD accesses still work,
> > > > despite "valid.max_access_size = 1".
> > >
> > > BTW do you happen to know why that's the case for KVM? Because if kvm
> > > ignores valid.max_access_size generally then commit 5d971f9e6725 is
> > > incomplete, and we probably have some related kvm-only bugs.
> >
> > It remains a mystery for me why KVM accel does not enforce
> > "valid.max_access_size".
> >
> > In the thread I started earlier (which led to this patch), at
> >
> >"IO port write width clamping differs between TCG and KVM"
> >https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html
>
> [...]
>
> > So, I think the bug is somehow "distributed" between
> > flatview_write_continue(), flatview_access_allowed(), and
> > memory_access_size(). flatview_access_allowed() does not care about "l"
> > at all, when it should (maybe?) compare it against
> > "mr->ops->valid.max_access_size". In turn, memory_access_size()
> > *silently* reduces the access width, based on
> > "->ops->valid.max_access_size".
> >
> > And all this this *precedes* the call to memory_region_access_valid(),
> > which is only called from within memory_region_dispatch_write(), which
> > already gets the reduced width only.
> >
> > Now, flatview_access_allowed() is from commit 3ab6fdc91b72
> > ("softmmu/physmem: Introduce MemTxAttrs::memory field and
> > MEMTX_ACCESS_ERROR", 2022-03-21), and the fact it does not check "len"
> > seems intentional -- it only takes "len" for logging.
> >
> > Hmm. After digging a lot more, I find the issue may have been introduced
> > over three commits:
> >
> > - 82f2563fc815 ("exec: introduce memory_access_size", 2013-05-29), which
> >(IIUC) was the first step towards automatically reducing the address
> >width, but at first only based on alignment,
> >
> > - 23326164ae6f ("exec: Support 64-bit operations in address_space_rw",
> >2013-07-14), which extended the splitting based on
> >"MemoryRegionOps.impl",
> >
> > - e1622f4b1539 ("exec: fix incorrect assumptions in memory_access_size",
> >2013-07-18), which flipped the splitting basis to
> >"MemoryRegionOps.valid".
> >
> > To me, 23326164ae6f seems *vaguely* correct ("vague" is not criticism
> > for the commit, it's criticism for my understanding :)); after all we're
> > on our way towards the device model, and the device model exposes via
> > "MemoryRegionOps.impl" what it can handle. Plus, commit 5d971f9e6725
> > does direct us towards "MemoryRegionOps.impl"!
> >
> > But clearly there must have been something wrong with 23326164ae6f,
> > according to e1622f4b1539...
>
> Maybe the long-standing unaligned access problem? Could be fixed by:
> https://lore.kernel.org/qemu-devel/20210619172626.875885-15-richard.hender...@linaro.org/
indeed. want to dust it up and post?
> > The latter is what introduced the current "silent splitting of access
> > based on 'valid'". The message of commit e1622f4b1539 says, almost like
> > an afterthought:
> >
> > > access_size_max can be mr->ops->valid.max_access_size because
> > > memory.c
> > > can and will still break accesses bigger than
> > > mr->ops->impl.max_access_size.
> >
> > I think this argument may have been wrong: if "impl.max_access_size" is
> > large (such as: unset), but "valid.max_access_size" is small, that just
> > means:
> >
> >the implementation is flexible and can deal with any access widths (so
> >"memory.c" *need not* break up accesses for the device model's sake),
> >but the device should restrict the *guest* to small accesses. So if
> >the guest tries something larger, we shouldn't silently accommodate
> >that.
>
> Indeed. '.impl' is a software thing for the device modeller, ideally one
> will chose a value that allows the simplest implementation. I.e. if a
> device only allows 8-bit access, use 8-bit registers aligned on a 64-bit
> boundary, the model might use:
>
> .impl.min_access_size = 8,
> .impl.max_access_size = 1,
>
> Also we need to keep in mind that even if most MemoryRegionOps structs
> are 'static const', such structure can be dynamically created. I.e.:
> https://lore.kernel.org/qemu-devel/20200817161853.593247-5-f4...@amsat.org/
>
> > I have zero idea how to fix this, but I feel that the quoted argument
> > from commit e1622f4b1539 is the reason why KVM accel is so lenient that
> > it sort of "de-fangs" commit 5d971f9e6725.
> >
> > Laszlo
> >
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 5/1/23 08:13, Laszlo Ersek wrote:
On 1/4/23 13:35, Michael S. Tsirkin wrote:
On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote:
[...]
To make things *even more* complicated, the breakage was (and remains, as
of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
no difference with KVM acceleration -- the DWORD accesses still work,
despite "valid.max_access_size = 1".
BTW do you happen to know why that's the case for KVM? Because if kvm
ignores valid.max_access_size generally then commit 5d971f9e6725 is
incomplete, and we probably have some related kvm-only bugs.
It remains a mystery for me why KVM accel does not enforce
"valid.max_access_size".
In the thread I started earlier (which led to this patch), at
"IO port write width clamping differs between TCG and KVM"
https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html
[...]
So, I think the bug is somehow "distributed" between
flatview_write_continue(), flatview_access_allowed(), and
memory_access_size(). flatview_access_allowed() does not care about "l"
at all, when it should (maybe?) compare it against
"mr->ops->valid.max_access_size". In turn, memory_access_size()
*silently* reduces the access width, based on
"->ops->valid.max_access_size".
And all this this *precedes* the call to memory_region_access_valid(),
which is only called from within memory_region_dispatch_write(), which
already gets the reduced width only.
Now, flatview_access_allowed() is from commit 3ab6fdc91b72
("softmmu/physmem: Introduce MemTxAttrs::memory field and
MEMTX_ACCESS_ERROR", 2022-03-21), and the fact it does not check "len"
seems intentional -- it only takes "len" for logging.
Hmm. After digging a lot more, I find the issue may have been introduced
over three commits:
- 82f2563fc815 ("exec: introduce memory_access_size", 2013-05-29), which
(IIUC) was the first step towards automatically reducing the address
width, but at first only based on alignment,
- 23326164ae6f ("exec: Support 64-bit operations in address_space_rw",
2013-07-14), which extended the splitting based on
"MemoryRegionOps.impl",
- e1622f4b1539 ("exec: fix incorrect assumptions in memory_access_size",
2013-07-18), which flipped the splitting basis to
"MemoryRegionOps.valid".
To me, 23326164ae6f seems *vaguely* correct ("vague" is not criticism
for the commit, it's criticism for my understanding :)); after all we're
on our way towards the device model, and the device model exposes via
"MemoryRegionOps.impl" what it can handle. Plus, commit 5d971f9e6725
does direct us towards "MemoryRegionOps.impl"!
But clearly there must have been something wrong with 23326164ae6f,
according to e1622f4b1539...
Maybe the long-standing unaligned access problem? Could be fixed by:
https://lore.kernel.org/qemu-devel/20210619172626.875885-15-richard.hender...@linaro.org/
The latter is what introduced the current "silent splitting of access
based on 'valid'". The message of commit e1622f4b1539 says, almost like
an afterthought:
access_size_max can be mr->ops->valid.max_access_size because memory.c
can and will still break accesses bigger than
mr->ops->impl.max_access_size.
I think this argument may have been wrong: if "impl.max_access_size" is
large (such as: unset), but "valid.max_access_size" is small, that just
means:
the implementation is flexible and can deal with any access widths (so
"memory.c" *need not* break up accesses for the device model's sake),
but the device should restrict the *guest* to small accesses. So if
the guest tries something larger, we shouldn't silently accommodate
that.
Indeed. '.impl' is a software thing for the device modeller, ideally one
will chose a value that allows the simplest implementation. I.e. if a
device only allows 8-bit access, use 8-bit registers aligned on a 64-bit
boundary, the model might use:
.impl.min_access_size = 8,
.impl.max_access_size = 1,
Also we need to keep in mind that even if most MemoryRegionOps structs
are 'static const', such structure can be dynamically created. I.e.:
https://lore.kernel.org/qemu-devel/20200817161853.593247-5-f4...@amsat.org/
I have zero idea how to fix this, but I feel that the quoted argument
from commit e1622f4b1539 is the reason why KVM accel is so lenient that
it sort of "de-fangs" commit 5d971f9e6725.
Laszlo
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 1/4/23 13:35, Michael S. Tsirkin wrote:
> On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote:
>> The modern ACPI CPU hotplug interface was introduced in the following
>> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>>
>> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
>> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
>> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
>> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
>> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
>> interface
>> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
>> interface
>> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
>> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>>
>> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
>> accesses for the hotplug register block. Patch#1 preserved the same
>> restriction for the legacy register block, but:
>>
>> - it specified DWORD accesses for some of the modern registers,
>>
>> - in particular, the switch from the legacy block to the modern block
>> would require a DWORD write to the *legacy* block.
>>
>> The latter functionality was then implemented in cpu_status_write()
>> [hw/acpi/cpu_hotplug.c], in patch#8.
>>
>> Unfortunately, all DWORD accesses depended on a dormant bug: the one
>> introced
>
> introduced
Huh, thanks. :)
... Because I'm proposing this for stable as well, I figure if I post a
v2 just with this small update, too.
(and I'm just noticing that Phil pointed out the same typo earlier --
sorry Phil, I scrolled through that, my apologies, and thanks for
catching it on your end as well!)
>
>> in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
>> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
>> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
>> register block would work in spite of the above series *not* relaxing
>> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>>
>>> static const MemoryRegionOps AcpiCpuHotplug_ops = {
>>> .read = cpu_status_read,
>>> .write = cpu_status_write,
>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>> .valid = {
>>> .min_access_size = 1,
>>> .max_access_size = 1,
>>> },
>>> };
>>
>> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
>> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
>> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
>> interface (including the documentation) was extended with another DWORD
>> *read* access, namely to the "Command data 2" register, which would be
>> important for the guest to confirm whether it managed to switch the
>> register block from legacy to modern.
>>
>> This functionality too silently depended on the bug from commit
>> a014ed07bd5a.
>>
>> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
>> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
>> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
>> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
>> the v2.7.0 series quoted at the top -- namely the fact that
>> "valid.max_access_size = 1" didn't match what the guest was supposed to
>> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>>
>> The symptom is that the "modern interface negotiation protocol"
>> described in commit ae340aa3d256:
>>
>>> + Use following steps to detect and enable modern CPU hotplug
>>> interface:
>>> +1. Store 0x0 to the 'CPU selector' register,
>>> + attempting to switch to modern mode
>>> +2. Store 0x0 to the 'CPU selector' register,
>>> + to ensure valid selector value
>>> +3. Store 0x0 to the 'Command field' register,
>>> +4. Read the 'Command data 2' register.
>>> + If read value is 0x0, the modern interface is enabled.
>>> + Otherwise legacy or no CPU hotplug interface available
>>
>> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
>> writes; so no switching happens. Step 3 (a single-byte write) is not
>> lost, but it has no effect; see the condition in cpu_status_write() in
>> patch#8. And step 4 *misleads* the guest into thinking that the switch
>> worked: the DWORD read is lost again -- it returns zero to the guest
>> without ever reaching the device model, so the guest never learns the
>> switch didn't work.
>>
>> This means that guest behavior centered on the "Command data 2" register
>> worked *only* in the v5.0.0 release; it got effectively regressed in
>> v5.1.0.
>>
>> To make things *even more* complicated, the breakage was (and remains, as
>> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
>> no difference with KVM
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Wed, Jan 04, 2023 at 10:01:38AM +0100, Laszlo Ersek wrote:
> The modern ACPI CPU hotplug interface was introduced in the following
> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>
> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
> interface
> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
> interface
> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>
> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
> accesses for the hotplug register block. Patch#1 preserved the same
> restriction for the legacy register block, but:
>
> - it specified DWORD accesses for some of the modern registers,
>
> - in particular, the switch from the legacy block to the modern block
> would require a DWORD write to the *legacy* block.
>
> The latter functionality was then implemented in cpu_status_write()
> [hw/acpi/cpu_hotplug.c], in patch#8.
>
> Unfortunately, all DWORD accesses depended on a dormant bug: the one
> introced
introduced
> in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
> register block would work in spite of the above series *not* relaxing
> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>
> > static const MemoryRegionOps AcpiCpuHotplug_ops = {
> > .read = cpu_status_read,
> > .write = cpu_status_write,
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > .valid = {
> > .min_access_size = 1,
> > .max_access_size = 1,
> > },
> > };
>
> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
> interface (including the documentation) was extended with another DWORD
> *read* access, namely to the "Command data 2" register, which would be
> important for the guest to confirm whether it managed to switch the
> register block from legacy to modern.
>
> This functionality too silently depended on the bug from commit
> a014ed07bd5a.
>
> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
> the v2.7.0 series quoted at the top -- namely the fact that
> "valid.max_access_size = 1" didn't match what the guest was supposed to
> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>
> The symptom is that the "modern interface negotiation protocol"
> described in commit ae340aa3d256:
>
> > + Use following steps to detect and enable modern CPU hotplug
> > interface:
> > +1. Store 0x0 to the 'CPU selector' register,
> > + attempting to switch to modern mode
> > +2. Store 0x0 to the 'CPU selector' register,
> > + to ensure valid selector value
> > +3. Store 0x0 to the 'Command field' register,
> > +4. Read the 'Command data 2' register.
> > + If read value is 0x0, the modern interface is enabled.
> > + Otherwise legacy or no CPU hotplug interface available
>
> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
> writes; so no switching happens. Step 3 (a single-byte write) is not
> lost, but it has no effect; see the condition in cpu_status_write() in
> patch#8. And step 4 *misleads* the guest into thinking that the switch
> worked: the DWORD read is lost again -- it returns zero to the guest
> without ever reaching the device model, so the guest never learns the
> switch didn't work.
>
> This means that guest behavior centered on the "Command data 2" register
> worked *only* in the v5.0.0 release; it got effectively regressed in
> v5.1.0.
>
> To make things *even more* complicated, the breakage was (and remains, as
> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
> no difference with KVM acceleration -- the DWORD accesses still work,
> despite "valid.max_access_size = 1".
BTW do you happen to know why that's the case for KVM?
Because if kvm ignores valid.max_access_size generally then
commit 5d971f9e6725 is incomplete, and we probably have some
related kvm-only bugs.
> As commit 5d971f9e6725 suggests, fix the problem by raising
> "valid.max_access_size" to 4 -- the spec now clearly instructs
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 4/1/23 11:38, Igor Mammedov wrote:
On Wed, 4 Jan 2023 10:34:09 +0100
Philippe Mathieu-Daudé wrote:
On 4/1/23 10:01, Laszlo Ersek wrote:
[...]
diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 53654f863830..ff14c3f4106f 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -52,6 +52,9 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
.endianness = DEVICE_LITTLE_ENDIAN,
.valid = {
.min_access_size = 1,
+.max_access_size = 4,
+},
+.impl = {
.max_access_size = 1,
Arguably:
Fixes: b8622725cf ("acpi_piix4: Add infrastructure to send CPU hot-plug
GPE to guest")
nope, this one is correct, as legacy interface used 1 byte access only
Yes, Laszlo explained elsewhere in the thread.
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 1/4/23 11:35, Igor Mammedov wrote:
> On Wed, 4 Jan 2023 10:01:38 +0100
> Laszlo Ersek wrote:
>
>> The modern ACPI CPU hotplug interface was introduced in the following
>> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>>
>> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
>> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
>> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
>> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
>> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
>> interface
>> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
>> interface
>> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
>> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>>
>> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
>> accesses for the hotplug register block. Patch#1 preserved the same
>> restriction for the legacy register block, but:
>>
>> - it specified DWORD accesses for some of the modern registers,
>>
>> - in particular, the switch from the legacy block to the modern block
>> would require a DWORD write to the *legacy* block.
>>
>> The latter functionality was then implemented in cpu_status_write()
>> [hw/acpi/cpu_hotplug.c], in patch#8.
>>
>> Unfortunately, all DWORD accesses depended on a dormant bug: the one
>> introced in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
>> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
>> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
>> register block would work in spite of the above series *not* relaxing
>> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>>
>>> static const MemoryRegionOps AcpiCpuHotplug_ops = {
>>> .read = cpu_status_read,
>>> .write = cpu_status_write,
>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>> .valid = {
>>> .min_access_size = 1,
>>> .max_access_size = 1,
>>> },
>>> };
>>
>> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
>> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
>> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
>> interface (including the documentation) was extended with another DWORD
>> *read* access, namely to the "Command data 2" register, which would be
>> important for the guest to confirm whether it managed to switch the
>> register block from legacy to modern.
>>
>> This functionality too silently depended on the bug from commit
>> a014ed07bd5a.
>>
>> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
>> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
>> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
>> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
>> the v2.7.0 series quoted at the top -- namely the fact that
>> "valid.max_access_size = 1" didn't match what the guest was supposed to
>> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>>
>> The symptom is that the "modern interface negotiation protocol"
>> described in commit ae340aa3d256:
>>
>>> + Use following steps to detect and enable modern CPU hotplug
>>> interface:
>>> +1. Store 0x0 to the 'CPU selector' register,
>>> + attempting to switch to modern mode
>>> +2. Store 0x0 to the 'CPU selector' register,
>>> + to ensure valid selector value
>>> +3. Store 0x0 to the 'Command field' register,
>>> +4. Read the 'Command data 2' register.
>>> + If read value is 0x0, the modern interface is enabled.
>>> + Otherwise legacy or no CPU hotplug interface available
>>
>> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
>> writes; so no switching happens. Step 3 (a single-byte write) is not
>> lost, but it has no effect; see the condition in cpu_status_write() in
>> patch#8. And step 4 *misleads* the guest into thinking that the switch
>> worked: the DWORD read is lost again -- it returns zero to the guest
>> without ever reaching the device model, so the guest never learns the
>> switch didn't work.
>>
>> This means that guest behavior centered on the "Command data 2" register
>> worked *only* in the v5.0.0 release; it got effectively regressed in
>> v5.1.0.
>>
>> To make things *even more* complicated, the breakage was (and remains, as
>> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
>> no difference with KVM acceleration -- the DWORD accesses still work,
>> despite "valid.max_access_size = 1".
>>
>> As commit 5d971f9e6725 suggests, fix the problem by raising
>> "valid.max_access_size" to 4 -- the spec now clearly instructs the guest
>> to perform DWORD accesses to the legacy register block too, for enabling
>> (and verifying!) the
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Wed, 4 Jan 2023 10:34:09 +0100
Philippe Mathieu-Daudé wrote:
> On 4/1/23 10:01, Laszlo Ersek wrote:
[...]
> > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > index 53654f863830..ff14c3f4106f 100644
> > --- a/hw/acpi/cpu_hotplug.c
> > +++ b/hw/acpi/cpu_hotplug.c
> > @@ -52,6 +52,9 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > .valid = {
> > .min_access_size = 1,
> > +.max_access_size = 4,
> > +},
> > +.impl = {
> > .max_access_size = 1,
>
> Arguably:
> Fixes: b8622725cf ("acpi_piix4: Add infrastructure to send CPU hot-plug
> GPE to guest")
nope, this one is correct, as legacy interface used 1 byte access only
>
> > },
> > };
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Wed, 4 Jan 2023 10:01:38 +0100
Laszlo Ersek wrote:
> The modern ACPI CPU hotplug interface was introduced in the following
> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>
> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
> interface
> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
> interface
> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>
> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
> accesses for the hotplug register block. Patch#1 preserved the same
> restriction for the legacy register block, but:
>
> - it specified DWORD accesses for some of the modern registers,
>
> - in particular, the switch from the legacy block to the modern block
> would require a DWORD write to the *legacy* block.
>
> The latter functionality was then implemented in cpu_status_write()
> [hw/acpi/cpu_hotplug.c], in patch#8.
>
> Unfortunately, all DWORD accesses depended on a dormant bug: the one
> introced in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
> register block would work in spite of the above series *not* relaxing
> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>
> > static const MemoryRegionOps AcpiCpuHotplug_ops = {
> > .read = cpu_status_read,
> > .write = cpu_status_write,
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > .valid = {
> > .min_access_size = 1,
> > .max_access_size = 1,
> > },
> > };
>
> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
> interface (including the documentation) was extended with another DWORD
> *read* access, namely to the "Command data 2" register, which would be
> important for the guest to confirm whether it managed to switch the
> register block from legacy to modern.
>
> This functionality too silently depended on the bug from commit
> a014ed07bd5a.
>
> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
> the v2.7.0 series quoted at the top -- namely the fact that
> "valid.max_access_size = 1" didn't match what the guest was supposed to
> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>
> The symptom is that the "modern interface negotiation protocol"
> described in commit ae340aa3d256:
>
> > + Use following steps to detect and enable modern CPU hotplug
> > interface:
> > +1. Store 0x0 to the 'CPU selector' register,
> > + attempting to switch to modern mode
> > +2. Store 0x0 to the 'CPU selector' register,
> > + to ensure valid selector value
> > +3. Store 0x0 to the 'Command field' register,
> > +4. Read the 'Command data 2' register.
> > + If read value is 0x0, the modern interface is enabled.
> > + Otherwise legacy or no CPU hotplug interface available
>
> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
> writes; so no switching happens. Step 3 (a single-byte write) is not
> lost, but it has no effect; see the condition in cpu_status_write() in
> patch#8. And step 4 *misleads* the guest into thinking that the switch
> worked: the DWORD read is lost again -- it returns zero to the guest
> without ever reaching the device model, so the guest never learns the
> switch didn't work.
>
> This means that guest behavior centered on the "Command data 2" register
> worked *only* in the v5.0.0 release; it got effectively regressed in
> v5.1.0.
>
> To make things *even more* complicated, the breakage was (and remains, as
> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
> no difference with KVM acceleration -- the DWORD accesses still work,
> despite "valid.max_access_size = 1".
>
> As commit 5d971f9e6725 suggests, fix the problem by raising
> "valid.max_access_size" to 4 -- the spec now clearly instructs the guest
> to perform DWORD accesses to the legacy register block too, for enabling
> (and verifying!) the modern block. In order to keep compatibility for the
> device model implementation though, set
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 1/4/23 10:34, Philippe Mathieu-Daudé wrote:
> On 4/1/23 10:01, Laszlo Ersek wrote:
>> The modern ACPI CPU hotplug interface was introduced in the following
>> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>>
>> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
>> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
>> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
>> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
>> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
>> interface
>> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
>> interface
>> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
>> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine
>> type
>>
>> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
>> accesses for the hotplug register block. Patch#1 preserved the same
>> restriction for the legacy register block, but:
>>
>> - it specified DWORD accesses for some of the modern registers,
>>
>> - in particular, the switch from the legacy block to the modern block
>> would require a DWORD write to the *legacy* block.
>>
>> The latter functionality was then implemented in cpu_status_write()
>> [hw/acpi/cpu_hotplug.c], in patch#8.
>>
>> Unfortunately, all DWORD accesses depended on a dormant bug: the one
>> introced in earlier commit a014ed07bd5a ("memory: accept mismatching
>> sizes
>
> Typo "introduced",
>
>> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
>> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU
>> hotplug
>> register block would work in spite of the above series *not* relaxing
>> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>>
>>> static const MemoryRegionOps AcpiCpuHotplug_ops = {
>>> .read = cpu_status_read,
>>> .write = cpu_status_write,
>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>> .valid = {
>>> .min_access_size = 1,
>>> .max_access_size = 1,
>>> },
>>> };
>>
>> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
>> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
>> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
>> interface (including the documentation) was extended with another DWORD
>> *read* access, namely to the "Command data 2" register, which would be
>> important for the guest to confirm whether it managed to switch the
>> register block from legacy to modern.
>>
>> This functionality too silently depended on the bug from commit
>> a014ed07bd5a.
>>
>> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
>> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
>> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
>> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
>> the v2.7.0 series quoted at the top -- namely the fact that
>> "valid.max_access_size = 1" didn't match what the guest was supposed to
>> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>>
>> The symptom is that the "modern interface negotiation protocol"
>> described in commit ae340aa3d256:
>>
>>> + Use following steps to detect and enable modern CPU hotplug
>>> interface:
>>> + 1. Store 0x0 to the 'CPU selector' register,
>>> + attempting to switch to modern mode
>>> + 2. Store 0x0 to the 'CPU selector' register,
>>> + to ensure valid selector value
>>> + 3. Store 0x0 to the 'Command field' register,
>>> + 4. Read the 'Command data 2' register.
>>> + If read value is 0x0, the modern interface is enabled.
>>> + Otherwise legacy or no CPU hotplug interface available
>>
>> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
>> writes; so no switching happens. Step 3 (a single-byte write) is not
>> lost, but it has no effect; see the condition in cpu_status_write() in
>> patch#8. And step 4 *misleads* the guest into thinking that the switch
>> worked: the DWORD read is lost again -- it returns zero to the guest
>> without ever reaching the device model, so the guest never learns the
>> switch didn't work.
>>
>> This means that guest behavior centered on the "Command data 2" register
>> worked *only* in the v5.0.0 release; it got effectively regressed in
>> v5.1.0.
>>
>> To make things *even more* complicated, the breakage was (and remains, as
>> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
>> no difference with KVM acceleration -- the DWORD accesses still work,
>> despite "valid.max_access_size = 1".
>>
>> As commit 5d971f9e6725 suggests, fix the problem by raising
>> "valid.max_access_size" to 4 -- the spec now clearly instructs the guest
>> to perform DWORD accesses to the legacy register block too, for
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 1/4/23 10:33, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 10:01, Laszlo Ersek wrote:
>>
>> The modern ACPI CPU hotplug interface was introduced in the following
>> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>>
>> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
>> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
>> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
>> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
>> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
>> interface
>> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
>> interface
>> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
>> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>>
>> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
>> accesses for the hotplug register block. Patch#1 preserved the same
>> restriction for the legacy register block, but:
>>
>> - it specified DWORD accesses for some of the modern registers,
>>
>> - in particular, the switch from the legacy block to the modern block
>> would require a DWORD write to the *legacy* block.
>>
>> The latter functionality was then implemented in cpu_status_write()
>> [hw/acpi/cpu_hotplug.c], in patch#8.
>>
>> Unfortunately, all DWORD accesses depended on a dormant bug: the one
>> introced in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
>> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
>> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
>> register block would work in spite of the above series *not* relaxing
>> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>>
>>> static const MemoryRegionOps AcpiCpuHotplug_ops = {
>>> .read = cpu_status_read,
>>> .write = cpu_status_write,
>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>> .valid = {
>>> .min_access_size = 1,
>>> .max_access_size = 1,
>>> },
>>> };
>>
>> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
>> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
>> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
>> interface (including the documentation) was extended with another DWORD
>> *read* access, namely to the "Command data 2" register, which would be
>> important for the guest to confirm whether it managed to switch the
>> register block from legacy to modern.
>>
>> This functionality too silently depended on the bug from commit
>> a014ed07bd5a.
>>
>> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
>> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
>> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
>> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
>> the v2.7.0 series quoted at the top -- namely the fact that
>> "valid.max_access_size = 1" didn't match what the guest was supposed to
>> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>>
>> The symptom is that the "modern interface negotiation protocol"
>> described in commit ae340aa3d256:
>>
>>> + Use following steps to detect and enable modern CPU hotplug
>>> interface:
>>> +1. Store 0x0 to the 'CPU selector' register,
>>> + attempting to switch to modern mode
>>> +2. Store 0x0 to the 'CPU selector' register,
>>> + to ensure valid selector value
>>> +3. Store 0x0 to the 'Command field' register,
>>> +4. Read the 'Command data 2' register.
>>> + If read value is 0x0, the modern interface is enabled.
>>> + Otherwise legacy or no CPU hotplug interface available
>>
>> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
>> writes; so no switching happens. Step 3 (a single-byte write) is not
>> lost, but it has no effect; see the condition in cpu_status_write() in
>> patch#8. And step 4 *misleads* the guest into thinking that the switch
>> worked: the DWORD read is lost again -- it returns zero to the guest
>> without ever reaching the device model, so the guest never learns the
>> switch didn't work.
>>
>> This means that guest behavior centered on the "Command data 2" register
>> worked *only* in the v5.0.0 release; it got effectively regressed in
>> v5.1.0.
>>
>> To make things *even more* complicated, the breakage was (and remains, as
>> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
>> no difference with KVM acceleration -- the DWORD accesses still work,
>> despite "valid.max_access_size = 1".
>>
>> As commit 5d971f9e6725 suggests, fix the problem by raising
>> "valid.max_access_size" to 4 -- the spec now clearly instructs the guest
>> to perform DWORD accesses to the legacy register block too, for enabling
>> (and verifying!) the modern
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On 4/1/23 10:01, Laszlo Ersek wrote:
The modern ACPI CPU hotplug interface was introduced in the following
series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
interface
6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
interface
7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
accesses for the hotplug register block. Patch#1 preserved the same
restriction for the legacy register block, but:
- it specified DWORD accesses for some of the modern registers,
- in particular, the switch from the legacy block to the modern block
would require a DWORD write to the *legacy* block.
The latter functionality was then implemented in cpu_status_write()
[hw/acpi/cpu_hotplug.c], in patch#8.
Unfortunately, all DWORD accesses depended on a dormant bug: the one
introced in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
Typo "introduced",
in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
register block would work in spite of the above series *not* relaxing
"valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
static const MemoryRegionOps AcpiCpuHotplug_ops = {
.read = cpu_status_read,
.write = cpu_status_write,
.endianness = DEVICE_LITTLE_ENDIAN,
.valid = {
.min_access_size = 1,
.max_access_size = 1,
},
};
Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
interface (including the documentation) was extended with another DWORD
*read* access, namely to the "Command data 2" register, which would be
important for the guest to confirm whether it managed to switch the
register block from legacy to modern.
This functionality too silently depended on the bug from commit
a014ed07bd5a.
In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
the bug from commit a014ed07bd5a was fixed (the commit was reverted).
That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
the v2.7.0 series quoted at the top -- namely the fact that
"valid.max_access_size = 1" didn't match what the guest was supposed to
do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
The symptom is that the "modern interface negotiation protocol"
described in commit ae340aa3d256:
+ Use following steps to detect and enable modern CPU hotplug interface:
+1. Store 0x0 to the 'CPU selector' register,
+ attempting to switch to modern mode
+2. Store 0x0 to the 'CPU selector' register,
+ to ensure valid selector value
+3. Store 0x0 to the 'Command field' register,
+4. Read the 'Command data 2' register.
+ If read value is 0x0, the modern interface is enabled.
+ Otherwise legacy or no CPU hotplug interface available
falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
writes; so no switching happens. Step 3 (a single-byte write) is not
lost, but it has no effect; see the condition in cpu_status_write() in
patch#8. And step 4 *misleads* the guest into thinking that the switch
worked: the DWORD read is lost again -- it returns zero to the guest
without ever reaching the device model, so the guest never learns the
switch didn't work.
This means that guest behavior centered on the "Command data 2" register
worked *only* in the v5.0.0 release; it got effectively regressed in
v5.1.0.
To make things *even more* complicated, the breakage was (and remains, as
of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
no difference with KVM acceleration -- the DWORD accesses still work,
despite "valid.max_access_size = 1".
As commit 5d971f9e6725 suggests, fix the problem by raising
"valid.max_access_size" to 4 -- the spec now clearly instructs the guest
to perform DWORD accesses to the legacy register block too, for enabling
(and verifying!) the modern block. In order to keep compatibility for the
device model implementation though, set "impl.max_access_size = 1", so
that wide accesses be split before they reach the legacy read/write
handlers, like they always have been on KVM, and like they were on TCG
before 5d971f9e6725 (v5.1.0).
Tested with:
- OVMF
Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
On Wed, 4 Jan 2023 at 10:01, Laszlo Ersek wrote:
>
> The modern ACPI CPU hotplug interface was introduced in the following
> series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
>
> 1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
> 2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
> 3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
> 4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
> 5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
> interface
> 6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
> interface
> 7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
> 8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
>
> Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
> accesses for the hotplug register block. Patch#1 preserved the same
> restriction for the legacy register block, but:
>
> - it specified DWORD accesses for some of the modern registers,
>
> - in particular, the switch from the legacy block to the modern block
> would require a DWORD write to the *legacy* block.
>
> The latter functionality was then implemented in cpu_status_write()
> [hw/acpi/cpu_hotplug.c], in patch#8.
>
> Unfortunately, all DWORD accesses depended on a dormant bug: the one
> introced in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
> in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
> Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
> register block would work in spite of the above series *not* relaxing
> "valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
>
> > static const MemoryRegionOps AcpiCpuHotplug_ops = {
> > .read = cpu_status_read,
> > .write = cpu_status_write,
> > .endianness = DEVICE_LITTLE_ENDIAN,
> > .valid = {
> > .min_access_size = 1,
> > .max_access_size = 1,
> > },
> > };
>
> Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
> field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
> usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
> interface (including the documentation) was extended with another DWORD
> *read* access, namely to the "Command data 2" register, which would be
> important for the guest to confirm whether it managed to switch the
> register block from legacy to modern.
>
> This functionality too silently depended on the bug from commit
> a014ed07bd5a.
>
> In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
> in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
> the bug from commit a014ed07bd5a was fixed (the commit was reverted).
> That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
> the v2.7.0 series quoted at the top -- namely the fact that
> "valid.max_access_size = 1" didn't match what the guest was supposed to
> do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
>
> The symptom is that the "modern interface negotiation protocol"
> described in commit ae340aa3d256:
>
> > + Use following steps to detect and enable modern CPU hotplug
> > interface:
> > +1. Store 0x0 to the 'CPU selector' register,
> > + attempting to switch to modern mode
> > +2. Store 0x0 to the 'CPU selector' register,
> > + to ensure valid selector value
> > +3. Store 0x0 to the 'Command field' register,
> > +4. Read the 'Command data 2' register.
> > + If read value is 0x0, the modern interface is enabled.
> > + Otherwise legacy or no CPU hotplug interface available
>
> falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
> writes; so no switching happens. Step 3 (a single-byte write) is not
> lost, but it has no effect; see the condition in cpu_status_write() in
> patch#8. And step 4 *misleads* the guest into thinking that the switch
> worked: the DWORD read is lost again -- it returns zero to the guest
> without ever reaching the device model, so the guest never learns the
> switch didn't work.
>
> This means that guest behavior centered on the "Command data 2" register
> worked *only* in the v5.0.0 release; it got effectively regressed in
> v5.1.0.
>
> To make things *even more* complicated, the breakage was (and remains, as
> of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
> no difference with KVM acceleration -- the DWORD accesses still work,
> despite "valid.max_access_size = 1".
>
> As commit 5d971f9e6725 suggests, fix the problem by raising
> "valid.max_access_size" to 4 -- the spec now clearly instructs the guest
> to perform DWORD accesses to the legacy register block too, for enabling
> (and verifying!) the modern block. In order to keep compatibility for the
> device model implementation though, set "impl.max_access_size = 1", so
>
[PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block
The modern ACPI CPU hotplug interface was introduced in the following
series (aa1dd39ca307..679dd1a957df), released in v2.7.0:
1 abd49bc2ed2f docs: update ACPI CPU hotplug spec with new protocol
2 16bcab97eb9f pc: piix4/ich9: add 'cpu-hotplug-legacy' property
3 5e1b5d93887b acpi: cpuhp: add CPU devices AML with _STA method
4 ac35f13ba8f8 pc: acpi: introduce AcpiDeviceIfClass.madt_cpu hook
5 d2238cb6781d acpi: cpuhp: implement hot-add parts of CPU hotplug
interface
6 8872c25a26cc acpi: cpuhp: implement hot-remove parts of CPU hotplug
interface
7 76623d00ae57 acpi: cpuhp: add cpu._OST handling
8 679dd1a957df pc: use new CPU hotplug interface since 2.7 machine type
Before patch#1, "docs/specs/acpi_cpu_hotplug.txt" only specified 1-byte
accesses for the hotplug register block. Patch#1 preserved the same
restriction for the legacy register block, but:
- it specified DWORD accesses for some of the modern registers,
- in particular, the switch from the legacy block to the modern block
would require a DWORD write to the *legacy* block.
The latter functionality was then implemented in cpu_status_write()
[hw/acpi/cpu_hotplug.c], in patch#8.
Unfortunately, all DWORD accesses depended on a dormant bug: the one
introced in earlier commit a014ed07bd5a ("memory: accept mismatching sizes
in memory_region_access_valid", 2013-05-29); first released in v1.6.0.
Due to commit a014ed07bd5a, the DWORD accesses to the *legacy* CPU hotplug
register block would work in spite of the above series *not* relaxing
"valid.max_access_size = 1" in "hw/acpi/cpu_hotplug.c":
> static const MemoryRegionOps AcpiCpuHotplug_ops = {
> .read = cpu_status_read,
> .write = cpu_status_write,
> .endianness = DEVICE_LITTLE_ENDIAN,
> .valid = {
> .min_access_size = 1,
> .max_access_size = 1,
> },
> };
Later, in commits e6d0c3ce6895 ("acpi: cpuhp: introduce 'Command data 2'
field", 2020-01-22) and ae340aa3d256 ("acpi: cpuhp: spec: add typical
usecases", 2020-01-22), first released in v5.0.0, the modern CPU hotplug
interface (including the documentation) was extended with another DWORD
*read* access, namely to the "Command data 2" register, which would be
important for the guest to confirm whether it managed to switch the
register block from legacy to modern.
This functionality too silently depended on the bug from commit
a014ed07bd5a.
In commit 5d971f9e6725 ('memory: Revert "memory: accept mismatching sizes
in memory_region_access_valid"', 2020-06-26), first released in v5.1.0,
the bug from commit a014ed07bd5a was fixed (the commit was reverted).
That swiftly exposed the bug in "AcpiCpuHotplug_ops", still present from
the v2.7.0 series quoted at the top -- namely the fact that
"valid.max_access_size = 1" didn't match what the guest was supposed to
do, according to the spec ("docs/specs/acpi_cpu_hotplug.txt").
The symptom is that the "modern interface negotiation protocol"
described in commit ae340aa3d256:
> + Use following steps to detect and enable modern CPU hotplug interface:
> +1. Store 0x0 to the 'CPU selector' register,
> + attempting to switch to modern mode
> +2. Store 0x0 to the 'CPU selector' register,
> + to ensure valid selector value
> +3. Store 0x0 to the 'Command field' register,
> +4. Read the 'Command data 2' register.
> + If read value is 0x0, the modern interface is enabled.
> + Otherwise legacy or no CPU hotplug interface available
falls apart for the guest: steps 1 and 2 are lost, because they are DWORD
writes; so no switching happens. Step 3 (a single-byte write) is not
lost, but it has no effect; see the condition in cpu_status_write() in
patch#8. And step 4 *misleads* the guest into thinking that the switch
worked: the DWORD read is lost again -- it returns zero to the guest
without ever reaching the device model, so the guest never learns the
switch didn't work.
This means that guest behavior centered on the "Command data 2" register
worked *only* in the v5.0.0 release; it got effectively regressed in
v5.1.0.
To make things *even more* complicated, the breakage was (and remains, as
of today) visible with TCG acceleration only. Commit 5d971f9e6725 makes
no difference with KVM acceleration -- the DWORD accesses still work,
despite "valid.max_access_size = 1".
As commit 5d971f9e6725 suggests, fix the problem by raising
"valid.max_access_size" to 4 -- the spec now clearly instructs the guest
to perform DWORD accesses to the legacy register block too, for enabling
(and verifying!) the modern block. In order to keep compatibility for the
device model implementation though, set "impl.max_access_size = 1", so
that wide accesses be split before they reach the legacy read/write
handlers, like they always have been on KVM, and like they were on TCG
before 5d971f9e6725 (v5.1.0).
Tested with:
- OVMF IA32 + qemu-system-i386, CPU hotplug/hot-unplug
