Re: [PATCH v2 1/2] x86/vlapic: Fix handling of writes to APIC_ESR

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 05/03/2025 1:49 pm, Jan Beulich wrote:
> On 03.03.2025 19:53, Andrew Cooper wrote:
>> Xen currently presents APIC_ESR to guests as a simple read/write register.
>>
>> This is incorrect.  The SDM states:
>>
>>   The ESR is a write/read register. Before attempt to read from the ESR,
>>   software should first write to it. (The value written does not affect the
>>   values read subsequently; only zero may be written in x2APIC mode.) This
>>   write clears any previously logged errors and updates the ESR with any
>>   errors detected since the last write to the ESR.
>>
>> Introduce a new pending_esr field in hvm_hw_lapic.
>>
>> Update vlapic_error() to accumulate errors here, and extend 
>> vlapic_reg_write()
>> to discard the written value and transfer pending_esr into APIC_ESR.  Reads
>> are still as before.
>>
>> Importantly, this means that guests no longer destroys the ESR value it's
>> looking for in the LVTERR handler when following the SDM instructions.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks.

> I guess there's no good Fixes: candidate?

Not that I could find.  5f32d186a8b introduced vlapic_error(), and
therefore delivery of LVTERR to guests, but the (mis)behaviour of the
APIC_ESR register goes back further.

d1bd157fbc9b "Big merge the HVM full-virtualisation abstractions" 19
years ago had a far more buggy behaviour (counted writes, and only
returned data on half the reads).

The read side (of this far more broken behaviour) was broken by
50b3cef2eecb ("[HVM] Place all APIC registers into one page in native
format.") and restored by 69f646a61b1b ("[HVM] Fix some IOAPIC and LAPIC
device model bugs").

The far more broken behaviour was dropped by f7c8af3a6476 ("[XEN] HVM:
Clean up and simplify vlapic device-model code.") in 3.0.4, leaving the
behaviour we've had until today.

~Andrew