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

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

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