Re: [PATCH 2/7] x86/apic: Add an SVSM APIC driver

[Melody Wang <huibo.wang@amd.com>]

Hi Tom,

On 7/31/26 10:39 AM, Tom Lendacky wrote:
>> +int svsm_do_call(struct svsm_call *call)
>> +{
>> +	call->caa = svsm_get_caa();
>> +	return svsm_perform_call_protocol(call);
>> +}
> 
> This helper is unneeded, all it does is set the calling area and then
> issue svsm_perform_call_protocol(). You're already setting other call
> values where you use this helper, so just add setting the calling area to
> those and directly issue svsm_perform_call_protocol().

Since svsm_perform_call_protocol() and svsm_get_caa() are all internal 
functions to coco (arch/x86/coco/sev/internal.h), I can not call them 
directly from the apic driver, so perhaps it is better that I have this 
helper function here.

>> diff --git a/arch/x86/kernel/apic/svsm_apic.c b/arch/x86/kernel/apic/svsm_apic.c
>> new file mode 100644
>> +static int svsm_apic_probe(void)
>> +{
>> +	if (!cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION) || !snp_vmpl)
> 
> Do you need both checks? Shouldn't the attribute only be set if the
> feature is set and the VMPL level is greater than 0?

Yes, the cc_platform_has() just checks if the sev_status has the 
MSR_AMD64_SNP_ALTERNATE_INJ bit set.

>> +	/* Alternate Injection and Secure AVIC are mutually exclusive */
>> +	if (cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
>> +		return 0;
> 
> What happens in this situation? Will Secure AVIC still work if Alternate
> Injection is enabled?

Secure AVIC and Alternate Injection are mutually exclusive, if the 
sev_status has secure AVIC set, we should not probe svsm apic:

"15.36.21.2 VMRUN and #VMEXIT
Secure AVIC mode is mutually exclusive with Restricted Injection, 
Alternate Injection, and
hypervisor controlled AVIC modes. If the SecureAvic bit is set to 1, and 
the AVIC Enable bit in the
VMCB is set to 1 or the RestrictedInjection or AlternateInjection bits 
in SEV_FEATURES are set to 1,
VMRUN will fail with #VMEXIT(VMEXIT_INVALID)."

I'm thinking I should probably check for secure AVIC first and if set, 
return 0.

>> +
>> +	if (!x2apic_mode) {
>> +		pr_err("Alternate Injection in non x2APIC mode impossible. Terminating.\n");
>> +		sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>> +	}
>> +
>> +	pr_info("Alternate Injection SVSM APIC enabled\n");
>> +
> 
> Should the probe routine query the SVSM for the APIC protocol?

Right now, the query for the APIC protocol is hardcoded as 0 which means 
basic APIC functionality related to interrupt delivery. So it is not 
needed to query the APIC protocol now. In the future, when the SVSM code 
changes with different set, we can adjust the guest code accordingly.

> 
>> +	return 1;
>> +}
>> +
>> +static int svsm_acpi_madt_oem_check(char *oem_id, char *oem_table_id)
>> +{
>> +	return x2apic_enabled() && cc_platform_has(CC_ATTR_SNP_ALTERNATE_INJECTION) && snp_vmpl;
>> +}
>> +
>> +static void svsm_apic_msr_write(u32 reg, u32 v)
>> +{
>> +	u32 msr = APIC_BASE_MSR + (reg >> 4);
>> +	struct svsm_call call = {};
>> +	int ret;
>> +
>> +	switch (reg) {
>> +	case APIC_ID:
>> +	case APIC_TASKPRI:
>> +	case APIC_PROCPRI:
>> +	case APIC_EOI:
>> +	case APIC_ISR ... APIC_ISR + 0x70:
>> +	case APIC_TMR ... APIC_TMR + 0x70:
>> +	case APIC_IRR ... APIC_IRR + 0x70:
>> +	case APIC_ICR:
>> +	case APIC_SELF_IPI:
>> +		call.rax = SVSM_APIC_CALL(SVSM_APIC_WRITE_REGISTER);
>> +		call.rcx = msr;
>> +		call.rdx = v;
>> +
>> +		ret = svsm_do_call(&call);
>> +		if (ret) {
>> +			pr_err("SVSM_APIC_WRITE_REGISTER: 0x%x, error: %d\n", reg, ret);
>> +			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
> 
> Can this be a new SEV_TERM_SET_LINUX value?
> 
>> +		}
>> +		break;
>> +	default:
>> +		pr_err("SVSM_APIC_WRITE_REGISTER 0x%x not supported\n", reg);
>> +		break;
>> +	}
>> +}
>> +
>> +static u32 svsm_apic_msr_read(u32 reg)
>> +{
>> +	u32 msr = APIC_BASE_MSR + (reg >> 4);
>> +	struct svsm_call call = {};
>> +	int ret;
>> +
>> +	switch (reg) {
>> +	case APIC_ID:
>> +	case APIC_TASKPRI:
>> +	case APIC_PROCPRI:
>> +	case APIC_EOI:
>> +	case APIC_ISR ... APIC_ISR + 0x70:
>> +	case APIC_TMR ... APIC_TMR + 0x70:
>> +	case APIC_IRR ... APIC_IRR + 0x70:
>> +	case APIC_ICR:
>> +	case APIC_SELF_IPI:
>> +		call.rax = SVSM_APIC_CALL(SVSM_APIC_READ_REGISTER);
>> +		call.rcx = msr;
>> +
>> +		ret = svsm_do_call(&call);
>> +		if (ret) {
>> +			pr_err("SVSM_APIC_READ_REGISTER: 0x%x, error: %d\n", reg, ret);
>> +			sev_es_terminate(SEV_TERM_SET_GEN, GHCB_SNP_UNSUPPORTED);
>> +		}
>> +		break;
>> +	default:
>> +		pr_err("SVSM_APIC_READ_REGISTER: 0x%x not supported\n", reg);
>> +		return 0;
>> +	}
>> +
>> +	return call.rdx_out;
>> +}
> 
> The read and write are very similar. Can you have a common function that
> takes a reg paramter, value parameter (that is input and output), and a
> mode parameter (read/write) and then have small read and write functions?

Yes, and Sashiko pointed that I need to prevent preemption for the caa 
call, I agree with it, but I think I should prevent interrupts here - I 
should do native_local_irq_save(), because there should not be any 
interrupts during a caa call as those things are not reentrant. Thoughts?

Thanks,
Melody