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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

~Andrew