Re: [PATCH v11 15/35] KVM: SEV: Add KVM_SNP_INIT command

Paolo Bonzini
2024-03-20T17:28:51Z

On Sat, Dec 30, 2023 at 6:26 PM Michael Roth <michael.roth@amd.com> wrote:
> + struct kvm_snp_init {
> + __u64 flags;
> + };
> +
> +The flags bitmap is defined as::
> +
> + /* enable the restricted injection */
> + #define KVM_SEV_SNP_RESTRICTED_INJET (1<<0)
> +
> + /* enable the restricted injection timer */
> + #define KVM_SEV_SNP_RESTRICTED_TIMER_INJET (1<<1)

The flags are the same as the vmsa_features introduced by
KVM_SEV_INIT2, which is great - SNP does not need any change in this
department and this patch almost entirely goes away.

> if (sev_es_debug_swap_enabled)
> save->sev_features |= SVM_SEV_FEAT_DEBUG_SWAP;
>
> + /* Enable the SEV-SNP feature */
> + if (sev_snp_guest(svm->vcpu.kvm))
> + save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;

... on the other hand this begs the question whether
SVM_SEV_FEAT_SNP_ACTIVE should be exposed in the
KVM_X86_SEV_VMSA_FEATURES attribute. I think it shouldn't.

This means that this patch becomes a two-liner change to
sev_guest_init() that you can squash in patch 14 ("KVM: SEV: Add
initial SEV-SNP support"):

sev->es_active = es_active;
sev->vmsa_features = data->vmsa_features;
+ if (vm_type == KVM_X86_SNP_VM)
+ sev->vmsa_features |= SVM_SEV_FEAT_SNP_ACTIVE

Also, since there is now sev->vmsa_features (that wasn't there at the
time of your posting), I'd even drop sev->snp_active in favor of
"sev->vmsa_features & SVM_SEV_FEAT_SNP_ACTIVE". It's only ever used in
sev_snp_guest() so it's a useless duplication.

Looking forward to see v12. :) If you have any problems rebasing on
top of https://lore.kernel.org/kvm/20240227232100.478238-1-pbonzini@redhat.com/,
please shout.

Paolo

> pr_debug("Virtual Machine Save Area (VMSA):\n");
> print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false);
>
> @@ -1883,6 +1914,12 @@ int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
> }
>
> switch (sev_cmd.id) {
> + case KVM_SEV_SNP_INIT:
> + if (!sev_snp_enabled) {
> + r = -ENOTTY;
> + goto out;
> + }
> + fallthrough;
> case KVM_SEV_ES_INIT:
> if (!sev_es_enabled) {
> r = -ENOTTY;
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index a3e27c82866b..07a9eb5b6ce5 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -76,6 +76,9 @@ enum {
> /* TPR and CR2 are always written before VMRUN */
> #define VMCB_ALWAYS_DIRTY_MASK ((1U << VMCB_INTR) | (1U << VMCB_CR2))
>
> +/* Supported init feature flags */
> +#define SEV_SNP_SUPPORTED_FLAGS 0x0
> +
> struct kvm_sev_info {
> bool active; /* SEV enabled guest */
> bool es_active; /* SEV-ES enabled guest */
> @@ -91,6 +94,7 @@ struct kvm_sev_info {
> struct list_head mirror_entry; /* Use as a list entry of mirrors */
> struct misc_cg *misc_cg; /* For misc cgroup accounting */
> atomic_t migration_in_progress;
> + u64 snp_init_flags;
> };
>
> struct kvm_svm {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index c3308536482b..73702e9b9d76 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1869,6 +1869,9 @@ enum sev_cmd_id {
> /* Guest Migration Extension */
> KVM_SEV_SEND_CANCEL,
>
> + /* SNP specific commands */
> + KVM_SEV_SNP_INIT,
> +
> KVM_SEV_NR_MAX,
> };
>
> @@ -1965,6 +1968,16 @@ struct kvm_sev_receive_update_data {
> __u32 trans_len;
> };
>
> +/* enable the restricted injection */
> +#define KVM_SEV_SNP_RESTRICTED_INJET (1 << 0)
> +
> +/* enable the restricted injection timer */
> +#define KVM_SEV_SNP_RESTRICTED_TIMER_INJET (1 << 1)
> +
> +struct kvm_snp_init {
> + __u64 flags;
> +};
> +
> #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
> #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
> #define KVM_DEV_ASSIGN_MASK_INTX (1 << 2)
> --
> 2.25.1
>