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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 <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.

I'm writing a longer email explaining why that's not correct :)

~Andrew