| Message ID | 20230105161804.82486-1-lersek@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block | expand |
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 > "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 with SMM, > intermixed with ACPI S3 suspend/resume, using KVM accel > (regression-test); > > - OVMF IA32X64 + qemu-system-x86_64, CPU hotplug/hot-unplug with SMM, > intermixed with ACPI S3 suspend/resume, using KVM accel > (regression-test); > > - OVMF IA32 + qemu-system-i386, SMM enabled, using TCG accel; verified the > register block switch and the present/possible CPU counting through the > modern hotplug interface, during OVMF boot (bugfix test); > > - I do not have any testcase (guest payload) for regression-testing CPU > hotplug through the *legacy* CPU hotplug register block. > > Cc: "Michael S. Tsirkin" <mst@redhat.com> > Cc: Ani Sinha <ani@anisinha.ca> > Cc: Ard Biesheuvel <ardb@kernel.org> > Cc: Igor Mammedov <imammedo@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Philippe Mathieu-Daudé <philmd@linaro.org> > Cc: qemu-stable@nongnu.org > Ref: "IO port write width clamping differs between TCG and KVM" > Link: http://mid.mail-archive.com/aaedee84-d3ed-a4f9-21e7-d221a28d1683@redhat.com > Link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html > Reported-by: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > Tested-by: Ard Biesheuvel <ardb@kernel.org> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Tested-by: Igor Mammedov <imammedo@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Notes: > v2 notes: > > - typo fix in commit message [Phil, Michael] > > - pick up T-b from Ard and Igor, R-b from Phil and Michael > > v1 notes: > > This should be applied to: > > - stable-5.2 (new branch) > > - stable-6.2 (new branch) > > - stable-7.2 (new branch) > > whichever is still considered maintained, as there is currently *no* > public QEMU release in which the modern CPU hotplug register block > works, when using TCG acceleration. v5.0.0 works, but that minor > release has been obsoleted by v5.2.0, which does not work. > > hw/acpi/cpu_hotplug.c | 3 +++ > 1 file changed, 3 insertions(+) > > 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, > }, > };
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, }, };
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 hotplug/hot-unplug with SMM, intermixed with ACPI S3 suspend/resume, using KVM accel (regression-test); - OVMF IA32X64 + qemu-system-x86_64, CPU hotplug/hot-unplug with SMM, intermixed with ACPI S3 suspend/resume, using KVM accel (regression-test); - OVMF IA32 + qemu-system-i386, SMM enabled, using TCG accel; verified the register block switch and the present/possible CPU counting through the modern hotplug interface, during OVMF boot (bugfix test); - I do not have any testcase (guest payload) for regression-testing CPU hotplug through the *legacy* CPU hotplug register block. Cc: "Michael S. Tsirkin" <mst@redhat.com> Cc: Ani Sinha <ani@anisinha.ca> Cc: Ard Biesheuvel <ardb@kernel.org> Cc: Igor Mammedov <imammedo@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Philippe Mathieu-Daudé <philmd@linaro.org> Cc: qemu-stable@nongnu.org Ref: "IO port write width clamping differs between TCG and KVM" Link: http://mid.mail-archive.com/aaedee84-d3ed-a4f9-21e7-d221a28d1683@redhat.com Link: https://lists.gnu.org/archive/html/qemu-devel/2023-01/msg00199.html Reported-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Laszlo Ersek <lersek@redhat.com> Tested-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> Tested-by: Igor Mammedov <imammedo@redhat.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> --- Notes: v2 notes: - typo fix in commit message [Phil, Michael] - pick up T-b from Ard and Igor, R-b from Phil and Michael v1 notes: This should be applied to: - stable-5.2 (new branch) - stable-6.2 (new branch) - stable-7.2 (new branch) whichever is still considered maintained, as there is currently *no* public QEMU release in which the modern CPU hotplug register block works, when using TCG acceleration. v5.0.0 works, but that minor release has been obsoleted by v5.2.0, which does not work. hw/acpi/cpu_hotplug.c | 3 +++ 1 file changed, 3 insertions(+)