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

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

On Thu, Nov 28, 2024 at 10:10:39AM +0000, Andrew Cooper wrote:
> On 28/11/2024 9:26 am, Roger Pau Monné wrote:
> > 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.
> 
> It can't stay as a mask, or we can't convert the logic to be lockless. 
> There's no such thing as test_and_set_mask()  (until we get into next
> years processors).

The test_and_set_bit() will also need to be changed if you agree with
my comment on patch 1, as the interrupt should only be injected when
vlapic->hw.pending_esr == 0 rather than whether the specific error is
set in ESR.

> If you really don't like ilog2(), then we need a parallel set of
> APIC_ESR_*_BIT constants, but I considered ilog2() to be the lesser of
> these two evils.
> 
> > 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));\
> > })
> 
> This is overkill IMO.  There are 3 callers and they're all local to
> apic.c (hopefully soon to gain a 4th, but still).

My recommendation would be a local macro in vlapic.c, but I'm
certainly not going to block the patch hover this.

Thanks, Roger.