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
