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
