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. >
