Re: [PATCH 2/2] x86/vlapic: Drop vlapic->esr_lock

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

On Thu, Nov 28, 2024 at 12:47:37AM +0000, Andrew Cooper wrote:
> With vlapic->hw.pending_esr held outside of the main regs page, it's much
> easier to use atomic operations.
> 
> Use xchg() in vlapic_reg_write(), and *set_bit() in vlapic_error().
> 
> The only interesting change is that vlapic_error() now needs to take an
> err_bit rather than an errmask, but thats fine for all current callers and
> forseable changes.
> 
> No practical change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> It turns out that XSA-462 had an indentation bug in it.
> 
> Our spinlock infrastructure is obscenely large.  Bloat-o-meter reports:
> 
>   add/remove: 0/0 grow/shrink: 0/3 up/down: 0/-111 (-111)
>   Function                                     old     new   delta
>   vlapic_init                                  208     190     -18
>   vlapic_error                                 112      67     -45
>   vlapic_reg_write                            1145    1097     -48
> 
> In principle we could revert the XSA-462 patch now, and remove the LVTERR
> vector handling special case.  MISRA is going to complain either way, because
> it will see the cycle through vlapic_set_irq() without considering the
> surrounding logic.
> ---
>  xen/arch/x86/hvm/vlapic.c             | 32 ++++++---------------------
>  xen/arch/x86/include/asm/hvm/vlapic.h |  1 -
>  2 files changed, 7 insertions(+), 26 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
> index 98394ed26a52..f41a5d4619bb 100644
> --- a/xen/arch/x86/hvm/vlapic.c
> +++ b/xen/arch/x86/hvm/vlapic.c
> @@ -102,14 +102,9 @@ static int vlapic_find_highest_irr(struct vlapic *vlapic)
>      return vlapic_find_highest_vector(&vlapic->regs->data[APIC_IRR]);
>  }
>  
> -static void vlapic_error(struct vlapic *vlapic, unsigned int errmask)
> +static void vlapic_error(struct vlapic *vlapic, unsigned int err_bit)

Having to use ilog2() in the callers is kind of ugly.  I would rather
keep the same function parameter (a mask), and then either assert that
it only has one bit set, or iterate over all possible set bits on the
mask.

I assume you had a preference for doing it at the caller because it
would then be done by the preprocessor as the passed values are
macros.  Maybe we could add a wrapper about it:

static void vlapic_set_error_bit(struct vlapic *vlapic, unsigned int bit)
{ ... }

#define vlapic_error(v, m) ({         \
    BUILD_BUG_ON((m) & ((m) - 1));    \
    vlapic_set_error_bit(v, ilog2(m));\
})


>  {
> -    unsigned long flags;
> -    uint32_t esr;
> -
> -    spin_lock_irqsave(&vlapic->esr_lock, flags);
> -    esr = vlapic->hw.pending_esr;
> -    if ( (esr & errmask) != errmask )
> +    if ( !test_and_set_bit(err_bit, &vlapic->hw.pending_esr) )
>      {
>          uint32_t lvterr = vlapic_get_reg(vlapic, APIC_LVTERR);
>          bool inj = false;
> @@ -124,15 +119,12 @@ static void vlapic_error(struct vlapic *vlapic, 
> unsigned int errmask)
>              if ( (lvterr & APIC_VECTOR_MASK) >= 16 )
>                   inj = true;

The line above also has bogus indentation.

Thanks, Roger.