On 1/4/23 11:35, Igor Mammedov wrote:
> On Wed, 4 Jan 2023 10:01:38 +0100
> Laszlo Ersek <ler...@redhat.com> 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
>> 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.
> I've checked it with old Seabios (that had it's own ACPI tables) (taken from
> 1.6 QEMU branch),
> it works fine in TCG and KVM mode.
>
> Tested-by: Igor Mammedov <imamm...@redhat.com>
Awesome, thank you!
Laszlo
>
>>
>> Cc: "Michael S. Tsirkin" <m...@redhat.com>
>> Cc: Ani Sinha <a...@anisinha.ca>
>> Cc: Ard Biesheuvel <a...@kernel.org>
>> Cc: Igor Mammedov <imamm...@redhat.com>
>> Cc: Paolo Bonzini <pbonz...@redhat.com>
>> Cc: Peter Maydell <peter.mayd...@linaro.org>
>> Cc: qemu-sta...@nongnu.org
>> Ref: "IO port write width clamping differs between TCG and KVM"
>> Link:
>> aaedee84-d3ed-a4f9-21e7-d221a28d1683@redhat.com">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 <a...@kernel.org>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>>
>> 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,
>> },
>> };
>
