Re: [RFC v2 06/17] x86/apic: Add support to send IPI for Secure AVIC

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

On 3/21/2025 8:36 PM, Thomas Gleixner wrote:
> On Wed, Feb 26 2025 at 14:35, Neeraj Upadhyay wrote:
>> +	/* Self IPIs are accelerated by hardware, use wrmsr */
>> +	case APIC_SELF_IPI:
>> +		cfg = __prepare_ICR(APIC_DEST_SELF, data, 0);
>> +		native_x2apic_icr_write(cfg, 0);
>> +		break;
> 
> Please move this into a proper inline helper with a understandable
> comment and do not hide it in the maze of this write() wrapper.
> 

Ok.

>>  	/* ALLOWED_IRR offsets are writable */
>>  	case SAVIC_ALLOWED_IRR_OFFSET ... SAVIC_ALLOWED_IRR_OFFSET + 0x70:
>>  		if (IS_ALIGNED(reg - SAVIC_ALLOWED_IRR_OFFSET, 16)) {
>> @@ -154,13 +159,100 @@ static void x2apic_savic_write(u32 reg, u32 data)
>>  	}
>>  }
>>  
>> +static void send_ipi(int cpu, int vector)
> 
> Both are unsigned
> 

Will update this.

>> +{
>> +	void *backing_page;
>> +	int reg_off;
>> +
>> +	backing_page = per_cpu(apic_backing_page, cpu);
>> +	reg_off = APIC_IRR + REG_POS(vector);
>> +	/*
>> +	 * Use test_and_set_bit() to ensure that IRR updates are atomic w.r.t. other
>> +	 * IRR updates such as during VMRUN and during CPU interrupt handling flow.
>> +	 */
>> +	test_and_set_bit(VEC_POS(vector), (unsigned long *)((char *)backing_page + reg_off));
> 
> See previous mail.
> 
>> +}
>> +
>> +static void send_ipi_dest(u64 icr_data)
>> +{
>> +	int vector, cpu;
>> +
>> +	vector = icr_data & APIC_VECTOR_MASK;
>> +	cpu = icr_data >> 32;
> 
> Yes, converting from u64 to int is the proper conversion (NOT)
> 

Do I need to use unsigned int? I will update this.

>> +
>> +	send_ipi(cpu, vector);
>> +}
>> +
>> +static void send_ipi_target(u64 icr_data)
>> +{
>> +	if (icr_data & APIC_DEST_LOGICAL) {
>> +		pr_err("IPI target should be of PHYSICAL type\n");
>> +		return;
>> +	}
>> +
>> +	send_ipi_dest(icr_data);
>> +}
>> +
>> +static void send_ipi_allbut(u64 icr_data)
>> +{
>> +	const struct cpumask *self_cpu_mask = get_cpu_mask(smp_processor_id());
>> +	unsigned long flags;
>> +	int vector, cpu;
>> +
>> +	vector = icr_data & APIC_VECTOR_MASK;
>> +	local_irq_save(flags);
>> +	for_each_cpu_andnot(cpu, cpu_present_mask, self_cpu_mask)
>> +		send_ipi(cpu, vector);
>> +	savic_ghcb_msr_write(APIC_ICR, icr_data);
>> +	local_irq_restore(flags);
>> +}
>> +
>> +static void send_ipi_allinc(u64 icr_data)
>> +{
>> +	int vector;
>> +
>> +	send_ipi_allbut(icr_data);
>> +	vector = icr_data & APIC_VECTOR_MASK;
>> +	native_x2apic_icr_write(APIC_DEST_SELF | vector, 0);
>> +}
>> +
>> +static void x2apic_savic_icr_write(u32 icr_low, u32 icr_high)
>> +{
>> +	int dsh, vector;
>> +	u64 icr_data;
>> +
>> +	icr_data = ((u64)icr_high) << 32 | icr_low;
>> +	dsh = icr_low & APIC_DEST_ALLBUT;
>> +
>> +	switch (dsh) {
>> +	case APIC_DEST_SELF:
>> +		vector = icr_data & APIC_VECTOR_MASK;
> 
> So you construct icr_data first and then extract the vector from it,
> which is encoded in the low bits of icr_low.
> 
>> +		x2apic_savic_write(APIC_SELF_IPI, vector);
>> +		break;
>> +	case APIC_DEST_ALLINC:
>> +		send_ipi_allinc(icr_data);
> 
> And you do the same nonsense in all other functions. Oh well...
> 

I will clean this up in the next version.

- Neeraj

> Thanks,
> 
>         tglx