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

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

On 4/17/2025 4:20 PM, Thomas Gleixner wrote:
> 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
> 

Ok

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

Ok

>> @@ -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?
>

apic_update_vector() is needed for some system vectors which are emulated/injected
by Hypervisor. Patch 8 calls it for lapic timer. HYPERVISOR_CALLBACK_VECTOR would need
it for hyperv. This patch series does not call apic_update_vector() for
HYPERVISOR_CALLBACK_VECTOR though.

Given that currently this callback is not used outside of apic code,
do I need to add it to arch/x86/kernel/apic/local.h or just remove it and use
conditional call in current callsites?
 
   
>> -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(...);
> 

Ok

>> +
>> +	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()

Ok, this was done to associate apic->update_vector() calls with the
setup (irq_matrix_alloc()) and teardown (irq_matrix_free()) of vector,
which was my understanding from your suggestion here: 
https://lore.kernel.org/lkml/87jz8i31dv.ffs@tglx/ . Given that there
is single callsite for each and vector_configure_legacy() calls
apic->update_vector() outside of alloc path, adding it to apic_update_irq_cfg(),
as you suggest handles all callers.


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

Yes, makes sense. apic_update_irq_cfg() is called for MANAGED_IRQ_SHUTDOWN_VECTOR
also. I am not aware of the use case of that vector. Whethere it is an interrupt
which is injected by Hypervisor.


static void vector_assign_managed_shutdown(struct irq_data *irqd)
{
        unsigned int cpu = cpumask_first(cpu_online_mask);

        apic_update_irq_cfg(irqd, MANAGED_IRQ_SHUTDOWN_VECTOR, cpu);
}


>> +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()
> 

Ok

> Something like the uncompiled below, no?
> 

Ok, makes sense. Looks good.


- Neeraj

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