On 28/11/2024 10:25 am, Roger Pau Monné wrote: > 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.
I'm writing a longer email explaining why that's not correct :) ~Andrew
