Re: [PATCH v1 3/8] x86/apic: Add an SVSM APIC driver

[Borislav Petkov]

In addition to the Sashiko review comments, some minor things below:

On Sat, Aug 29, 2026 at 03:39:41AM +0000, Melody Wang wrote:
> Alternate Injection replaces hypervisor-based interrupt queuing and
> event injection, requiring guest-controlled queuing and injection. In
> order to perform this guest-controlled queuing and injectiion, an SVSM

"injection"

Please use a spellchecker when writing commit messages.

> is used to update the guest VMSA to perform the required actions.
> 
> The guest uses the SVSM APIC protocol to communicate with the SVSM to
> perform selected APIC related operations instead of using standard APIC
> MSR access.
> 
> Add such a SVSM APIC driver (which implements a subset of an X2APIC),
> for the APIC emulation supported by the SVSM.
> 
> Signed-off-by: Melody Wang <huibo.wang@xxxxxxx>
> ---

...

> diff --git a/arch/x86/kernel/apic/Makefile b/arch/x86/kernel/apic/Makefile
> index 581db89477f9..23d83f1293e3 100644
> --- a/arch/x86/kernel/apic/Makefile
> +++ b/arch/x86/kernel/apic/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_SMP)		+= ipi.o
>  ifeq ($(CONFIG_X86_64),y)
>  # APIC probe will depend on the listing order here
>  obj-$(CONFIG_X86_NUMACHIP)	+= apic_numachip.o
> +obj-$(CONFIG_AMD_ALTERNATE_INJ)	+= x2apic_svsm.o

This should probably be after secure AVIC so that secure AVIC gets preferred
on machines that support it.

>  obj-$(CONFIG_X86_UV)		+= x2apic_uv_x.o
>  obj-$(CONFIG_AMD_SECURE_AVIC)	+= x2apic_savic.o
>  obj-$(CONFIG_X86_X2APIC)	+= x2apic_phys.o

...

> +static int svsm_apic_probe(void)
> +{
> +	if (cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION) && !snp_vmpl) {
> +		pr_err("Alternate Injection in VMPL0 impossible. Terminating.\n");
> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
> +	}
> +
> +	if (!cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION))
> +		return 0;

This check should come first, ofc. IOW,

	if (!cc...) {
		...
	} else {
		...
	}

IOW, something like this:

static int svsm_apic_probe(void)
{
        if (!cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION)) {
                return 0;
        } else {               
                if (!snp_vmpl) {
                        pr_err("Alternate Injection in VMPL0 impossible. Terminating.\n");
                        goto terminate;
                }       
        }
        
        if (!x2apic_mode) {
                pr_err("Alternate Injection in non x2APIC mode impossible. Terminating.\n");
                goto terminate;
        }
        
        pr_info("Alternate Injection SVSM APIC enabled\n");
        return 1;
        
terminate:
        sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
        return -EINVAL;
}

...

> +static inline u64 svsm_apic_icr_read(void)
> +{
> +	struct svsm_call call = {};
> +	u32 reg;
> +	int ret;
> +
> +	reg = APIC_ICR;
> +
> +	call.rax = SVSM_APIC_CALL(SVSM_APIC_READ_REGISTER);
> +	call.rcx = APIC_BASE_MSR + (reg >> 4);
> +
> +	ret = svsm_perform_call_protocol(&call);
> +	if (ret) {
> +		pr_err("svsm_apic_icr_read error: %d\n", ret);

Make that

		pr_err("%s: error: %d\n", __func__, ret);

> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
> +	}
> +
> +	return call.rdx_out;
> +}
> +
> +static void svsm_apic_icr_write(u32 low, u32 id)
> +{
> +	struct svsm_call call = {};
> +	u64 icr_data;
> +	u32 reg;
> +	int ret;
> +
> +	reg = APIC_ICR;
> +	icr_data = ((u64)id) << 32 | low;
> +
> +	call.rax = SVSM_APIC_CALL(SVSM_APIC_WRITE_REGISTER);
> +	call.rcx = APIC_BASE_MSR + (reg >> 4);
> +	call.rdx = icr_data;

You can do

	call.rdx = ((u64)id) << 32 | low;

and get rid of icr_data.

> +	ret = svsm_perform_call_protocol(&call);
> +	if (ret) {
> +		pr_err("svsm_apic_icr_write error: %d\n", ret);

Same as above:

		pr_err("%s: error: %d\n", __func__, ret);
	
> +		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_ALT_INJ_FAIL);
> +	}
> +}
> +
> +static void svsm_apic_send_IPI(int cpu, int vector)
> +{
> +	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
> +
> +	svsm_apic_icr_write(__prepare_ICR(0, vector, APIC_DEST_PHYSICAL), dest);
> +}
> +
> +static void __svsm_apic_send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
> +{
> +	unsigned long query_cpu;
> +	unsigned long this_cpu;

One line:

	unsigned long query_cpu, this_cpu;

> +
> +	guard(irqsave)();
> +
> +	this_cpu = smp_processor_id();
> +	for_each_cpu(query_cpu, mask) {
> +		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
> +			continue;
> +
> +		svsm_apic_send_IPI(query_cpu, vector);
> +	}
> +}
 

-- 
Regards/Gruss,
    Boris.

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