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

Thomas Gleixner
2025-03-21T14:27:10Z

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