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