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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0cd2aa35-a12c-464d-85db-afa8d477d993@amd.com>
Date: Tue, 1 Apr 2025 15:55:50 +0530
From: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
To: Thomas Gleixner <tglx@...utronix.de>, linux-kernel@...r.kernel.org
Cc: bp@...en8.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
 Thomas.Lendacky@....com, nikunj@....com, Santosh.Shukla@....com,
 Vasant.Hegde@....com, Suravee.Suthikulpanit@....com, David.Kaplan@....com,
 x86@...nel.org, hpa@...or.com, peterz@...radead.org, seanjc@...gle.com,
 pbonzini@...hat.com, kvm@...r.kernel.org, kirill.shutemov@...ux.intel.com,
 huibo.wang@....com, naveen.rao@....com
Subject: Re: [RFC v2 06/17] x86/apic: Add support to send IPI for Secure AVIC



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