Re: [PATCH v4 06/18] x86/apic: Add update_vector callback for Secure AVIC

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, Apr 17 2025 at 14:46, Neeraj Upadhyay wrote:
> Add update_vector callback to set/clear ALLOWED_IRR field in
> a vCPU's APIC backing page for external vectors. 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 and the vectors which
> are not emulated by the hypervisor, such as IPI vectors, are part
> of system vectors and are not set in the ALLOWED_IRR.

Please structure changelogs properly in paragraphs:

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

>  arch/x86/include/asm/apic.h         |  9 +++++
>  arch/x86/kernel/apic/vector.c       | 53 ++++++++++++++++++++++-------
>  arch/x86/kernel/apic/x2apic_savic.c | 35 +++++++++++++++++++

And split this patch up into two:

  1) Do the modifications in vector.c which is what the $Subject line
     says

  2) Add the SAVIC specific bits

> @@ -471,6 +473,12 @@ static __always_inline bool apic_id_valid(u32 apic_id)
>  	return apic_id <= apic->max_apic_id;
>  }
>  
> +static __always_inline void apic_update_vector(unsigned int cpu, unsigned int vector, bool set)
> +{
> +	if (apic->update_vector)
> +		apic->update_vector(cpu, vector, set);
> +}

This is in the public header because it can?
  
> -static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
> -			       unsigned int newcpu)
> +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);

        int vector = irq_matrix_alloc(...);

> +
> +	if (vector >= 0)
> +		apic_update_vector(*cpu, vector, true);
> +
> +	return vector;
> +}
> +
> +static int irq_alloc_managed_vector(unsigned int *cpu)
> +{
> +	int vector;
> +
> +	vector = irq_matrix_alloc_managed(vector_matrix, vector_searchmask, cpu);
> +
> +	if (vector >= 0)
> +		apic_update_vector(*cpu, vector, true);
> +
> +	return vector;
> +}

I completely fail to see the value of these two functions. Each of them
has exactly _ONE_ call site and both sites invoke apic_update_vector()
when the allocation succeeded. Why can't you just do the obvious and
leave the existing code alone and add

      if (apic->update_vector)
      		apic->update_vector();

into apic_update_vector()? But then you have another place where you
need the update, which does not invoke apic_update_vector().

Now if you look deeper, then you notice that all places which invoke
apic_update_vector() invoke apic_update_irq_cfg(), which is also called
at this other place, no?

> +static void irq_free_vector(unsigned int cpu, unsigned int vector, bool managed)
> +{
> +	apic_update_vector(cpu, vector, false);
> +	irq_matrix_free(vector_matrix, cpu, vector, managed);
> +}

This one makes sense, but please name it: apic_free_vector()

Something like the uncompiled below, no?

Thanks,

        tglx
---
--- a/arch/x86/kernel/apic/vector.c
+++ b/arch/x86/kernel/apic/vector.c
@@ -134,9 +134,19 @@ static void apic_update_irq_cfg(struct i
 
 	apicd->hw_irq_cfg.vector = vector;
 	apicd->hw_irq_cfg.dest_apicid = apic->calc_dest_apicid(cpu);
+
+	if (apic->update_vector)
+		apic->update_vector(cpu, vector, true);
+
 	irq_data_update_effective_affinity(irqd, cpumask_of(cpu));
-	trace_vector_config(irqd->irq, vector, cpu,
-			    apicd->hw_irq_cfg.dest_apicid);
+	trace_vector_config(irqd->irq, vector, cpu, apicd->hw_irq_cfg.dest_apicid);
+}
+
+static void apic_free_vector(unsigned int cpu, unsigned int vector, bool managed)
+{
+	if (apic->update_vector)
+		apic->update_vector(cpu, vector, false);
+	irq_matrix_free(vector_matrix, cpu, vector, managed);
 }
 
 static void apic_update_vector(struct irq_data *irqd, unsigned int newvec,
@@ -174,8 +184,7 @@ static void apic_update_vector(struct ir
 		apicd->prev_cpu = apicd->cpu;
 		WARN_ON_ONCE(apicd->cpu == newcpu);
 	} else {
-		irq_matrix_free(vector_matrix, apicd->cpu, apicd->vector,
-				managed);
+		apic_free_vector(apicd->cpu, apicd->vector, managed);
 	}
 
 setnew:
@@ -183,6 +192,7 @@ static void apic_update_vector(struct ir
 	apicd->cpu = newcpu;
 	BUG_ON(!IS_ERR_OR_NULL(per_cpu(vector_irq, newcpu)[newvec]));
 	per_cpu(vector_irq, newcpu)[newvec] = desc;
+	apic_update_irq_cfg(irqd, newvec, newcpu);
 }
 
 static void vector_assign_managed_shutdown(struct irq_data *irqd)
@@ -261,8 +271,6 @@ assign_vector_locked(struct irq_data *ir
 	if (vector < 0)
 		return vector;
 	apic_update_vector(irqd, vector, cpu);
-	apic_update_irq_cfg(irqd, vector, cpu);
-
 	return 0;
 }
 
@@ -338,7 +346,6 @@ assign_managed_vector(struct irq_data *i
 	if (vector < 0)
 		return vector;
 	apic_update_vector(irqd, vector, cpu);
-	apic_update_irq_cfg(irqd, vector, cpu);
 	return 0;
 }
 
@@ -357,7 +364,7 @@ static void clear_irq_vector(struct irq_
 			   apicd->prev_cpu);
 
 	per_cpu(vector_irq, apicd->cpu)[vector] = VECTOR_SHUTDOWN;
-	irq_matrix_free(vector_matrix, apicd->cpu, vector, managed);
+	apic_free_vector(apicd->cpu, vector, managed);
 	apicd->vector = 0;
 
 	/* Clean up move in progress */
@@ -366,7 +373,7 @@ static void clear_irq_vector(struct irq_
 		return;
 
 	per_cpu(vector_irq, apicd->prev_cpu)[vector] = VECTOR_SHUTDOWN;
-	irq_matrix_free(vector_matrix, apicd->prev_cpu, vector, managed);
+	apic_free_vector(apicd->prev_cpu, vector, managed);
 	apicd->prev_vector = 0;
 	apicd->move_in_progress = 0;
 	hlist_del_init(&apicd->clist);
@@ -905,7 +912,7 @@ static void free_moved_vector(struct api
 	 *    affinity mask comes online.
 	 */
 	trace_vector_free_moved(apicd->irq, cpu, vector, managed);
-	irq_matrix_free(vector_matrix, cpu, vector, managed);
+	apic_free_vector(cpu, vector, managed);
 	per_cpu(vector_irq, cpu)[vector] = VECTOR_UNUSED;
 	hlist_del_init(&apicd->clist);
 	apicd->prev_vector = 0;