Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure AVIC

[Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>]

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