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

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

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)) {

> +            __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;