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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
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)
{
- 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;
else
- errmask |= APIC_ESR_RECVILL;
+ set_bit(ilog2(APIC_ESR_RECVILL), &vlapic->hw.pending_esr);
}
- vlapic->hw.pending_esr |= errmask;
-
if ( inj )
vlapic_set_irq(vlapic, lvterr & APIC_VECTOR_MASK, 0);
}
- spin_unlock_irqrestore(&vlapic->esr_lock, flags);
}
bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec)
@@ -153,7 +145,7 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
if ( unlikely(vec < 16) )
{
- vlapic_error(vlapic, APIC_ESR_RECVILL);
+ vlapic_error(vlapic, ilog2(APIC_ESR_RECVILL));
return;
}
@@ -525,7 +517,7 @@ void vlapic_ipi(
vlapic_domain(vlapic), vlapic, short_hand, dest, dest_mode);
if ( unlikely((icr_low & APIC_VECTOR_MASK) < 16) )
- vlapic_error(vlapic, APIC_ESR_SENDILL);
+ vlapic_error(vlapic, ilog2(APIC_ESR_SENDILL));
else if ( target )
vlapic_accept_irq(vlapic_vcpu(target), icr_low);
break;
@@ -534,7 +526,7 @@ void vlapic_ipi(
case APIC_DM_FIXED:
if ( unlikely((icr_low & APIC_VECTOR_MASK) < 16) )
{
- vlapic_error(vlapic, APIC_ESR_SENDILL);
+ vlapic_error(vlapic, ilog2(APIC_ESR_SENDILL));
break;
}
/* fall through */
@@ -803,17 +795,9 @@ void vlapic_reg_write(struct vcpu *v, unsigned int reg, uint32_t val)
break;
case APIC_ESR:
- {
- unsigned long flags;
-
- spin_lock_irqsave(&vlapic->esr_lock, flags);
- val = vlapic->hw.pending_esr;
- vlapic->hw.pending_esr = 0;
- spin_unlock_irqrestore(&vlapic->esr_lock, flags);
-
+ val = xchg(&vlapic->hw.pending_esr, 0);
vlapic_set_reg(vlapic, APIC_ESR, val);
break;
- }
case APIC_TASKPRI:
vlapic_set_reg(vlapic, APIC_TASKPRI, val & 0xff);
@@ -1716,8 +1700,6 @@ int vlapic_init(struct vcpu *v)
vlapic_reset(vlapic);
- spin_lock_init(&vlapic->esr_lock);
-
tasklet_init(&vlapic->init_sipi.tasklet, vlapic_init_sipi_action, v);
if ( v->vcpu_id == 0 )
diff --git a/xen/arch/x86/include/asm/hvm/vlapic.h b/xen/arch/x86/include/asm/hvm/vlapic.h
index 2c4ff94ae7a8..c38855119836 100644
--- a/xen/arch/x86/include/asm/hvm/vlapic.h
+++ b/xen/arch/x86/include/asm/hvm/vlapic.h
@@ -69,7 +69,6 @@ struct vlapic {
bool hw, regs;
uint32_t id, ldr;
} loaded;
- spinlock_t esr_lock;
struct periodic_time pt;
s_time_t timer_last_update;
struct page_info *regs_page;
--
2.39.5
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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> 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.
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));\
})
> {
> - 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.
Thanks, Roger.
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@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> 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
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@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> 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.
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@citrix.com> >>>> --- >>>> CC: Jan Beulich <JBeulich@suse.com> >>>> CC: Roger Pau Monné <roger.pau@citrix.com> >>>> >>>> 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
© 2016 - 2024 Red Hat, Inc.