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

[Borislav Petkov]

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