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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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

Yes, that was mentioned.

~Andrew