linux-kernel - Re: [PATCH v9 03/18] x86/apic: Populate .read()/.write() callbacks of Secure AVIC driver

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: <20250818112650.GFaKMN-kR_4SLxrqov@fat_crate.local>
Date: Mon, 18 Aug 2025 13:26:50 +0200
From: Borislav Petkov <bp@...en8.de>
To: Neeraj Upadhyay <Neeraj.Upadhyay@....com>
Cc: linux-kernel@...r.kernel.org, tglx@...utronix.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, francescolavra.fl@...il.com,
	tiala@...rosoft.com
Subject: Re: [PATCH v9 03/18] x86/apic: Populate .read()/.write() callbacks
 of Secure AVIC driver

On Mon, Aug 11, 2025 at 03:14:29PM +0530, Neeraj Upadhyay wrote:
> Add read() and write() APIC callback functions to read and write x2APIC
> registers directly from the guest APIC backing page of a vCPU.
> 
> The x2APIC registers are mapped at an offset within the guest APIC
> backing page which is same as their x2APIC MMIO offset. Secure AVIC
> adds new registers such as ALLOWED_IRRs (which are at 4-byte offset
> within the IRR register offset range) and NMI_REQ to the APIC register
> space.
> 
> When Secure AVIC is enabled, guest's rdmsr/wrmsr of APIC registers
> result in VC exception (for non-accelerated register accesses) with

s/VC/#VC/g since you're talking about an exception vector.

> error code VMEXIT_AVIC_NOACCEL. The VC exception handler can read/write
> the x2APIC register in the guest APIC backing page to complete the
> rdmsr/wrmsr.

All x86 insns in caps pls: RDMSR/WRMSR.

> +static u32 savic_read(u32 reg)
> +{
> +	void *ap = this_cpu_ptr(secure_avic_page);
> +
> +	/*
> +	 * When Secure AVIC is enabled, rdmsr/wrmsr of APIC registers
> +	 * result in VC exception (for non-accelerated register accesses)
> +	 * with VMEXIT_AVIC_NOACCEL error code. The VC exception handler
> +	 * can read/write the x2APIC register in the guest APIC backing page.
> +	 * Since doing this would increase the latency of accessing x2APIC
> +	 * registers, instead of doing rdmsr/wrmsr based accesses and
> +	 * handling apic register reads/writes in VC exception, the read()

s/apic/APIC/g

Please be consistent across the whole set. Acronyms are in all caps. Insn
names too.

> +	 * and write() callbacks directly read/write APIC register from/to
> +	 * the vCPU APIC backing page.
> +	 */

Move that comment above the function. And also split it in paragraphs: when it
becomes more than 4-5 lines, split the next one with a new line.

> +	switch (reg) {
> +	case APIC_LVTT:
> +	case APIC_TMICT:
> +	case APIC_TMCCT:
> +	case APIC_TDCR:
> +	case APIC_ID:
> +	case APIC_LVR:
> +	case APIC_TASKPRI:
> +	case APIC_ARBPRI:
> +	case APIC_PROCPRI:
> +	case APIC_LDR:
> +	case APIC_SPIV:
> +	case APIC_ESR:
> +	case APIC_LVTTHMR:
> +	case APIC_LVTPC:
> +	case APIC_LVT0:
> +	case APIC_LVT1:
> +	case APIC_LVTERR:
> +	case APIC_EFEAT:
> +	case APIC_ECTRL:
> +	case APIC_SEOI:
> +	case APIC_IER:
> +	case APIC_EILVTn(0) ... APIC_EILVTn(3):
> +		return apic_get_reg(ap, reg);
> +	case APIC_ICR:
> +		return (u32) apic_get_reg64(ap, reg);
			    ^

no need for that space.

> +	case APIC_ISR ... APIC_ISR + 0x70:
> +	case APIC_TMR ... APIC_TMR + 0x70:
> +		if (WARN_ONCE(!IS_ALIGNED(reg, 16),
> +			      "APIC reg read offset 0x%x not aligned at 16 bytes", reg))
> +			return 0;
> +		return apic_get_reg(ap, reg);
> +	/* IRR and ALLOWED_IRR offset range */
> +	case APIC_IRR ... APIC_IRR + 0x74:
> +		/*
> +		 * Either aligned at 16 bytes for valid IRR reg offset or a
> +		 * valid Secure AVIC ALLOWED_IRR offset.
> +		 */
> +		if (WARN_ONCE(!(IS_ALIGNED(reg, 16) ||
> +				IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16)),
> +			      "Misaligned IRR/ALLOWED_IRR APIC reg read offset 0x%x", reg))

What is that second thing supposed to catch?

reg can be aligned to 16 but reg - SAVIC_ALLOWED_IRR cannot be?

I can't follow the comment... perhaps write it out and not try to be clever.

> +			return 0;
> +		return apic_get_reg(ap, reg);
> +	default:
> +		pr_err("Permission denied: read of Secure AVIC reg offset 0x%x\n", reg);

Permission denied?

"Error reading unknown Secure AVIC reg offset..."

I'd say.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette