Re: [PATCH] acpi: cpuhp: fix guest-visible maximum access size to the legacy reg block

[Laszlo Ersek]

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