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

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
> Add update_vector callback to set/clear ALLOWED_IRR field in
> the APIC backing page. The ALLOWED_IRR field indicates the
> interrupt vectors which the guest allows the hypervisor to
> send (typically for emulated devices). Interrupt vectors used
> exclusively by the guest itself (like IPI vectors) should not
> be allowed to be injected into the guest for security reasons.
> The update_vector callback is invoked from APIC vector domain
> whenever a vector is allocated, freed or moved.

Your changelog tells a lot about the WHAT. Please read and follow the
documentation, which describes how a change log should be structured.

https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog

> diff --git a/arch/x86/kernel/apic/vector.c b/arch/x86/kernel/apic/vector.c
> index 72fa4bb78f0a..e0c9505e05f8 100644
> --- a/arch/x86/kernel/apic/vector.c
> +++ b/arch/x86/kernel/apic/vector.c
> @@ -174,6 +174,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>  		apicd->prev_cpu = apicd->cpu;
>  		WARN_ON_ONCE(apicd->cpu == newcpu);
>  	} else {
> +		if (apic->update_vector)
> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>  		irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
>  				managed);
>  	}
> @@ -183,6 +185,8 @@ static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
>  	apicd->cpu = newcpu;
>  	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
>  	per_cpu(vector_irq, newcpu)[newvec] = desc;
> +	if (apic->update_vector)
> +		apic->update_vector(apicd->cpu, apicd->vector, true);

A trivial

static inline void apic_update_vector(....)
{
        if (apic->update_vector)
           ....
}

would be too easy to read and add not enough line count, right?

>  static void vector_assign_managed_shutdown(struct irq_data *irqd)
> @@ -528,11 +532,15 @@ static bool vector_configure_legacy(unsigned int virq, struct irq_data *irqd,
>  	if (irqd_is_activated(irqd)) {
>  		trace_vector_setup(virq, true, 0);
>  		apic_update_irq_cfg(irqd, apicd->vector, apicd->cpu);
> +		if (apic->update_vector)
> +			apic->update_vector(apicd->cpu, apicd->vector, true);
>  	} else {
>  		/* Release the vector */
>  		apicd->can_reserve = true;
>  		irqd_set_can_reserve(irqd);
>  		clear_irq_vector(irqd);
> +		if (apic->update_vector)
> +			apic->update_vector(apicd->cpu, apicd->vector, false);
>  		realloc = true;

This is as incomplete as it gets. None of the other code paths which
invoke clear_irq_vector() nor those which invoke free_moved_vector() are
mopping up the leftovers in the backing page.

And no, you don't sprinkle this nonsense all over the call sites. There
is only a very limited number of functions which are involed in setting
up and tearing down a vector. Doing this at the call sites is a
guarantee for missing out as you demonstrated.

> +#define VEC_POS(v)	((v) & (32 - 1))
> +#define REG_POS(v)	(((v) >> 5) << 4)

This is unreadable, undocumented and incomprehensible garbage.

>  static DEFINE_PER_CPU(void *, apic_backing_page);
>  
>  struct apic_id_node {
> @@ -192,6 +195,22 @@ 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)
> +{
> +	void *backing_page;
> +	unsigned long *reg;
> +	int reg_off;
> +
> +	backing_page = per_cpu(apic_backing_page, cpu);
> +	reg_off = SAVIC_ALLOWED_IRR_OFFSET + REG_POS(vector);
> +	reg = (unsigned long *)((char *)backing_page + reg_off);
> +
> +	if (set)
> +		test_and_set_bit(VEC_POS(vector), reg);
> +	else
> +		test_and_clear_bit(VEC_POS(vector), reg);
> +}

What's the test_and_ for if you ignore the return value anyway?

Als I have no idea what SAVIC_ALLOWED_IRR_OFFSET means. Whether it's
something from the datashit or a made up thing does not matter. It's
patently non-informative.

Again:

struct apic_page {
	union {
		u32	regs[NR_APIC_REGS];
		u8	bytes[PAGE_SIZE];
	};
};                

       struct apic_page *ap = this_cpu_ptr(apic_page);
       unsigned long *sirr;

       /*
        * apic_page.regs[SAVIC_ALLOWED_IRR_OFFSET...] is an array of
        * consecutive 32-bit registers, which represents a vector bitmap.
        */
        sirr = (unsigned long *) &ap->regs[SAVIC_ALLOWED_IRR_OFFSET];
        if (set)
        	set_bit(sirr, vector);
        else
        	clear_bit(sirr, vector);

See how code suddenly becomes self explaining, obvious and
comprehensible?

Thanks,

        tglx