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 <[email protected]>
> >> ---
> >> CC: Jan Beulich <[email protected]>
> >> CC: Roger Pau Monné <[email protected]>
> >>
> >> 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.
