Re: [RFC v2 01/17] x86/apic: Add new driver for Secure AVIC

[Neeraj Upadhyay <Neeraj.Upadhyay@amd.com>]

On 3/20/2025 9:21 PM, Borislav Petkov wrote:
> On Wed, Feb 26, 2025 at 02:35:09PM +0530, Neeraj Upadhyay wrote:
>> +config AMD_SECURE_AVIC
>> +	bool "AMD Secure AVIC"
>> +	depends on X86_X2APIC
>> +	help
>> +	  This enables AMD Secure AVIC support on guests that have this feature.
> 
> "Enable this to get ..."
> 

Will update.

>> +	  AMD Secure AVIC provides hardware acceleration for performance sensitive
>> +	  APIC accesses and support for managing guest owned APIC state for SEV-SNP
>> +	  guests. Secure AVIC does not support xapic mode. It has functional
>> +	  dependency on x2apic being enabled in the guest.
>> +
>> +	  If you don't know what to do here, say N.
>> +
>>  config X86_POSTED_MSI
>>  	bool "Enable MSI and MSI-x delivery by posted interrupts"
>>  	depends on X86_64 && IRQ_REMAP
>> @@ -1557,6 +1570,7 @@ config AMD_MEM_ENCRYPT
>>  	select X86_MEM_ENCRYPT
>>  	select UNACCEPTED_MEMORY
>>  	select CRYPTO_LIB_AESGCM
>> +	select AMD_SECURE_AVIC
> 
> AMD_MEM_ENCRYPT doesn't absolutely need AMD_SECURE_AVIC so this can go.
> 

The intent here is to prevent a configuration where CONFIG_AMD_SECURE_AVIC
is disabled in build  and sev_status (features enabled in hypervisor) says Secure
AVIC is enabled. In this configuration, while SNP_FEATURES_PRESENT says
Secure AVIC feature is present in guest and snp_get_unsupported_features()
would not flag mismatched features between host and guest, guest would boot
without Secure AVIC apic driver being selected. Do you think we should
handle this case differently and not force select AMD_SECURE_AVIC config
when AMD_MEM_ENCRYPT config is enabled?

#define SNP_FEATURES_PRESENT    (MSR_AMD64_SNP_DEBUG_SWAP |     \
                                 MSR_AMD64_SNP_SECURE_TSC |     \
                                 MSR_AMD64_SNP_SECURE_AVIC)

u64 snp_get_unsupported_features(u64 status)
{
        if (!(status & MSR_AMD64_SEV_SNP_ENABLED))
                return 0;

        return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
}



>>  	help
>>  	  Say yes to enable support for the encryption of system memory.
>>  	  This requires an AMD processor that supports Secure Memory
> 
> ...
> 
>> +static void x2apic_savic_send_IPI(int cpu, int vector)
>> +{
>> +	u32 dest = per_cpu(x86_cpu_to_apicid, cpu);
>> +
>> +	/* x2apic MSRs are special and need a special fence: */
>> +	weak_wrmsr_fence();
>> +	__x2apic_send_IPI_dest(dest, vector, APIC_DEST_PHYSICAL);
>> +}
>> +
>> +static void
> 
> Unnecessary line break.
> 

Will update.

>> +__send_IPI_mask(const struct cpumask *mask, int vector, int apic_dest)
>> +{
>> +	unsigned long query_cpu;
>> +	unsigned long this_cpu;
>> +	unsigned long flags;
>> +
>> +	/* x2apic MSRs are special and need a special fence: */
>> +	weak_wrmsr_fence();
>> +
>> +	local_irq_save(flags);
>> +
>> +	this_cpu = smp_processor_id();
>> +	for_each_cpu(query_cpu, mask) {
>> +		if (apic_dest == APIC_DEST_ALLBUT && this_cpu == query_cpu)
>> +			continue;
>> +		__x2apic_send_IPI_dest(per_cpu(x86_cpu_to_apicid, query_cpu),
>> +				       vector, APIC_DEST_PHYSICAL);
>> +	}
>> +	local_irq_restore(flags);
>> +}
>> +
>> +static void x2apic_savic_send_IPI_mask(const struct cpumask *mask, int vector)
>> +{
>> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLINC);
>> +}
>> +
>> +static void x2apic_savic_send_IPI_mask_allbutself(const struct cpumask *mask, int vector)
>> +{
>> +	__send_IPI_mask(mask, vector, APIC_DEST_ALLBUT);
>> +}
>> +
>> +static int x2apic_savic_probe(void)
>> +{
>> +	if (!cc_platform_has(CC_ATTR_SNP_SECURE_AVIC))
>> +		return 0;
>> +
>> +	if (!x2apic_mode) {
>> +		pr_err("Secure AVIC enabled in non x2APIC mode\n");
>> +		snp_abort();
>> +	}
>> +
>> +	pr_info("Secure AVIC Enabled\n");
> 
> That's not necessary.
> 

Will update.

> Actually, you could figure out why that
> 
> 	pr_info("Switched APIC routing to: %s\n", driver->name);
> 
> doesn't come out in current kernels anymore:
> 

Interesting. I see it working on 6.14-rc7 and master branch.

dmesg | grep -i "switched apic"
[    1.044435] APIC: Switched APIC routing to: physical x2apic


- Neeraj

> $ dmesg | grep -i "switched apic"
> $
> 
> and fix that as a separate patch.
> 
> Looks like it broke in 6.10 or so:
> 
> $ grep -E "Switched APIC" *
> 04-rc7+:Switched APIC routing to physical flat.
> 05-rc1+:Switched APIC routing to physical flat.
> 05-rc2+:Switched APIC routing to physical flat.
> 05-rc3+:Switched APIC routing to physical flat.
> 05-rc4+:APIC: Switched APIC routing to: physical flat
> 05-rc6+:Switched APIC routing to physical flat.
> 06-rc4+:APIC: Switched APIC routing to: physical flat
> 06-rc6+:APIC: Switched APIC routing to: physical flat
> 07-0+:APIC: Switched APIC routing to: physical flat
> 07-rc1+:APIC: Switched APIC routing to: physical flat
> 07-rc7+:APIC: Switched APIC routing to: physical flat
> 08-rc1+:APIC: Switched APIC routing to: physical flat
> 08-rc3+:APIC: Switched APIC routing to: physical flat
> 08-rc6+:APIC: Switched APIC routing to: physical flat
> 08-rc7+:APIC: Switched APIC routing to: physical flat
> 09-rc7+:APIC: Switched APIC routing to: physical flat
> 10-rc1+:APIC: Switched APIC routing to: physical flat
> 10-rc6+:APIC: Switched APIC routing to: physical flat
> <--- EOF
> 
> Thx.
> 
>