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

[Thomas Gleixner <tglx@linutronix.de>]

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.

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

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

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

Thanks,

        tglx