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 <[email protected]>
No Fixes: tag presumably because the issue had been there 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. Plus ...
> --- a/xen/include/public/arch-x86/hvm/save.h
> +++ b/xen/include/public/arch-x86/hvm/save.h
> @@ -394,6 +394,7 @@ struct hvm_hw_lapic {
> uint32_t disabled; /* VLAPIC_xx_DISABLED */
> uint32_t timer_divisor;
> uint64_t tdt_msr;
> + uint32_t pending_esr;
> };
... I think you need to make padding explicit here, and then check that
to be zero.
Jan
