On 3/27/2025 3:57 PM, Thomas Gleixner wrote:
> On Tue, Mar 25 2025 at 17:40, Neeraj Upadhyay wrote:
>> On 3/21/2025 9:05 PM, Neeraj Upadhyay wrote:
>> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
>> index 736f62812f5c..fef6faffeed1 100644
>> --- a/arch/x86/kernel/apic/vector.c
>> +++ b/arch/x86/kernel/apic/vector.c
>> @@ -139,6 +139,46 @@ static void apic_update_irq_cfg(struct irq_data *irqd, unsigned int vector,
>> apicd->hw_irq_cfg.dest_apicid);
>> }
>>
>> +static inline void apic_drv_update_vector(unsigned int cpu, unsigned int vector, bool set)
>> +{
>> + if (apic->update_vector)
>> + apic->update_vector(cpu, vector, set);
>> +}
>> +
>> +static int irq_alloc_vector(const struct cpumask *dest, bool resvd, unsigned int *cpu)
>> +{
>> + int vector;
>> +
>> + vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);
>> +
>> + if (vector < 0)
>> + return vector;
>> +
>> + apic_drv_update_vector(*cpu, vector, true);
>> +
>> + return vector;
>> +}
>
> static int irq_alloc_vector(const struct cpumask *dest, bool resvd, unsigned int *cpu)
> {
> int vector = irq_matrix_alloc(vector_matrix, dest, resvd, cpu);
>
> if (vector > 0)
> apic_drv_update_vector(*cpu, vector, true);
> return vector;
> }
>
> Perhaps?
>
Sounds good.
>> After checking more on this, set_bit(vector, ) cannot be used directly here, as
>> 32-bit registers are not consecutive. Each register is aligned at 16 byte
>> boundary.
>
> Fair enough.
>
>> So, I changed it to below:
>>
>> --- a/arch/x86/kernel/apic/x2apic_savic.c
>> +++ b/arch/x86/kernel/apic/x2apic_savic.c
>> @@ -19,6 +19,26 @@
>>
>> /* APIC_EILVTn(3) is the last defined APIC register. */
>> #define NR_APIC_REGS (APIC_EILVTn(4) >> 2)
>> +/*
>> + * APIC registers such as APIC_IRR, APIC_ISR, ... are mapped as
>> + * 32-bit registers and are aligned at 16-byte boundary. For
>> + * example, APIC_IRR registers mapping looks like below:
>> + *
>> + * #Offset #bits Description
>> + * 0x200 31:0 vectors 0-31
>> + * 0x210 31:0 vectors 32-63
>> + * ...
>> + * 0x270 31:0 vectors 224-255
>> + *
>> + * VEC_BIT_POS gives the bit position of a vector in the APIC
>> + * reg containing its state.
>> + */
>> +#define VEC_BIT_POS(v) ((v) & (32 - 1))
>> +/*
>> + * VEC_REG_OFF gives the relative (from the start offset of that APIC
>> + * register) offset of the APIC register containing state for a vector.
>> + */
>> +#define VEC_REG_OFF(v) (((v) >> 5) << 4)
>>
>> struct apic_page {
>> union {
>> @@ -185,6 +205,35 @@ static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, in
>> __send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>> }
>>
>> +static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
>> +{
>> + struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
>> + unsigned long *sirr;
>> + int vec_bit;
>> + int reg_off;
>> +
>> + /*
>> + * ALLOWED_IRR registers are mapped in the apic_page at below byte
>> + * offsets. Each register is a 32-bit register aligned at 16-byte
>> + * boundary.
>> + *
>> + * #Offset #bits Description
>> + * SAVIC_ALLOWED_IRR_OFFSET 31:0 Guest allowed vectors 0-31
>> + * "" + 0x10 31:0 Guest allowed vectors 32-63
>> + * ...
>> + * "" + 0x70 31:0 Guest allowed vectors 224-255
>> + *
>> + */
>> + reg_off = SAVIC_ALLOWED_IRR_OFFSET + VEC_REG_OFF(vector);
>> + sirr = (unsigned long *) &ap->regs[reg_off >> 2];
>> + vec_bit = VEC_BIT_POS(vector);
>> +
>> + if (set)
>> + set_bit(vec_bit, sirr);
>> + else
>> + clear_bit(vec_bit, sirr);
>> +}
>
> If you need 20 lines of horrific comments to explain incomprehensible
> macros and code, then something is fundamentally wrong. Then you want to
> sit back and think about whether this can't be expressed in simple and
> obvious ways. Let's look at the math.
>
Hmm, indeed. I will keep this in mind, thanks!
> The relevant registers are starting at regs[SAVIC_ALLOWED_IRR]. Due to
> the 16-byte alignment the vector number obviously cannot be used for
> linear bitmap addressing.
>
> But the resulting bit number can be trivially calculated with:
>
> bit = vector + 32 * (vector / 32);
>
Somehow, this math is not working for me. I will think more on how this
works. From what I understand, bit number is:
bit = vector % 32 + (vector / 32) * 16 * 8
So, for example, vector number 32, bit number need to be 128.
With you formula, it comes as 64.
- Neeraj
> which can be converted to:
>
> bit = vector + (vector & ~0x1f);
>
> That conversion should be done by any reasonable compiler.
>
> Ergo the whole thing can be condensed to:
>
> static void x2apic_savic_update_vector(unsigned int cpu, unsigned int vector, bool set)
> {
> struct apic_page *ap = per_cpu_ptr(apic_backing_page, cpu);
> unsigned long *sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR];
>
> /*
> * The registers are 32-bit wide and 16-byte aligned.
> * Compensate for the resulting bit number spacing.
> */
> unsigned int bit = vector + 32 * (vector / 32);
>
> if (set)
> set_bit(vec_bit, sirr);
> else
> clear_bit(vec_bit, sirr);
> }
>
> Two comment lines plus one line of trivial math makes this
> comprehensible and obvious. No?
>
> If you need that adjustment for other places as well, then you can
> provide a trivial and documented inline function for it.
>
> Thanks,
>
> tglx
