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

[Tom Lendacky <thomas.lendacky@amd.com>]

On 2/10/25 12:53, Tom Lendacky wrote:
> On 2/7/25 17:33, 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
>>
>> Signed-off-by: Kim Phillips <kim.phillips@amd.com>
>> ---
>>  qapi/qom.json     |  6 ++++-
>>  target/i386/sev.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++
>>  target/i386/sev.h |  2 ++
>>  3 files changed, 67 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index 28ce24cd8d..113b44ad74 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -948,13 +948,17 @@
>>  #     designated guest firmware page for measured boot with -kernel
>>  #     (default: false) (since 6.2)
>>  #
>> +# @allowed-sev-features: true if secure allowed-sev-features feature
>> +#     is to be enabled in an SEV-ES or SNP guest. (default: false)
>> +#
>>  # Since: 9.1
>>  ##
>>  { 'struct': 'SevCommonProperties',
>>    'data': { '*sev-device': 'str',
>>              '*cbitpos': 'uint32',
>>              'reduced-phys-bits': 'uint32',
>> -            '*kernel-hashes': 'bool' } }
>> +            '*kernel-hashes': 'bool',
>> +            '*allowed-sev-features': 'bool' } }
>>  
>>  ##
>>  # @SevGuestProperties:
>> diff --git a/target/i386/sev.c b/target/i386/sev.c
>> index 0e1dbb6959..85ad73f9a0 100644
>> --- a/target/i386/sev.c
>> +++ b/target/i386/sev.c
>> @@ -98,6 +98,7 @@ struct SevCommonState {
>>      uint32_t cbitpos;
>>      uint32_t reduced_phys_bits;
>>      bool kernel_hashes;
>> +    uint64_t vmsa_features;
>>  
>>      /* runtime state */
>>      uint8_t api_major;
>> @@ -411,6 +412,33 @@ sev_get_reduced_phys_bits(void)
>>      return sev_common ? sev_common->reduced_phys_bits : 0;
>>  }
>>  
>> +static __u64
>> +sev_supported_vmsa_features(void)
> 
> s/sev_/sev_get_/ ?
> 
>> +{
>> +    uint64_t supported_vmsa_features = 0;
>> +    struct kvm_device_attr attr = {
>> +        .group = KVM_X86_GRP_SEV,
>> +        .attr = KVM_X86_SEV_VMSA_FEATURES,
>> +        .addr = (unsigned long) &supported_vmsa_features
>> +    };
>> +
>> +    bool sys_attr = kvm_check_extension(kvm_state, KVM_CAP_SYS_ATTRIBUTES);
>> +    if (!sys_attr) {
>> +        return 0;
>> +    }
>> +
>> +    int rc = kvm_ioctl(kvm_state, KVM_GET_DEVICE_ATTR, &attr);
>> +    if (rc < 0) {
>> +        if (rc != -ENXIO) {
>> +            warn_report("KVM_GET_DEVICE_ATTR(0, KVM_X86_SEV_VMSA_FEATURES) "
>> +                        "error: %d", rc);
>> +        }
>> +        return 0;
>> +    }
>> +
>> +    return supported_vmsa_features;
>> +}
>> +
>>  static SevInfo *sev_get_info(void)
>>  {
>>      SevInfo *info;
>> @@ -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()) {
> 
> Shouldn't this be
> 
> if (sev_es_enabled() && (sev_common->vmsa_features & SEV_VMSA_ALLOWED_SEV_FEATURES)) {

Actually, I guess it doesn't matter. The vmsa_features field will be 0
by default and only be set if "allowed-sev-features=on" is specified. So
doing this will just error out a bit earlier than KVM erroring out on
the INIT2 call if some vmsa_feature bit is set that KVM doesn't know about.

Thanks,
Tom

> 
>> +            __u64 vmsa_features, supported_vmsa_features;
> 
> s/__u64/uint64_t/ ?
> 
>> +
>> +            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);
>> +            return -1;
>> +            }
> 
> Add a blank line
> 
>> +            args.vmsa_features = vmsa_features;
>> +        }
> 
> Add a blank line
> 
> Thanks,
> Tom
> 
>>          ret = sev_ioctl(sev_common->sev_fd, KVM_SEV_INIT2, &args, &fw_error);
>>          break;
>>      }
>> @@ -2044,6 +2086,19 @@ static void sev_common_set_kernel_hashes(Object *obj, bool value, Error **errp)
>>      SEV_COMMON(obj)->kernel_hashes = value;
>>  }
>>  
>> +static bool
>> +sev_snp_guest_get_allowed_sev_features(Object *obj, Error **errp)
>> +{
>> +    return SEV_COMMON(obj)->vmsa_features & SEV_VMSA_ALLOWED_SEV_FEATURES;
>> +}
>> +
>> +static void
>> +sev_snp_guest_set_allowed_sev_features(Object *obj, bool value, Error **errp)
>> +{
>> +    if (value)
>> +        SEV_COMMON(obj)->vmsa_features |= SEV_VMSA_ALLOWED_SEV_FEATURES;
>> +}
>> +
>>  static void
>>  sev_common_class_init(ObjectClass *oc, void *data)
>>  {
>> @@ -2061,6 +2116,11 @@ sev_common_class_init(ObjectClass *oc, void *data)
>>                                     sev_common_set_kernel_hashes);
>>      object_class_property_set_description(oc, "kernel-hashes",
>>              "add kernel hashes to guest firmware for measured Linux boot");
>> +    object_class_property_add_bool(oc, "allowed-sev-features",
>> +                                   sev_snp_guest_get_allowed_sev_features,
>> +                                   sev_snp_guest_set_allowed_sev_features);
>> +    object_class_property_set_description(oc, "allowed-sev-features",
>> +            "Enable the Allowed SEV Features feature");
>>  }
>>  
>>  static void
>> diff --git a/target/i386/sev.h b/target/i386/sev.h
>> index 373669eaac..07447c4b01 100644
>> --- a/target/i386/sev.h
>> +++ b/target/i386/sev.h
>> @@ -44,6 +44,8 @@ bool sev_snp_enabled(void);
>>  #define SEV_SNP_POLICY_SMT      0x10000
>>  #define SEV_SNP_POLICY_DBG      0x80000
>>  
>> +#define SEV_VMSA_ALLOWED_SEV_FEATURES BIT_ULL(63)
>> +
>>  typedef struct SevKernelLoaderContext {
>>      char *setup_data;
>>      size_t setup_size;