Re: [PATCH 3/3] x86/irq: introduce APIC_VECTOR_VALID

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Monday, March 17th, 2025 at 1:30 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:

>
>
> On 15.03.2025 02:00, dmkhn@xxxxxxxxx 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