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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2024 12:10, Andrew Cooper wrote:
> On 28/11/2024 10:31 am, Jan Beulich wrote:
>> On 28.11.2024 01:47, 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. This write also rearms 
>>> the
>>>   APIC error interrupt triggering mechanism.
>>>
>>> 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 instead 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>
>> No Fixes: tag presumably because the issue had been there forever?
> 
> Oh, I forgot to note that.
> 
> I can't decide between forever, or since the introduction of the ESR
> support (so Xen 4.5 like XSA-462, and still basically forever).
>>> ---
>>> Slightly RFC.  This collides with Alejandro's patch which adds the apic_id
>>> field to hvm_hw_lapic too.  However, this is a far more obvious backport
>>> candidate.
>>>
>>> lapic_check_hidden() might in principle want to audit this field, but it's 
>>> not
>>> clear what to check.  While prior Xen will never have produced it in the
>>> migration stream, Intel APIC-V will set APIC_ESR_ILLREGA above and beyond 
>>> what
>>> Xen will currently emulate.
>> The ESR really is an 8-bit value (in a 32-bit register), so checking the
>> upper bits may be necessary.
> 
> It is now, but it may not be in the future.
> 
> My concern is that this value is generated by microcode, so we can't
> audit based on which reserved bits we think prior versions of Xen never set.
> 
> I don't particularly care about a toolstack deciding to feed ~0 in
> here.  But, if any bit beyond 7 gets allocated in the future, then
> auditing the bottom byte would lead to a migration failure of what is in
> practice a correct value.

If a bit beyond zero got allocated, then it being set in an incoming stream
will, for an unaware Xen version, still be illegal. Such a guest simply can't
be migrated to a Xen version unaware of the bit. Once Xen becomes aware, the
auditing would (of course) also need adjustment.

Jan