| Message ID | 20250315010033.2917197-4-dmukhin@ford.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/irq: cleanup use of open-coded values | expand |
On 15.03.2025 02:00, dmkhn@proton.me wrote: > Add new symbol APIC_VECTOR_VALID to replace open-coded value 16 in > LAPIC and virtual LAPIC code. First a good name is needed to make such a change. APIC_VECTOR_VALID could imo be the name of a predicate macro, but it can't be a mere number. Then ... > --- a/xen/arch/x86/cpu/mcheck/mce_intel.c > +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c > @@ -136,7 +136,7 @@ static void intel_init_thermal(struct cpuinfo_x86 *c) > * is required to set the same value for all threads/cores). > */ > if ( (val & APIC_DM_MASK) != APIC_DM_FIXED > - || (val & APIC_VECTOR_MASK) > 0xf ) > + || (val & APIC_VECTOR_MASK) > APIC_VECTOR_VALID ) ... care needs to be taken that replacements are done such that the "no functional change" claim is actually correct. (The 0xf, i.e. 15, is replaced by 16 here. I didn't check if there are other similar issues.) > --- a/xen/arch/x86/include/asm/apicdef.h > +++ b/xen/arch/x86/include/asm/apicdef.h > @@ -78,6 +78,7 @@ > #define APIC_DM_STARTUP 0x00600 > #define APIC_DM_EXTINT 0x00700 > #define APIC_VECTOR_MASK 0x000FF > +#define APIC_VECTOR_VALID (16) > #define APIC_ICR2 0x310 Nit: No real need for parentheses here; adjacent #define-s don't have any, so it's a little hard to see why you added them. Jan
On Monday, March 17th, 2025 at 1:30 AM, Jan Beulich <jbeulich@suse.com> wrote: > > > On 15.03.2025 02:00, dmkhn@proton.me wrote: > > > Add new symbol APIC_VECTOR_VALID to replace open-coded value 16 in > > LAPIC and virtual LAPIC code. > > > First a good name is needed to make such a change. APIC_VECTOR_VALID > could imo be the name of a predicate macro, but it can't be a mere > number. Do you think something like #define APIC_VECTOR_VALID_START 16 #define APIC_VECTOR_VALID_END 255 will be satisfactory names to use? > > Then ... > > > --- a/xen/arch/x86/cpu/mcheck/mce_intel.c > > +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c > > @@ -136,7 +136,7 @@ static void intel_init_thermal(struct cpuinfo_x86 *c) > > * is required to set the same value for all threads/cores). > > */ > > if ( (val & APIC_DM_MASK) != APIC_DM_FIXED > > - || (val & APIC_VECTOR_MASK) > 0xf ) > > + || (val & APIC_VECTOR_MASK) > APIC_VECTOR_VALID ) > > > ... care needs to be taken that replacements are done such that the > "no functional change" claim is actually correct. (The 0xf, i.e. 15, > is replaced by 16 here. I didn't check if there are other similar > issues.) My bad. Thanks for the catch. > > > --- a/xen/arch/x86/include/asm/apicdef.h > > +++ b/xen/arch/x86/include/asm/apicdef.h > > @@ -78,6 +78,7 @@ > > #define APIC_DM_STARTUP 0x00600 > > #define APIC_DM_EXTINT 0x00700 > > #define APIC_VECTOR_MASK 0x000FF > > +#define APIC_VECTOR_VALID (16) > > #define APIC_ICR2 0x310 > > > Nit: No real need for parentheses here; adjacent #define-s don't have > any, so it's a little hard to see why you added them. My understanding was MISRA requires parentheses around expressions resulting from the expansion of a macro. After double checking, such requirement only applicable to macros w/ parameters. > > Jan
On 19.03.2025 00:42, Denis Mukhin wrote: > On Monday, March 17th, 2025 at 1:30 AM, Jan Beulich <jbeulich@suse.com> wrote: >> On 15.03.2025 02:00, dmkhn@proton.me wrote: >> >>> Add new symbol APIC_VECTOR_VALID to replace open-coded value 16 in >>> LAPIC and virtual LAPIC code. >> >> >> First a good name is needed to make such a change. APIC_VECTOR_VALID >> could imo be the name of a predicate macro, but it can't be a mere >> number. > > Do you think something like > > #define APIC_VECTOR_VALID_START 16 > #define APIC_VECTOR_VALID_END 255 > > will be satisfactory names to use? Maybe. In some of the places the _END one may be awkward to use, though. If you absolutely want to replace those numbers, imo the previously alluded to APIC_VECTOR_VALID() predicate macro may be a better thing to use. Personally, despite generally disliking plain literal numbers, I'm not overly concerned of the ones here. Jan
diff --git a/xen/arch/x86/cpu/mcheck/mce_intel.c b/xen/arch/x86/cpu/mcheck/mce_intel.c index 07b50f8793..e8c252e03a 100644 --- a/xen/arch/x86/cpu/mcheck/mce_intel.c +++ b/xen/arch/x86/cpu/mcheck/mce_intel.c @@ -136,7 +136,7 @@ static void intel_init_thermal(struct cpuinfo_x86 *c) * is required to set the same value for all threads/cores). */ if ( (val & APIC_DM_MASK) != APIC_DM_FIXED - || (val & APIC_VECTOR_MASK) > 0xf ) + || (val & APIC_VECTOR_MASK) > APIC_VECTOR_VALID ) apic_write(APIC_LVTTHMR, val); if ( (msr_content & (1ULL<<3)) diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c index 065b2aab5b..a0f46e540c 100644 --- a/xen/arch/x86/hvm/vlapic.c +++ b/xen/arch/x86/hvm/vlapic.c @@ -123,7 +123,7 @@ static void vlapic_error(struct vlapic *vlapic, unsigned int err_bit) * will end up back here. Break the cycle by only injecting LVTERR * if it will succeed, and folding in RECVILL otherwise. */ - if ( (lvterr & APIC_VECTOR_MASK) >= 16 ) + if ( (lvterr & APIC_VECTOR_MASK) >= APIC_VECTOR_VALID ) inj = true; else set_bit(ilog2(APIC_ESR_RECVILL), &vlapic->hw.pending_esr); @@ -136,7 +136,7 @@ static void vlapic_error(struct vlapic *vlapic, unsigned int err_bit) bool vlapic_test_irq(const struct vlapic *vlapic, uint8_t vec) { - if ( unlikely(vec < 16) ) + if ( unlikely(vec < APIC_VECTOR_VALID) ) return false; if ( hvm_funcs.test_pir && @@ -150,7 +150,7 @@ void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig) { struct vcpu *target = vlapic_vcpu(vlapic); - if ( unlikely(vec < 16) ) + if ( unlikely(vec < APIC_VECTOR_VALID) ) { vlapic_error(vlapic, ilog2(APIC_ESR_RECVILL)); return; @@ -523,7 +523,7 @@ void vlapic_ipi( struct vlapic *target = vlapic_lowest_prio( vlapic_domain(vlapic), vlapic, short_hand, dest, dest_mode); - if ( unlikely((icr_low & APIC_VECTOR_MASK) < 16) ) + if ( unlikely((icr_low & APIC_VECTOR_MASK) < APIC_VECTOR_VALID) ) vlapic_error(vlapic, ilog2(APIC_ESR_SENDILL)); else if ( target ) vlapic_accept_irq(vlapic_vcpu(target), icr_low); @@ -531,7 +531,7 @@ void vlapic_ipi( } case APIC_DM_FIXED: - if ( unlikely((icr_low & APIC_VECTOR_MASK) < 16) ) + if ( unlikely((icr_low & APIC_VECTOR_MASK) < APIC_VECTOR_VALID) ) { vlapic_error(vlapic, ilog2(APIC_ESR_SENDILL)); break; diff --git a/xen/arch/x86/include/asm/apicdef.h b/xen/arch/x86/include/asm/apicdef.h index 49e29ec801..7750583481 100644 --- a/xen/arch/x86/include/asm/apicdef.h +++ b/xen/arch/x86/include/asm/apicdef.h @@ -78,6 +78,7 @@ #define APIC_DM_STARTUP 0x00600 #define APIC_DM_EXTINT 0x00700 #define APIC_VECTOR_MASK 0x000FF +#define APIC_VECTOR_VALID (16) #define APIC_ICR2 0x310 #define GET_xAPIC_DEST_FIELD(x) (((x)>>24)&0xFF) #define SET_xAPIC_DEST_FIELD(x) ((x)<<24)
Add new symbol APIC_VECTOR_VALID to replace open-coded value 16 in LAPIC and virtual LAPIC code. See: Intel SDM volume 3A Chapter "ADVANCED PROGRAMMABLE INTERRUPT CONTROLLER" Section "Valid Interrupt Vectors" No functional change. Signed-off-by: Denis Mukhin <dmukhin@ford.com> --- xen/arch/x86/cpu/mcheck/mce_intel.c | 2 +- xen/arch/x86/hvm/vlapic.c | 10 +++++----- xen/arch/x86/include/asm/apicdef.h | 1 + 3 files changed, 7 insertions(+), 6 deletions(-)