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

[Upadhyay, Neeraj]

On 8/18/2025 4:56 PM, Borislav Petkov wrote:
> 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.

Ok

> > 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.

Ok

> > +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.

Ok

> > +	 * 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.

Ok

> > +	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.

Ok, will remove.

> > +	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?

APIC_IRR register offsets are:

#Offset    #bits         Description

0x200      31:0         vectors 0-31
0x210      31:0         vectors 32-63
...
0x270      31:0         vectors 224-255

ALLOWED_IRR register offsets are:

#Offset    #bits         Description

0x204      31:0         vectors 0-31
0x214      31:0         vectors 32-63
...
0x274      31:0         vectors 224-255

IS_ALIGNED(reg, 16) is when 'reg' is an APIC_IRR register, which are 16 
byte aligned.

IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16) is for the case when 'reg' is
a SAVIC_ALLOWED_IRR register. which are at 16 byte strides from the 
SAVIC_ALLOWED_IRR base offset. Expected values of (reg - 
SAVIC_ALLOWED_IRR) are 0, 0x10, 0x20, ..., 0x70.

If both checks fail, that is a invalid offset ('!' is on the final ORed
value).

!(IS_ALIGNED(reg, 16) || IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16))




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

Maybe change it to below?

/*
 * Valid APIC_IRR/SAVIC_ALLOWED_IRR registers are at 16 bytes strides
 * from their respective base offset.
 */

if (WARN_ONCE(!(IS_ALIGNED(reg - APIC_IRR, 16) ||
                IS_ALIGNED(reg - SAVIC_ALLOWED_IRR, 16)),
              "Misaligned APIC_IRR/ALLOWED_IRR APIC register read 
offset 0x%x",
              reg))

> > +			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.

Ok, yes it is clearer.

- Neeraj