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
