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

Neeraj Upadhyay
2025-04-02T10:34:47Z

On 4/2/2025 3:17 PM, Borislav Petkov wrote:
> On Tue, Apr 01, 2025 at 10:42:17AM +0530, Neeraj Upadhyay wrote:
>>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>>> index edc31615cb67..ecf86b8a6601 100644
>>> --- a/arch/x86/include/asm/msr-index.h
>>> +++ b/arch/x86/include/asm/msr-index.h
>>> @@ -685,8 +685,14 @@
>>> #define MSR_AMD64_SNP_VMSA_REG_PROT BIT_ULL(MSR_AMD64_SNP_VMSA_REG_PROT_BIT)
>>> #define MSR_AMD64_SNP_SMT_PROT_BIT 17
>>> #define MSR_AMD64_SNP_SMT_PROT BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
>>> +
>>> #define MSR_AMD64_SNP_SECURE_AVIC_BIT 18
>>> -#define MSR_AMD64_SNP_SECURE_AVIC BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
>>> +#ifdef CONFIG_AMD_SECURE_AVIC
>>> +#define MSR_AMD64_SNP_SECURE_AVIC BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
>>> +#else
>>> +#define MSR_AMD64_SNP_SECURE_AVIC 0
>>> +#endif
>>> +
>>
>> I missed this part. I think this does not work because if CONFIG_AMD_SECURE_AVIC
>> is not enabled, MSR_AMD64_SNP_SECURE_AVIC bit becomes 0 in both SNP_FEATURES_IMPL_REQ
>> and SNP_FEATURES_PRESENT.
>>
>> So, snp_get_unsupported_features() won't report SECURE_AVIC feature as not being
>> enabled in guest launched with SECURE_AVIC vmsa feature enabled. Thoughts?
>
> Your formulations are killing me :-P
>
> ... won't report.. as not being enabled ... with feature enabled.
>
> Double negation with a positive at the end.
>
> So this translates to
>
> "will report as enabled when enabled"
>
> which doesn't make too much sense.
>
> *IF* you have CONFIG_AMD_SECURE_AVIC disabled, then you don't have SAVIC
> support and then SAVIC VMSA feature bit better be 0.
>
> Or what do you mean?
>

My bad. Let me try again.

In previous sentence

"SECURE_AVIC feature as not being enabled" - SAVIC not enabled inside guest.
"SECURE_AVIC vmsa feature enabled" - SAVIC enabled/active in hypervisor for that guest.

This is basically continuation of our previous discussion here [1]

- "sev_status" reports the SEV features enabled/active in Hypervisor for a guest.

- If guest is launched (qemu/VMM launch) with SAVIC VMSA feature enabled, hypervisor
uses SAVIC interrupt injection flow for that guest.

SAVIC VMSA feature is reported in "sev_status" and tells guest that SAVIC
functionality is active (in hypervisor) for that guest.

- snp_get_unsupported_features() looks like below.
It checks that, for the feature bits which are part of SNP_FEATURES_IMPL_REQ,
if they are enabled in hypervisor (and so reported in sev_status),
guest need to implement/enable those features. SAVIC also falls in that category
of SNP features.

So, if CONFIG_AMD_SECURE_AVIC is disabled, guest would run with SAVIC feature
disabled in guest. This would cause undefined behavior for that guest if SAVIC
feature is active for that guest in hypervisor.

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

return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
}

/*
* SNP_FEATURES_IMPL_REQ is the mask of SNP features that will need
* guest side implementation for proper functioning of the guest. If any
* of these features are enabled in the hypervisor but are lacking guest
* side implementation, the behavior of the guest will be undefined. The
* guest could fail in non-obvious way making it difficult to debug.
*/
#define SNP_FEATURES_IMPL_REQ ...

[1] https://lore.kernel.org/lkml/20241009110224.GGZwZiwD27ZvME841d@fat_crate.local/#t

- Neeraj