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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Nov 28, 2024 at 12:47:36AM +0000, 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>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> 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.
> 
> I've checked that this does behave correctly under Intel APIC-V.  Writes to
> APIC_ESR drop the written value into the backing page then take a trap-style
> EXIT_REASON_APIC_WRITE which allows us to sample/latch properly.
> ---
>  xen/arch/x86/hvm/vlapic.c              | 17 +++++++++++++++--
>  xen/include/public/arch-x86/hvm/save.h |  1 +
>  2 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 3363926b487b..98394ed26a52 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -108,7 +108,7 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
> int errmask)
>      uint32_t esr;
>  
>      spin_lock_irqsave(&vlapic->esr_lock, flags);
> -    esr = vlapic_get_reg(vlapic, APIC_ESR);
> +    esr = vlapic->hw.pending_esr;
>      if ( (esr & errmask) != errmask )
>      {
>          uint32_t lvterr = vlapic_get_reg(vlapic, APIC_LVTERR);
> @@ -127,7 +127,7 @@ static void vlapic_error(struct vlapic *vlapic, unsigned 
> int errmask)
>                   errmask |= APIC_ESR_RECVILL;
>          }
>  
> -        vlapic_set_reg(vlapic, APIC_ESR, esr | errmask);
> +        vlapic->hw.pending_esr |= errmask;
>  
>          if ( inj )
>              vlapic_set_irq(vlapic, lvterr & APIC_VECTOR_MASK, 0);

The SDM also contains:

"This write also rearms the APIC error interrupt triggering
mechanism."

Where "this write" is a write to the ESR register.  My understanding
is that the error vector will only be injected for the first reported
error. I think the logic regarding whether to inject the lvterr vector
needs to additionally be gated on whether vlapic->hw.pending_esr ==
0.

Thanks, Roger.