Re: [RFC] target/i386: sev: Add cmdline option to enable the Allowed SEV Features feature

[Kim Phillips <kim.phillips@amd.com>]

On 2/10/25 3:24 AM, Daniel P. Berrangé wrote:
> On Fri, Feb 07, 2025 at 05:33:27PM -0600, Kim Phillips wrote:
>> The Allowed SEV Features feature allows the host kernel to control
>> which SEV features it does not want the guest to enable [1].
>>
>> This has to be explicitly opted-in by the user because it has the
>> ability to break existing VMs if it were set automatically.
>>
>> Currently, both the PmcVirtualization and SecureAvic features
>> require the Allowed SEV Features feature to be set.
>>
>> Based on a similar patch written for Secure TSC [2].
>>
>> [1] Section 15.36.20 "Allowed SEV Features", AMD64 Architecture
>>      Programmer's Manual, Pub. 24593 Rev. 3.42 - March 2024:
>>      https://bugzilla.kernel.org/attachment.cgi?id=306250
>>
>> [2] https://github.com/qemu/qemu/commit/4b2288dc6025ba32519ee8d202ca72d565cbbab7
> 
> Despite that URL, that commit also does not appear to be merged into
> the QEMU git repo, and indeed I can't find any record of it even being
> posted as a patch for review on qemu-devel.
> 
> This is horribly misleading to reviewers, suggesting that the referenced
> patch was already accepted :-(

Apologies, that was not the intent.  I'll remove it from the next version.

>> @@ -1524,6 +1552,20 @@ static int sev_common_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>>       case KVM_X86_SNP_VM: {
>>           struct kvm_sev_init args = { 0 };
>>   
>> +        if (sev_es_enabled()) {
>> +            __u64 vmsa_features, supported_vmsa_features;
>> +
>> +            supported_vmsa_features = sev_supported_vmsa_features();
>> +            vmsa_features = sev_common->vmsa_features;
>> +            if ((vmsa_features & supported_vmsa_features) != vmsa_features) {
>> +                error_setg(errp, "%s: requested sev feature mask (0x%llx) "
>> +                           "contains bits not supported by the host kernel "
>> +                           " (0x%llx)", __func__, vmsa_features,
>> +                           supported_vmsa_features);
> 
> This logic is being applied unconditionally, and not connected to
> the setting of the new 'allowed-sev-features' flag value. Is that
> correct  ?

That's correct.

> Will this end up breaking existing deployed guests, or is this a
> scenario that would have been blocked with an error later on
> regardless ?

It would have been blocked regardless by this check in kvm's __sev_guest_init:

https://elixir.bootlin.com/linux/v6.13.2/source/arch/x86/kvm/svm/sev.c#L418

I've addressed all your other comments.

Thank you for your review,

Kim