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

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

On 10/9/2024 10:53 AM, Borislav Petkov wrote:
> On Wed, Oct 09, 2024 at 07:26:55AM +0530, Neeraj Upadhyay wrote:
>> As SECURE_AVIC feature is not supported (as reported by snp_get_unsupported_features())
>> by guest at this patch in the series, it is added to SNP_FEATURES_IMPL_REQ here. The bit
>> value within SNP_FEATURES_IMPL_REQ hasn't changed with this change as the same bit pos
>> was part of MSR_AMD64_SNP_RESERVED_MASK before this patch. In patch 14 SECURE_AVIC guest
>> support is indicated by guest.
> 
> So what's the point of adding it to SNP_FEATURES_IMPL_REQ here? What does that
> do at all in this patch alone? Why is this change needed in here?
> 

Before this patch, if hypervisor enables Secure AVIC  (reported in sev_status), guest would
terminate in snp_check_features(). The reason for this is, SNP_FEATURES_IMPL_REQ had the Secure
AVIC bit set before this patch, as that bit was part of MSR_AMD64_SNP_RESERVED_MASK 
GENMASK_ULL(63, 18).

#define SNP_FEATURES_IMPL_REQ	(MSR_AMD64_SNP_VTOM |			\
				 ...
				 MSR_AMD64_SNP_RESERVED_MASK)



Adding MSR_AMD64_SNP_SECURE_AVIC_BIT (bit 18) to SNP_FEATURES_IMPL_REQ in this patch
keeps that behavior intact as now with this change MSR_AMD64_SNP_RESERVED_MASK becomes
GENMASK_ULL(63, 19).


> IOW, why don't you do all the feature bit handling in the last patch, where it
> all belongs logically?
> 

If we do that, then hypervisor could have enabled Secure AVIC support and the guest
code at this patch won't catch the missing guest-support early and it can result in some
unknown failures at later point during guest boot.


- Neeraj

> In the last patch you can start *testing* for
> MSR_AMD64_SNP_SECURE_AVIC_ENABLED *and* enforce it with SNP_FEATURES_PRESENT.
>