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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.11.2024 12:57, Andrew Cooper wrote:
> On 28/11/2024 11:50 am, Jan Beulich wrote:
>> 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.
> 
> That's the whole point.  It's not about Xen's awareness; it's what
> APIC-V/AVIC might do *in existing configurations* on future hardware
> without taking a VMExit.

How would you migrate such a guest to arbitrary other hardware, i.e.
potentially lacking support for that bit? If LVTERR triggering is as per
Roger's reading of the SDM, without knowing how many bits hardware
presently checks we couldn't guarantee correctness. Bits from 8 up being
reserved right now even leaves me wondering what happens on present
hardware when one of those top 24 bits is set.

> If there were no APIC-V support to begin with, this would be easy and
> auditing would be limited to SENDILL|RECVILL as those are the only two
> bits Xen knows about.

Limiting to just these two bits would be wrong; future Xen might make
use of more of them, and a guest should then still migrate correctly
(just that, after this initial being set of extra bits, it would never
again see any of them becoming set).

Jan