Message-ID: <871puizs2p.ffs@tglx>
Date: Thu, 27 Mar 2025 11:27:42 +0100
From: Thomas Gleixner <tglx@...utronix.de>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>, linux-kernel@...r.kernel.org
Cc: bp@...en8.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
 Thomas.Lendacky@....com, nikunj@....com, Santosh.Shukla@....com,
 Vasant.Hegde@....com, Suravee.Suthikulpanit@....com, David.Kaplan@....com,
 x86@...nel.org, hpa@...or.com, peterz@...radead.org, seanjc@...gle.com,
 pbonzini@...hat.com, kvm@...r.kernel.org, kirill.shutemov@...ux.intel.com,
 huibo.wang@....com, naveen.rao@....com
Subject: Re: [RFC v2 05/17] x86/apic: Add update_vector callback for Secure
 AVIC

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?

> 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.

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);

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

Powered by blists - more mailing lists