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

["Upadhyay, Neeraj" <neeraj.upadhyay@amd.com>]

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