[Part2,v5,23/45] KVM: SVM: Add KVM_SNP_INIT command

Commit Message

Brijesh Singh Aug. 20, 2021, 3:58 p.m. UTC

The KVM_SNP_INIT command is used by the hypervisor to initialize the
SEV-SNP platform context. In a typical workflow, this command should be the
first command issued. When creating SEV-SNP guest, the VMM must use this
command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT.

The flags value must be zero, it will be extended in future SNP support to
communicate the optional features (such as restricted INT injection etc).

Co-developed-by: Pavan Kumar Paluri <[e-mail]>
Signed-off-by: Pavan Kumar Paluri <[e-mail]>
Signed-off-by: Brijesh Singh <[e-mail]>
---
 .../virt/kvm/amd-memory-encryption.rst        | 27 ++++++++++++
 arch/x86/include/asm/svm.h                    |  2 +
 arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
 arch/x86/kvm/svm/svm.h                        |  4 ++
 include/uapi/linux/kvm.h                      | 13 ++++++
 5 files changed, 88 insertions(+), 2 deletions(-)

Comments

Dov Murik Sept. 5, 2021, 6:56 a.m. UTC | #1

Hi Brijesh,

On 20/08/2021 18:58, Brijesh Singh wrote:
> The KVM_SNP_INIT command is used by the hypervisor to initialize the
> SEV-SNP platform context. In a typical workflow, this command should be the
> first command issued. When creating SEV-SNP guest, the VMM must use this
> command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT.
> 
> The flags value must be zero, it will be extended in future SNP support to
> communicate the optional features (such as restricted INT injection etc).
> 
> Co-developed-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Brijesh Singh <[e-mail]>
> ---
>  .../virt/kvm/amd-memory-encryption.rst        | 27 ++++++++++++
>  arch/x86/include/asm/svm.h                    |  2 +
>  arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
>  arch/x86/kvm/svm/svm.h                        |  4 ++
>  include/uapi/linux/kvm.h                      | 13 ++++++
>  5 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
> index 5c081c8c7164..7b1d32fb99a8 100644
> --- a/Documentation/virt/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/amd-memory-encryption.rst
> @@ -427,6 +427,33 @@ issued by the hypervisor to make the guest ready for execution.
>  
>  Returns: 0 on success, -negative on error
>  
> +18. KVM_SNP_INIT
> +----------------
> +
> +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP
> +context. In a typical workflow, this command should be the first command issued.
> +
> +Parameters (in/out): struct kvm_snp_init
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +
> +        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)

Typo in these flags: s/INJET/INJECT/

Also in the actual .h file below.


-Dov


> +
> +If the specified flags is not supported then return -EOPNOTSUPP, and the supported
> +flags are returned.
> +
>  References
>  ==========
>  
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 44a3f920f886..a39e31845a33 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -218,6 +218,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define SVM_NESTED_CTL_SEV_ENABLE	BIT(1)
>  #define SVM_NESTED_CTL_SEV_ES_ENABLE	BIT(2)
>  
> +#define SVM_SEV_FEAT_SNP_ACTIVE		BIT(0)
> +
>  struct vmcb_seg {
>  	u16 selector;
>  	u16 attrib;
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 50fddbe56981..93da463545ef 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -235,10 +235,30 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
>  	sev_decommission(handle);
>  }
>  
> +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +	struct kvm_snp_init params;
> +	int ret = 0;
> +
> +	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
> +		return -EFAULT;
> +
> +	if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS)
> +		ret = -EOPNOTSUPP;
> +
> +	params.flags = SEV_SNP_SUPPORTED_FLAGS;
> +
> +	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
> +		ret = -EFAULT;
> +
> +	return ret;
> +}
> +
>  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
> +	bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -	bool es_active = argp->id == KVM_SEV_ES_INIT;
> +	bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>  	int asid, ret;
>  
>  	if (kvm->created_vcpus)
> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  		return ret;
>  
>  	sev->es_active = es_active;
> +	sev->snp_active = snp_active;
>  	asid = sev_asid_new(sev);
>  	if (asid < 0)
>  		goto e_no_asid;
>  	sev->asid = asid;
>  
> -	ret = sev_platform_init(&argp->error);
> +	if (snp_active) {
> +		ret = verify_snp_init_flags(kvm, argp);
> +		if (ret)
> +			goto e_free;
> +
> +		ret = sev_snp_init(&argp->error);
> +	} else {
> +		ret = sev_platform_init(&argp->error);
> +	}
> +
>  	if (ret)
>  		goto e_free;
>  
> @@ -600,6 +630,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>  	save->pkru = svm->vcpu.arch.pkru;
>  	save->xss  = svm->vcpu.arch.ia32_xss;
>  
> +	/* Enable the SEV-SNP feature */
> +	if (sev_snp_guest(svm->vcpu.kvm))
> +		save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
> +
>  	return 0;
>  }
>  
> @@ -1532,6 +1566,12 @@ int svm_mem_enc_op(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 01953522097d..57c3c404b0b3 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -69,6 +69,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 */
> @@ -81,6 +84,7 @@ struct kvm_sev_info {
>  	u64 ap_jump_table;	/* SEV-ES AP Jump Table address */
>  	struct kvm *enc_context_owner; /* Owner of copied encryption context */
>  	struct misc_cg *misc_cg; /* For misc cgroup accounting */
> +	u64 snp_init_flags;
>  };
>  
>  struct kvm_svm {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index d9e4aabcb31a..944e2bf601fe 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1712,6 +1712,9 @@ enum sev_cmd_id {
>  	/* Guest Migration Extension */
>  	KVM_SEV_SEND_CANCEL,
>  
> +	/* SNP specific commands */
> +	KVM_SEV_SNP_INIT,
> +
>  	KVM_SEV_NR_MAX,
>  };
>  
> @@ -1808,6 +1811,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)
>

Brijesh Singh Sept. 5, 2021, 1:59 p.m. UTC | #2

Hi Dov,


On 9/5/21 1:56 AM, Dov Murik wrote:
> + /* 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)
> Typo in these flags: s/INJET/INJECT/
>
> Also in the actual .h file below.

Noted. thanks

Marc Orr Sept. 10, 2021, 3:32 a.m. UTC | #3

On Fri, Aug 20, 2021 at 9:00 AM Brijesh Singh <[e-mail]> wrote:
>
> The KVM_SNP_INIT command is used by the hypervisor to initialize the
> SEV-SNP platform context. In a typical workflow, this command should be the
> first command issued. When creating SEV-SNP guest, the VMM must use this
> command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT.
>
> The flags value must be zero, it will be extended in future SNP support to
> communicate the optional features (such as restricted INT injection etc).
>
> Co-developed-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Brijesh Singh <[e-mail]>
> ---
>  .../virt/kvm/amd-memory-encryption.rst        | 27 ++++++++++++
>  arch/x86/include/asm/svm.h                    |  2 +
>  arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
>  arch/x86/kvm/svm/svm.h                        |  4 ++
>  include/uapi/linux/kvm.h                      | 13 ++++++
>  5 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
> index 5c081c8c7164..7b1d32fb99a8 100644
> --- a/Documentation/virt/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/amd-memory-encryption.rst
> @@ -427,6 +427,33 @@ issued by the hypervisor to make the guest ready for execution.
>
>  Returns: 0 on success, -negative on error
>
> +18. KVM_SNP_INIT
> +----------------
> +
> +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP
> +context. In a typical workflow, this command should be the first command issued.
> +
> +Parameters (in/out): struct kvm_snp_init
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +
> +        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)
> +
> +If the specified flags is not supported then return -EOPNOTSUPP, and the supported
> +flags are returned.
> +
>  References
>  ==========
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 44a3f920f886..a39e31845a33 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -218,6 +218,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define SVM_NESTED_CTL_SEV_ENABLE      BIT(1)
>  #define SVM_NESTED_CTL_SEV_ES_ENABLE   BIT(2)
>
> +#define SVM_SEV_FEAT_SNP_ACTIVE                BIT(0)
> +
>  struct vmcb_seg {
>         u16 selector;
>         u16 attrib;
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 50fddbe56981..93da463545ef 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -235,10 +235,30 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
>         sev_decommission(handle);
>  }
>
> +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +       struct kvm_snp_init params;
> +       int ret = 0;
> +
> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
> +               return -EFAULT;
> +
> +       if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS)
> +               ret = -EOPNOTSUPP;
> +
> +       params.flags = SEV_SNP_SUPPORTED_FLAGS;
> +
> +       if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
> +               ret = -EFAULT;
> +
> +       return ret;
> +}
> +
>  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>         int asid, ret;
>
>         if (kvm->created_vcpus)
> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>                 return ret;
>
>         sev->es_active = es_active;
> +       sev->snp_active = snp_active;
>         asid = sev_asid_new(sev);
>         if (asid < 0)
>                 goto e_no_asid;
>         sev->asid = asid;
>
> -       ret = sev_platform_init(&argp->error);
> +       if (snp_active) {
> +               ret = verify_snp_init_flags(kvm, argp);
> +               if (ret)
> +                       goto e_free;
> +
> +               ret = sev_snp_init(&argp->error);
> +       } else {
> +               ret = sev_platform_init(&argp->error);
> +       }
> +
>         if (ret)
>                 goto e_free;
>
> @@ -600,6 +630,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>         save->pkru = svm->vcpu.arch.pkru;
>         save->xss  = svm->vcpu.arch.ia32_xss;
>
> +       /* Enable the SEV-SNP feature */
> +       if (sev_snp_guest(svm->vcpu.kvm))
> +               save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
> +
>         return 0;
>  }
>
> @@ -1532,6 +1566,12 @@ int svm_mem_enc_op(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 01953522097d..57c3c404b0b3 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -69,6 +69,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 */
> @@ -81,6 +84,7 @@ struct kvm_sev_info {
>         u64 ap_jump_table;      /* SEV-ES AP Jump Table address */
>         struct kvm *enc_context_owner; /* Owner of copied encryption context */
>         struct misc_cg *misc_cg; /* For misc cgroup accounting */
> +       u64 snp_init_flags;

This field never gets set anywhere. Should it get set in
`verify_snp_init_flags()`?

>  };
>
>  struct kvm_svm {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index d9e4aabcb31a..944e2bf601fe 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1712,6 +1712,9 @@ enum sev_cmd_id {
>         /* Guest Migration Extension */
>         KVM_SEV_SEND_CANCEL,
>
> +       /* SNP specific commands */
> +       KVM_SEV_SNP_INIT,
> +
>         KVM_SEV_NR_MAX,
>  };
>
> @@ -1808,6 +1811,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.17.1
>
>

Brijesh Singh Sept. 13, 2021, 11:32 a.m. UTC | #4

On 9/9/21 10:32 PM, Marc Orr wrote:
> On Fri, Aug 20, 2021 at 9:00 AM Brijesh Singh <[e-mail]> wrote:
>> The KVM_SNP_INIT command is used by the hypervisor to initialize the
>> SEV-SNP platform context. In a typical workflow, this command should be the
>> first command issued. When creating SEV-SNP guest, the VMM must use this
>> command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT.
>>
>> The flags value must be zero, it will be extended in future SNP support to
>> communicate the optional features (such as restricted INT injection etc).
>>
>> Co-developed-by: Pavan Kumar Paluri <[e-mail]>
>> Signed-off-by: Pavan Kumar Paluri <[e-mail]>
>> Signed-off-by: Brijesh Singh <[e-mail]>
>> ---
>>  .../virt/kvm/amd-memory-encryption.rst        | 27 ++++++++++++
>>  arch/x86/include/asm/svm.h                    |  2 +
>>  arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
>>  arch/x86/kvm/svm/svm.h                        |  4 ++
>>  include/uapi/linux/kvm.h                      | 13 ++++++
>>  5 files changed, 88 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
>> index 5c081c8c7164..7b1d32fb99a8 100644
>> --- a/Documentation/virt/kvm/amd-memory-encryption.rst
>> +++ b/Documentation/virt/kvm/amd-memory-encryption.rst
>> @@ -427,6 +427,33 @@ issued by the hypervisor to make the guest ready for execution.
>>
>>  Returns: 0 on success, -negative on error
>>
>> +18. KVM_SNP_INIT
>> +----------------
>> +
>> +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP
>> +context. In a typical workflow, this command should be the first command issued.
>> +
>> +Parameters (in/out): struct kvm_snp_init
>> +
>> +Returns: 0 on success, -negative on error
>> +
>> +::
>> +
>> +        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)
>> +
>> +If the specified flags is not supported then return -EOPNOTSUPP, and the supported
>> +flags are returned.
>> +
>>  References
>>  ==========
>>
>> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
>> index 44a3f920f886..a39e31845a33 100644
>> --- a/arch/x86/include/asm/svm.h
>> +++ b/arch/x86/include/asm/svm.h
>> @@ -218,6 +218,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>>  #define SVM_NESTED_CTL_SEV_ENABLE      BIT(1)
>>  #define SVM_NESTED_CTL_SEV_ES_ENABLE   BIT(2)
>>
>> +#define SVM_SEV_FEAT_SNP_ACTIVE                BIT(0)
>> +
>>  struct vmcb_seg {
>>         u16 selector;
>>         u16 attrib;
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 50fddbe56981..93da463545ef 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -235,10 +235,30 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
>>         sev_decommission(handle);
>>  }
>>
>> +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
>> +{
>> +       struct kvm_snp_init params;
>> +       int ret = 0;
>> +
>> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
>> +               return -EFAULT;
>> +
>> +       if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS)
>> +               ret = -EOPNOTSUPP;
>> +
>> +       params.flags = SEV_SNP_SUPPORTED_FLAGS;
>> +
>> +       if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
>> +               ret = -EFAULT;
>> +
>> +       return ret;
>> +}
>> +
>>  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>  {
>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>>         int asid, ret;
>>
>>         if (kvm->created_vcpus)
>> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>                 return ret;
>>
>>         sev->es_active = es_active;
>> +       sev->snp_active = snp_active;
>>         asid = sev_asid_new(sev);
>>         if (asid < 0)
>>                 goto e_no_asid;
>>         sev->asid = asid;
>>
>> -       ret = sev_platform_init(&argp->error);
>> +       if (snp_active) {
>> +               ret = verify_snp_init_flags(kvm, argp);
>> +               if (ret)
>> +                       goto e_free;
>> +
>> +               ret = sev_snp_init(&argp->error);
>> +       } else {
>> +               ret = sev_platform_init(&argp->error);
>> +       }
>> +
>>         if (ret)
>>                 goto e_free;
>>
>> @@ -600,6 +630,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>>         save->pkru = svm->vcpu.arch.pkru;
>>         save->xss  = svm->vcpu.arch.ia32_xss;
>>
>> +       /* Enable the SEV-SNP feature */
>> +       if (sev_snp_guest(svm->vcpu.kvm))
>> +               save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
>> +
>>         return 0;
>>  }
>>
>> @@ -1532,6 +1566,12 @@ int svm_mem_enc_op(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 01953522097d..57c3c404b0b3 100644
>> --- a/arch/x86/kvm/svm/svm.h
>> +++ b/arch/x86/kvm/svm/svm.h
>> @@ -69,6 +69,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 */
>> @@ -81,6 +84,7 @@ struct kvm_sev_info {
>>         u64 ap_jump_table;      /* SEV-ES AP Jump Table address */
>>         struct kvm *enc_context_owner; /* Owner of copied encryption context */
>>         struct misc_cg *misc_cg; /* For misc cgroup accounting */
>> +       u64 snp_init_flags;
> This field never gets set anywhere. Should it get set in
> `verify_snp_init_flags()`?

Actually the supported flag value is zero, so didn't update it. But to
make code cleaner I will set the flag after the negotiation.


>
>>  };
>>
>>  struct kvm_svm {
>> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
>> index d9e4aabcb31a..944e2bf601fe 100644
>> --- a/include/uapi/linux/kvm.h
>> +++ b/include/uapi/linux/kvm.h
>> @@ -1712,6 +1712,9 @@ enum sev_cmd_id {
>>         /* Guest Migration Extension */
>>         KVM_SEV_SEND_CANCEL,
>>
>> +       /* SNP specific commands */
>> +       KVM_SEV_SNP_INIT,
>> +
>>         KVM_SEV_NR_MAX,
>>  };
>>
>> @@ -1808,6 +1811,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.17.1
>>
>>

Peter Gonda Sept. 16, 2021, 3:50 p.m. UTC | #5

On Fri, Aug 20, 2021 at 10:00 AM Brijesh Singh <[e-mail]> wrote:
>
> The KVM_SNP_INIT command is used by the hypervisor to initialize the
> SEV-SNP platform context. In a typical workflow, this command should be the
> first command issued. When creating SEV-SNP guest, the VMM must use this
> command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT.
>
> The flags value must be zero, it will be extended in future SNP support to
> communicate the optional features (such as restricted INT injection etc).
>
> Co-developed-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Brijesh Singh <[e-mail]>
> ---
>  .../virt/kvm/amd-memory-encryption.rst        | 27 ++++++++++++
>  arch/x86/include/asm/svm.h                    |  2 +
>  arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
>  arch/x86/kvm/svm/svm.h                        |  4 ++
>  include/uapi/linux/kvm.h                      | 13 ++++++
>  5 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
> index 5c081c8c7164..7b1d32fb99a8 100644
> --- a/Documentation/virt/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/amd-memory-encryption.rst
> @@ -427,6 +427,33 @@ issued by the hypervisor to make the guest ready for execution.
>
>  Returns: 0 on success, -negative on error
>
> +18. KVM_SNP_INIT
> +----------------
> +
> +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP
> +context. In a typical workflow, this command should be the first command issued.
> +
> +Parameters (in/out): struct kvm_snp_init
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +
> +        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)
> +
> +If the specified flags is not supported then return -EOPNOTSUPP, and the supported
> +flags are returned.
> +
>  References
>  ==========
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 44a3f920f886..a39e31845a33 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -218,6 +218,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define SVM_NESTED_CTL_SEV_ENABLE      BIT(1)
>  #define SVM_NESTED_CTL_SEV_ES_ENABLE   BIT(2)
>
> +#define SVM_SEV_FEAT_SNP_ACTIVE                BIT(0)
> +
>  struct vmcb_seg {
>         u16 selector;
>         u16 attrib;
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 50fddbe56981..93da463545ef 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -235,10 +235,30 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
>         sev_decommission(handle);
>  }
>
> +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +       struct kvm_snp_init params;
> +       int ret = 0;
> +
> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
> +               return -EFAULT;
> +
> +       if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS)
> +               ret = -EOPNOTSUPP;
> +
> +       params.flags = SEV_SNP_SUPPORTED_FLAGS;
> +
> +       if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
> +               ret = -EFAULT;
> +
> +       return ret;
> +}
> +
>  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>         int asid, ret;

Not sure if this is the patch place for this but I think you want to
disallow svm_vm_copy_asid_from() if snp_active == true.

>
>         if (kvm->created_vcpus)
> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>                 return ret;
>
>         sev->es_active = es_active;
> +       sev->snp_active = snp_active;
>         asid = sev_asid_new(sev);
>         if (asid < 0)
>                 goto e_no_asid;
>         sev->asid = asid;
>
> -       ret = sev_platform_init(&argp->error);
> +       if (snp_active) {
> +               ret = verify_snp_init_flags(kvm, argp);
> +               if (ret)
> +                       goto e_free;
> +
> +               ret = sev_snp_init(&argp->error);
> +       } else {
> +               ret = sev_platform_init(&argp->error);
> +       }
> +
>         if (ret)
>                 goto e_free;
>
> @@ -600,6 +630,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>         save->pkru = svm->vcpu.arch.pkru;
>         save->xss  = svm->vcpu.arch.ia32_xss;
>
> +       /* Enable the SEV-SNP feature */
> +       if (sev_snp_guest(svm->vcpu.kvm))
> +               save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
> +
>         return 0;
>  }
>
> @@ -1532,6 +1566,12 @@ int svm_mem_enc_op(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 01953522097d..57c3c404b0b3 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -69,6 +69,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 */
> @@ -81,6 +84,7 @@ struct kvm_sev_info {
>         u64 ap_jump_table;      /* SEV-ES AP Jump Table address */
>         struct kvm *enc_context_owner; /* Owner of copied encryption context */
>         struct misc_cg *misc_cg; /* For misc cgroup accounting */
> +       u64 snp_init_flags;
>  };
>
>  struct kvm_svm {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index d9e4aabcb31a..944e2bf601fe 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1712,6 +1712,9 @@ enum sev_cmd_id {
>         /* Guest Migration Extension */
>         KVM_SEV_SEND_CANCEL,
>
> +       /* SNP specific commands */
> +       KVM_SEV_SNP_INIT,
> +
>         KVM_SEV_NR_MAX,
>  };
>
> @@ -1808,6 +1811,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.17.1
>
>

Alper Gun June 13, 2022, 8:58 p.m. UTC | #6

On Fri, Aug 20, 2021 at 9:00 AM Brijesh Singh <[e-mail]> wrote:
>
> The KVM_SNP_INIT command is used by the hypervisor to initialize the
> SEV-SNP platform context. In a typical workflow, this command should be the
> first command issued. When creating SEV-SNP guest, the VMM must use this
> command instead of the KVM_SEV_INIT or KVM_SEV_ES_INIT.
>
> The flags value must be zero, it will be extended in future SNP support to
> communicate the optional features (such as restricted INT injection etc).
>
> Co-developed-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Pavan Kumar Paluri <[e-mail]>
> Signed-off-by: Brijesh Singh <[e-mail]>
> ---
>  .../virt/kvm/amd-memory-encryption.rst        | 27 ++++++++++++
>  arch/x86/include/asm/svm.h                    |  2 +
>  arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
>  arch/x86/kvm/svm/svm.h                        |  4 ++
>  include/uapi/linux/kvm.h                      | 13 ++++++
>  5 files changed, 88 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
> index 5c081c8c7164..7b1d32fb99a8 100644
> --- a/Documentation/virt/kvm/amd-memory-encryption.rst
> +++ b/Documentation/virt/kvm/amd-memory-encryption.rst
> @@ -427,6 +427,33 @@ issued by the hypervisor to make the guest ready for execution.
>
>  Returns: 0 on success, -negative on error
>
> +18. KVM_SNP_INIT
> +----------------
> +
> +The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP
> +context. In a typical workflow, this command should be the first command issued.
> +
> +Parameters (in/out): struct kvm_snp_init
> +
> +Returns: 0 on success, -negative on error
> +
> +::
> +
> +        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)
> +
> +If the specified flags is not supported then return -EOPNOTSUPP, and the supported
> +flags are returned.
> +
>  References
>  ==========
>
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index 44a3f920f886..a39e31845a33 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -218,6 +218,8 @@ struct __attribute__ ((__packed__)) vmcb_control_area {
>  #define SVM_NESTED_CTL_SEV_ENABLE      BIT(1)
>  #define SVM_NESTED_CTL_SEV_ES_ENABLE   BIT(2)
>
> +#define SVM_SEV_FEAT_SNP_ACTIVE                BIT(0)
> +
>  struct vmcb_seg {
>         u16 selector;
>         u16 attrib;
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 50fddbe56981..93da463545ef 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -235,10 +235,30 @@ static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
>         sev_decommission(handle);
>  }
>
> +static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
> +{
> +       struct kvm_snp_init params;
> +       int ret = 0;
> +
> +       if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
> +               return -EFAULT;
> +
> +       if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS)
> +               ret = -EOPNOTSUPP;
> +
> +       params.flags = SEV_SNP_SUPPORTED_FLAGS;
> +
> +       if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
> +               ret = -EFAULT;
> +
> +       return ret;
> +}
> +
>  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>         struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>         int asid, ret;
>
>         if (kvm->created_vcpus)
> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>                 return ret;
>
>         sev->es_active = es_active;
> +       sev->snp_active = snp_active;
>         asid = sev_asid_new(sev);
>         if (asid < 0)
>                 goto e_no_asid;
>         sev->asid = asid;
>
> -       ret = sev_platform_init(&argp->error);
> +       if (snp_active) {
> +               ret = verify_snp_init_flags(kvm, argp);
> +               if (ret)
> +                       goto e_free;
> +
> +               ret = sev_snp_init(&argp->error);
> +       } else {
> +               ret = sev_platform_init(&argp->error);

After SEV INIT_EX support patches, SEV may be initialized in the platform late.
In my tests, if SEV has not been initialized in the platform yet, SNP
VMs fail with SEV_DF_FLUSH required error. I tried calling
SEV_DF_FLUSH right after the SNP platform init but this time it failed
later on the SNP launch update command with SEV_RET_INVALID_PARAM
error. Looks like there is another dependency on SEV platform
initialization.

Calling sev_platform_init for SNP VMs fixes the problem in our tests.

> +       }
> +
>         if (ret)
>                 goto e_free;
>
> @@ -600,6 +630,10 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
>         save->pkru = svm->vcpu.arch.pkru;
>         save->xss  = svm->vcpu.arch.ia32_xss;
>
> +       /* Enable the SEV-SNP feature */
> +       if (sev_snp_guest(svm->vcpu.kvm))
> +               save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
> +
>         return 0;
>  }
>
> @@ -1532,6 +1566,12 @@ int svm_mem_enc_op(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 01953522097d..57c3c404b0b3 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -69,6 +69,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 */
> @@ -81,6 +84,7 @@ struct kvm_sev_info {
>         u64 ap_jump_table;      /* SEV-ES AP Jump Table address */
>         struct kvm *enc_context_owner; /* Owner of copied encryption context */
>         struct misc_cg *misc_cg; /* For misc cgroup accounting */
> +       u64 snp_init_flags;
>  };
>
>  struct kvm_svm {
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index d9e4aabcb31a..944e2bf601fe 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1712,6 +1712,9 @@ enum sev_cmd_id {
>         /* Guest Migration Extension */
>         KVM_SEV_SEND_CANCEL,
>
> +       /* SNP specific commands */
> +       KVM_SEV_SNP_INIT,
> +
>         KVM_SEV_NR_MAX,
>  };
>
> @@ -1808,6 +1811,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.17.1
>
>

Ashish Kalra June 13, 2022, 11:15 p.m. UTC | #7

Hello Alper,

On 6/13/22 20:58, Alper Gun wrote:
> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   {
>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>>          struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>>          int asid, ret;
>>
>>          if (kvm->created_vcpus)
>> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>                  return ret;
>>
>>          sev->es_active = es_active;
>> +       sev->snp_active = snp_active;
>>          asid = sev_asid_new(sev);
>>          if (asid < 0)
>>                  goto e_no_asid;
>>          sev->asid = asid;
>>
>> -       ret = sev_platform_init(&argp->error);
>> +       if (snp_active) {
>> +               ret = verify_snp_init_flags(kvm, argp);
>> +               if (ret)
>> +                       goto e_free;
>> +
>> +               ret = sev_snp_init(&argp->error);
>> +       } else {
>> +               ret = sev_platform_init(&argp->error);
> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> In my tests, if SEV has not been initialized in the platform yet, SNP
> VMs fail with SEV_DF_FLUSH required error. I tried calling
> SEV_DF_FLUSH right after the SNP platform init but this time it failed
> later on the SNP launch update command with SEV_RET_INVALID_PARAM
> error. Looks like there is another dependency on SEV platform
> initialization.
>
> Calling sev_platform_init for SNP VMs fixes the problem in our tests.

Trying to get some more context for this issue.

When you say after SEV_INIT_EX support patches, SEV may be initialized 
in the platform late, do you mean sev_pci_init()->sev_snp_init() ... 
sev_platform_init() code path has still not executed on the host BSP ?

Before launching the first SNP/SEV guest launch after INIT, we need to 
issue SEV_CMD_DF_FLUSH command.

I assume that we will always be initially doing SNP firmware 
initialization with SNP_INIT command followed by sev_platform_init(), if 
SNP is enabled on boot CPU.

Thanks, Ashish

Alper Gun June 13, 2022, 11:33 p.m. UTC | #8

On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
>
> Hello Alper,
>
> On 6/13/22 20:58, Alper Gun wrote:
> > static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >>   {
> >> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
> >>          struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> >> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> >> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> >>          int asid, ret;
> >>
> >>          if (kvm->created_vcpus)
> >> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >>                  return ret;
> >>
> >>          sev->es_active = es_active;
> >> +       sev->snp_active = snp_active;
> >>          asid = sev_asid_new(sev);
> >>          if (asid < 0)
> >>                  goto e_no_asid;
> >>          sev->asid = asid;
> >>
> >> -       ret = sev_platform_init(&argp->error);
> >> +       if (snp_active) {
> >> +               ret = verify_snp_init_flags(kvm, argp);
> >> +               if (ret)
> >> +                       goto e_free;
> >> +
> >> +               ret = sev_snp_init(&argp->error);
> >> +       } else {
> >> +               ret = sev_platform_init(&argp->error);
> > After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > In my tests, if SEV has not been initialized in the platform yet, SNP
> > VMs fail with SEV_DF_FLUSH required error. I tried calling
> > SEV_DF_FLUSH right after the SNP platform init but this time it failed
> > later on the SNP launch update command with SEV_RET_INVALID_PARAM
> > error. Looks like there is another dependency on SEV platform
> > initialization.
> >
> > Calling sev_platform_init for SNP VMs fixes the problem in our tests.
>
> Trying to get some more context for this issue.
>
> When you say after SEV_INIT_EX support patches, SEV may be initialized
> in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> sev_platform_init() code path has still not executed on the host BSP ?
>

Correct, INIT_EX requires the file system to be ready and there is a
ccp module param to call it only when needed.

MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be
initialized on module init. Else the PSP will be initialized on the
first command requiring it");

If this module param is false, it won't initialize SEV on the platform
until the first SEV VM.

> Before launching the first SNP/SEV guest launch after INIT, we need to
> issue SEV_CMD_DF_FLUSH command.
>
> I assume that we will always be initially doing SNP firmware
> initialization with SNP_INIT command followed by sev_platform_init(), if
> SNP is enabled on boot CPU.
>
> Thanks, Ashish
>

Ashish Kalra June 14, 2022, 12:21 a.m. UTC | #9

On 6/13/22 23:33, Alper Gun wrote:
> On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
>> Hello Alper,
>>
>> On 6/13/22 20:58, Alper Gun wrote:
>>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>>    {
>>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
>>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
>>>>           int asid, ret;
>>>>
>>>>           if (kvm->created_vcpus)
>>>> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>>                   return ret;
>>>>
>>>>           sev->es_active = es_active;
>>>> +       sev->snp_active = snp_active;
>>>>           asid = sev_asid_new(sev);
>>>>           if (asid < 0)
>>>>                   goto e_no_asid;
>>>>           sev->asid = asid;
>>>>
>>>> -       ret = sev_platform_init(&argp->error);
>>>> +       if (snp_active) {
>>>> +               ret = verify_snp_init_flags(kvm, argp);
>>>> +               if (ret)
>>>> +                       goto e_free;
>>>> +
>>>> +               ret = sev_snp_init(&argp->error);
>>>> +       } else {
>>>> +               ret = sev_platform_init(&argp->error);
>>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
>>> In my tests, if SEV has not been initialized in the platform yet, SNP
>>> VMs fail with SEV_DF_FLUSH required error. I tried calling
>>> SEV_DF_FLUSH right after the SNP platform init but this time it failed
>>> later on the SNP launch update command with SEV_RET_INVALID_PARAM
>>> error. Looks like there is another dependency on SEV platform
>>> initialization.
>>>
>>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
>> Trying to get some more context for this issue.
>>
>> When you say after SEV_INIT_EX support patches, SEV may be initialized
>> in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
>> sev_platform_init() code path has still not executed on the host BSP ?
>>
> Correct, INIT_EX requires the file system to be ready and there is a
> ccp module param to call it only when needed.
>
> MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be
> initialized on module init. Else the PSP will be initialized on the
> first command requiring it");
>
> If this module param is false, it won't initialize SEV on the platform
> until the first SEV VM.
>
Ok, that makes sense.

So the fix will be to call sev_platform_init() unconditionally here in 
sev_guest_init(), and both sev_snp_init() and sev_platform_init() are 
protected from being called again, so there won't be any issues if these 
functions are invoked again at SNP/SEV VM launch if they have been 
invoked earlier during module init.

Thanks, Ashish

Peter Gonda June 14, 2022, 3:37 p.m. UTC | #10

On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
>
>
> On 6/13/22 23:33, Alper Gun wrote:
> > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> >> Hello Alper,
> >>
> >> On 6/13/22 20:58, Alper Gun wrote:
> >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >>>>    {
> >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
> >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> >>>>           int asid, ret;
> >>>>
> >>>>           if (kvm->created_vcpus)
> >>>> @@ -249,12 +269,22 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >>>>                   return ret;
> >>>>
> >>>>           sev->es_active = es_active;
> >>>> +       sev->snp_active = snp_active;
> >>>>           asid = sev_asid_new(sev);
> >>>>           if (asid < 0)
> >>>>                   goto e_no_asid;
> >>>>           sev->asid = asid;
> >>>>
> >>>> -       ret = sev_platform_init(&argp->error);
> >>>> +       if (snp_active) {
> >>>> +               ret = verify_snp_init_flags(kvm, argp);
> >>>> +               if (ret)
> >>>> +                       goto e_free;
> >>>> +
> >>>> +               ret = sev_snp_init(&argp->error);
> >>>> +       } else {
> >>>> +               ret = sev_platform_init(&argp->error);
> >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> >>> In my tests, if SEV has not been initialized in the platform yet, SNP
> >>> VMs fail with SEV_DF_FLUSH required error. I tried calling
> >>> SEV_DF_FLUSH right after the SNP platform init but this time it failed
> >>> later on the SNP launch update command with SEV_RET_INVALID_PARAM
> >>> error. Looks like there is another dependency on SEV platform
> >>> initialization.
> >>>
> >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> >> Trying to get some more context for this issue.
> >>
> >> When you say after SEV_INIT_EX support patches, SEV may be initialized
> >> in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> >> sev_platform_init() code path has still not executed on the host BSP ?
> >>
> > Correct, INIT_EX requires the file system to be ready and there is a
> > ccp module param to call it only when needed.
> >
> > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be
> > initialized on module init. Else the PSP will be initialized on the
> > first command requiring it");
> >
> > If this module param is false, it won't initialize SEV on the platform
> > until the first SEV VM.
> >
> Ok, that makes sense.
>
> So the fix will be to call sev_platform_init() unconditionally here in
> sev_guest_init(), and both sev_snp_init() and sev_platform_init() are
> protected from being called again, so there won't be any issues if these
> functions are invoked again at SNP/SEV VM launch if they have been
> invoked earlier during module init.

That's one solution. I don't know if there is a downside to the system
for enabling SEV if SNP is being enabled but another solution could be
to just directly place a DF_FLUSH command instead of calling
sev_platform_init().

>
> Thanks, Ashish
>

Kalra, Ashish June 14, 2022, 4:11 p.m. UTC | #11

[AMD Official Use Only - General]


-----Original Message-----
From: Peter Gonda <[e-mail]> 
Sent: Tuesday, June 14, 2022 10:38 AM
To: Kalra, Ashish <[e-mail]>
Cc: Alper Gun <[e-mail]>; Brijesh Singh <[e-mail]>; Kalra, Ashish <[e-mail]>; the arch/x86 maintainers <[e-mail]>; LKML <[e-mail]>; kvm list <[e-mail]>; [e-mail]; [e-mail]; Linux Crypto Mailing List <[e-mail]>; Thomas Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo Bonzini <[e-mail]>; Sean Christopherson <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave Hansen <[e-mail]>; Sergio Lopez <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas Pandruvada <[e-mail]>; David Rientjes <[e-mail]>; Dov Murik <[e-mail]>; Tobin Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; Roth, Michael <[e-mail]>; Vlastimil Babka <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr <[e-mail]>; Sathyanarayanan Kuppuswamy <[e-mail]>; Pavan Kumar Paluri <[e-mail]>
Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command

On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
>
>
> On 6/13/22 23:33, Alper Gun wrote:
> > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> >> Hello Alper,
> >>
> >> On 6/13/22 20:58, Alper Gun wrote:
> >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd 
> >>> *argp)
> >>>>    {
> >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id 
> >>>> + == KVM_SEV_SNP_INIT);
> >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> >>>>           int asid, ret;
> >>>>
> >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ static 
> >>>> int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> >>>>                   return ret;
> >>>>
> >>>>           sev->es_active = es_active;
> >>>> +       sev->snp_active = snp_active;
> >>>>           asid = sev_asid_new(sev);
> >>>>           if (asid < 0)
> >>>>                   goto e_no_asid;
> >>>>           sev->asid = asid;
> >>>>
> >>>> -       ret = sev_platform_init(&argp->error);
> >>>> +       if (snp_active) {
> >>>> +               ret = verify_snp_init_flags(kvm, argp);
> >>>> +               if (ret)
> >>>> +                       goto e_free;
> >>>> +
> >>>> +               ret = sev_snp_init(&argp->error);
> >>>> +       } else {
> >>>> +               ret = sev_platform_init(&argp->error);
> >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> >>> In my tests, if SEV has not been initialized in the platform yet, 
> >>> SNP VMs fail with SEV_DF_FLUSH required error. I tried calling 
> >>> SEV_DF_FLUSH right after the SNP platform init but this time it 
> >>> failed later on the SNP launch update command with 
> >>> SEV_RET_INVALID_PARAM error. Looks like there is another 
> >>> dependency on SEV platform initialization.
> >>>
> >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> >> Trying to get some more context for this issue.
> >>
> >> When you say after SEV_INIT_EX support patches, SEV may be 
> >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> >> sev_platform_init() code path has still not executed on the host BSP ?
> >>
> > Correct, INIT_EX requires the file system to be ready and there is a 
> > ccp module param to call it only when needed.
> >
> > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be 
> > initialized on module init. Else the PSP will be initialized on the 
> > first command requiring it");
> >
> > If this module param is false, it won't initialize SEV on the 
> > platform until the first SEV VM.
> >
> Ok, that makes sense.
>
> So the fix will be to call sev_platform_init() unconditionally here in 
> sev_guest_init(), and both sev_snp_init() and sev_platform_init() are 
> protected from being called again, so there won't be any issues if 
> these functions are invoked again at SNP/SEV VM launch if they have 
> been invoked earlier during module init.

>That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().

Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.

Thanks,
Ashish

Peter Gonda June 14, 2022, 4:30 p.m. UTC | #12

On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <[e-mail]> wrote:
>
> [AMD Official Use Only - General]
>
>
> -----Original Message-----
> From: Peter Gonda <[e-mail]>
> Sent: Tuesday, June 14, 2022 10:38 AM
> To: Kalra, Ashish <[e-mail]>
> Cc: Alper Gun <[e-mail]>; Brijesh Singh <[e-mail]>; Kalra, Ashish <[e-mail]>; the arch/x86 maintainers <[e-mail]>; LKML <[e-mail]>; kvm list <[e-mail]>; [e-mail]; [e-mail]; Linux Crypto Mailing List <[e-mail]>; Thomas Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo Bonzini <[e-mail]>; Sean Christopherson <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave Hansen <[e-mail]>; Sergio Lopez <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas Pandruvada <[e-mail]>; David Rientjes <[e-mail]>; Dov Murik <[e-mail]>; Tobin Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; Roth, Michael <[e-mail]>; Vlastimil Babka <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr <[e-mail]>; Sathyanarayanan Kuppuswamy <[e-mail]>; Pavan Kumar Paluri <[e-mail]>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
> >
> >
> > On 6/13/22 23:33, Alper Gun wrote:
> > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> > >> Hello Alper,
> > >>
> > >> On 6/13/22 20:58, Alper Gun wrote:
> > >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd
> > >>> *argp)
> > >>>>    {
> > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id
> > >>>> + == KVM_SEV_SNP_INIT);
> > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > >>>>           int asid, ret;
> > >>>>
> > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ static
> > >>>> int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >>>>                   return ret;
> > >>>>
> > >>>>           sev->es_active = es_active;
> > >>>> +       sev->snp_active = snp_active;
> > >>>>           asid = sev_asid_new(sev);
> > >>>>           if (asid < 0)
> > >>>>                   goto e_no_asid;
> > >>>>           sev->asid = asid;
> > >>>>
> > >>>> -       ret = sev_platform_init(&argp->error);
> > >>>> +       if (snp_active) {
> > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > >>>> +               if (ret)
> > >>>> +                       goto e_free;
> > >>>> +
> > >>>> +               ret = sev_snp_init(&argp->error);
> > >>>> +       } else {
> > >>>> +               ret = sev_platform_init(&argp->error);
> > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > >>> In my tests, if SEV has not been initialized in the platform yet,
> > >>> SNP VMs fail with SEV_DF_FLUSH required error. I tried calling
> > >>> SEV_DF_FLUSH right after the SNP platform init but this time it
> > >>> failed later on the SNP launch update command with
> > >>> SEV_RET_INVALID_PARAM error. Looks like there is another
> > >>> dependency on SEV platform initialization.
> > >>>
> > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > >> Trying to get some more context for this issue.
> > >>
> > >> When you say after SEV_INIT_EX support patches, SEV may be
> > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > >> sev_platform_init() code path has still not executed on the host BSP ?
> > >>
> > > Correct, INIT_EX requires the file system to be ready and there is a
> > > ccp module param to call it only when needed.
> > >
> > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be
> > > initialized on module init. Else the PSP will be initialized on the
> > > first command requiring it");
> > >
> > > If this module param is false, it won't initialize SEV on the
> > > platform until the first SEV VM.
> > >
> > Ok, that makes sense.
> >
> > So the fix will be to call sev_platform_init() unconditionally here in
> > sev_guest_init(), and both sev_snp_init() and sev_platform_init() are
> > protected from being called again, so there won't be any issues if
> > these functions are invoked again at SNP/SEV VM launch if they have
> > been invoked earlier during module init.
>
> >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
>
> Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.

But if psp_init_on_probe is false, sev_platform_init() isn't called
down this path. Alper has suggested we always call sev_platform_init()
but we could just place an SEV_DF_FLUSH command instead. Or am I still
missing something?

>
> Thanks,
> Ashish

Kalra, Ashish June 14, 2022, 5:16 p.m. UTC | #13

[AMD Official Use Only - General]

Hello Alper, Peter,

-----Original Message-----
From: Peter Gonda <[e-mail]> 
Sent: Tuesday, June 14, 2022 11:30 AM
To: Kalra, Ashish <[e-mail]>
Cc: Alper Gun <[e-mail]>; Brijesh Singh <[e-mail]>; the arch/x86 maintainers <[e-mail]>; LKML <[e-mail]>; kvm list <[e-mail]>; [e-mail]; [e-mail]; Linux Crypto Mailing List <[e-mail]>; Thomas Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo Bonzini <[e-mail]>; Sean Christopherson <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave Hansen <[e-mail]>; Sergio Lopez <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas Pandruvada <[e-mail]>; David Rientjes <[e-mail]>; Dov Murik <[e-mail]>; Tobin Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; Roth, Michael <[e-mail]>; Vlastimil Babka <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr <[e-mail]>; Sathyanarayanan Kuppuswamy <[e-mail]>; Pavan Kumar Paluri <[e-mail]>
Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command

On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <[e-mail]> wrote:
>
> [AMD Official Use Only - General]
>
>
> -----Original Message-----
> From: Peter Gonda <[e-mail]>
> Sent: Tuesday, June 14, 2022 10:38 AM
> To: Kalra, Ashish <[e-mail]>
> Cc: Alper Gun <[e-mail]>; Brijesh Singh 
> <[e-mail]>; Kalra, Ashish <[e-mail]>; the 
> arch/x86 maintainers <[e-mail]>; LKML 
> <[e-mail]>; kvm list <[e-mail]>; 
> [e-mail]; [e-mail]; Linux Crypto Mailing 
> List <[e-mail]>; Thomas Gleixner 
> <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel 
> <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. 
> Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo 
> Bonzini <[e-mail]>; Sean Christopherson 
> <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng 
> Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy 
> Lutomirski <[e-mail]>; Dave Hansen 
> <[e-mail]>; Sergio Lopez <[e-mail]>; Peter 
> Zijlstra <[e-mail]>; Srinivas Pandruvada 
> <[e-mail]>; David Rientjes 
> <[e-mail]>; Dov Murik <[e-mail]>; Tobin 
> Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; 
> Roth, Michael <[e-mail]>; Vlastimil Babka 
> <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi 
> Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr 
> <[e-mail]>; Sathyanarayanan Kuppuswamy 
> <[e-mail]>; Pavan Kumar Paluri 
> <[e-mail]>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
> >
> >
> > On 6/13/22 23:33, Alper Gun wrote:
> > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> > >> Hello Alper,
> > >>
> > >> On 6/13/22 20:58, Alper Gun wrote:
> > >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd
> > >>> *argp)
> > >>>>    {
> > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT || 
> > >>>> + argp->id == KVM_SEV_SNP_INIT);
> > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > >>>>           int asid, ret;
> > >>>>
> > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ static 
> > >>>> int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >>>>                   return ret;
> > >>>>
> > >>>>           sev->es_active = es_active;
> > >>>> +       sev->snp_active = snp_active;
> > >>>>           asid = sev_asid_new(sev);
> > >>>>           if (asid < 0)
> > >>>>                   goto e_no_asid;
> > >>>>           sev->asid = asid;
> > >>>>
> > >>>> -       ret = sev_platform_init(&argp->error);
> > >>>> +       if (snp_active) {
> > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > >>>> +               if (ret)
> > >>>> +                       goto e_free;
> > >>>> +
> > >>>> +               ret = sev_snp_init(&argp->error);
> > >>>> +       } else {
> > >>>> +               ret = sev_platform_init(&argp->error);
> > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > >>> In my tests, if SEV has not been initialized in the platform 
> > >>> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried 
> > >>> calling SEV_DF_FLUSH right after the SNP platform init but this 
> > >>> time it failed later on the SNP launch update command with 
> > >>> SEV_RET_INVALID_PARAM error. Looks like there is another 
> > >>> dependency on SEV platform initialization.
> > >>>
> > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > >> Trying to get some more context for this issue.
> > >>
> > >> When you say after SEV_INIT_EX support patches, SEV may be 
> > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > >> sev_platform_init() code path has still not executed on the host BSP ?
> > >>
> > > Correct, INIT_EX requires the file system to be ready and there is 
> > > a ccp module param to call it only when needed.
> > >
> > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be 
> > > initialized on module init. Else the PSP will be initialized on 
> > > the first command requiring it");
> > >
> > > If this module param is false, it won't initialize SEV on the 
> > > platform until the first SEV VM.
> > >
> > Ok, that makes sense.
> >
> > So the fix will be to call sev_platform_init() unconditionally here 
> > in sev_guest_init(), and both sev_snp_init() and sev_platform_init() 
> > are protected from being called again, so there won't be any issues 
> > if these functions are invoked again at SNP/SEV VM launch if they 
> > have been invoked earlier during module init.
>
> >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
>
> Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.

> But if psp_init_on_probe is false, sev_platform_init() isn't called down this path. Alper has suggested we always call sev_platform_init() but we could just place an SEV_DF_FLUSH command instead. Or am I still missing something?

>After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> In my tests, if SEV has not been initialized in the platform 
> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried 
> calling SEV_DF_FLUSH right after the SNP platform init.

Are you getting the DLFLUSH_REQUIRED error after the SNP activate command ?

Also did you use the SEV_DF_FLUSH command or the SNP_DF_FLUSH command ?

With SNP you need to use SNP_DF_FLUSH command.

Thanks,
Ashish

Alper Gun June 14, 2022, 6:58 p.m. UTC | #14

Let me summarize what I tried.

1- when using psp_init_probe false, the SNP VM fails in
SNP_LAUNCH_START step with error SEV_RET_DFFLUSH_REQUIRED(15).
2- added SEV_DF_FLUSH just after SNP platform init and it didn't fail
in launch start but failed later during SNP_LAUNCH_UPDATE with
SEV_RET_INVALID_PARAM(22)
3- added SNP_DF_FLUSH just after SNP platform init and it failed again
during SNP_LAUNCH_UPDATE with SEV_RET_INVALID_PARAM(22)
4- added sev_platform_init for SNP VMs and it worked.

For me DF_FLUSH alone didn' help boot a VM. I don't know yet why sev
platform status impacts the SNP VM, but sev_platform_init fixes the
problem.


On Tue, Jun 14, 2022 at 10:16 AM Kalra, Ashish <[e-mail]> wrote:
>
> [AMD Official Use Only - General]
>
> Hello Alper, Peter,
>
> -----Original Message-----
> From: Peter Gonda <[e-mail]>
> Sent: Tuesday, June 14, 2022 11:30 AM
> To: Kalra, Ashish <[e-mail]>
> Cc: Alper Gun <[e-mail]>; Brijesh Singh <[e-mail]>; the arch/x86 maintainers <[e-mail]>; LKML <[e-mail]>; kvm list <[e-mail]>; [e-mail]; [e-mail]; Linux Crypto Mailing List <[e-mail]>; Thomas Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo Bonzini <[e-mail]>; Sean Christopherson <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave Hansen <[e-mail]>; Sergio Lopez <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas Pandruvada <[e-mail]>; David Rientjes <[e-mail]>; Dov Murik <[e-mail]>; Tobin Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; Roth, Michael <[e-mail]>; Vlastimil Babka <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr <[e-mail]>; Sathyanarayanan Kuppuswamy <[e-mail]>; Pavan Kumar Paluri <[e-mail]>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <[e-mail]> wrote:
> >
> > [AMD Official Use Only - General]
> >
> >
> > -----Original Message-----
> > From: Peter Gonda <[e-mail]>
> > Sent: Tuesday, June 14, 2022 10:38 AM
> > To: Kalra, Ashish <[e-mail]>
> > Cc: Alper Gun <[e-mail]>; Brijesh Singh
> > <[e-mail]>; Kalra, Ashish <[e-mail]>; the
> > arch/x86 maintainers <[e-mail]>; LKML
> > <[e-mail]>; kvm list <[e-mail]>;
> > [e-mail]; [e-mail]; Linux Crypto Mailing
> > List <[e-mail]>; Thomas Gleixner
> > <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel
> > <[e-mail]>; Lendacky, Thomas <[e-mail]>; H.
> > Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo
> > Bonzini <[e-mail]>; Sean Christopherson
> > <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng
> > Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy
> > Lutomirski <[e-mail]>; Dave Hansen
> > <[e-mail]>; Sergio Lopez <[e-mail]>; Peter
> > Zijlstra <[e-mail]>; Srinivas Pandruvada
> > <[e-mail]>; David Rientjes
> > <[e-mail]>; Dov Murik <[e-mail]>; Tobin
> > Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>;
> > Roth, Michael <[e-mail]>; Vlastimil Babka
> > <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi
> > Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr
> > <[e-mail]>; Sathyanarayanan Kuppuswamy
> > <[e-mail]>; Pavan Kumar Paluri
> > <[e-mail]>
> > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
> >
> > On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
> > >
> > >
> > > On 6/13/22 23:33, Alper Gun wrote:
> > > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> > > >> Hello Alper,
> > > >>
> > > >> On 6/13/22 20:58, Alper Gun wrote:
> > > >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd
> > > >>> *argp)
> > > >>>>    {
> > > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT ||
> > > >>>> + argp->id == KVM_SEV_SNP_INIT);
> > > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > > >>>>           int asid, ret;
> > > >>>>
> > > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ static
> > > >>>> int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > > >>>>                   return ret;
> > > >>>>
> > > >>>>           sev->es_active = es_active;
> > > >>>> +       sev->snp_active = snp_active;
> > > >>>>           asid = sev_asid_new(sev);
> > > >>>>           if (asid < 0)
> > > >>>>                   goto e_no_asid;
> > > >>>>           sev->asid = asid;
> > > >>>>
> > > >>>> -       ret = sev_platform_init(&argp->error);
> > > >>>> +       if (snp_active) {
> > > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > > >>>> +               if (ret)
> > > >>>> +                       goto e_free;
> > > >>>> +
> > > >>>> +               ret = sev_snp_init(&argp->error);
> > > >>>> +       } else {
> > > >>>> +               ret = sev_platform_init(&argp->error);
> > > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > >>> In my tests, if SEV has not been initialized in the platform
> > > >>> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried
> > > >>> calling SEV_DF_FLUSH right after the SNP platform init but this
> > > >>> time it failed later on the SNP launch update command with
> > > >>> SEV_RET_INVALID_PARAM error. Looks like there is another
> > > >>> dependency on SEV platform initialization.
> > > >>>
> > > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > > >> Trying to get some more context for this issue.
> > > >>
> > > >> When you say after SEV_INIT_EX support patches, SEV may be
> > > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > > >> sev_platform_init() code path has still not executed on the host BSP ?
> > > >>
> > > > Correct, INIT_EX requires the file system to be ready and there is
> > > > a ccp module param to call it only when needed.
> > > >
> > > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be
> > > > initialized on module init. Else the PSP will be initialized on
> > > > the first command requiring it");
> > > >
> > > > If this module param is false, it won't initialize SEV on the
> > > > platform until the first SEV VM.
> > > >
> > > Ok, that makes sense.
> > >
> > > So the fix will be to call sev_platform_init() unconditionally here
> > > in sev_guest_init(), and both sev_snp_init() and sev_platform_init()
> > > are protected from being called again, so there won't be any issues
> > > if these functions are invoked again at SNP/SEV VM launch if they
> > > have been invoked earlier during module init.
> >
> > >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
> >
> > Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.
>
> > But if psp_init_on_probe is false, sev_platform_init() isn't called down this path. Alper has suggested we always call sev_platform_init() but we could just place an SEV_DF_FLUSH command instead. Or am I still missing something?
>
> >After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > In my tests, if SEV has not been initialized in the platform
> > yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried
> > calling SEV_DF_FLUSH right after the SNP platform init.
>
> Are you getting the DLFLUSH_REQUIRED error after the SNP activate command ?
>
> Also did you use the SEV_DF_FLUSH command or the SNP_DF_FLUSH command ?
>
> With SNP you need to use SNP_DF_FLUSH command.
>
> Thanks,
> Ashish

Kalra, Ashish June 14, 2022, 8:23 p.m. UTC | #15

[AMD Official Use Only - General]

Hello Alper,

Here is the feedback from the SEV/SNP firmware team:

The SNP spec has this line in SNP_INIT_EX:

“The firmware marks all encryption capable ASIDs as unusable for encrypted virtualization.”

This is a back-handed way of saying that after SNP_INIT, all of the ASIDs are considered “dirty”. None are in use, but the FW can’t know if there’s any data in the caches left over from some prior guest. So after doing an SNP_INIT (or SNP_INIT_EX), a WBINVD (on all threads) and DF_FLUSH sequence is required. It doesn’t matter whether it’s an SEV DF_FLUSH or an SNP_DF_FLUSH… they both do EXACTLY the same thing.

I don’t understand off hand why SNP_LAUNCH_START would require a DF_FLUSH… Usually, it’s only an “activate” command that requires the DF_FLUSH. ACTIVATE/ACTIVATE_EX/SNP_ACTIVATE/SNP_ACTIVATE_EX

[Ashish] That's why I asked if you are getting the DLFLUSH_REQUIRED error after the SNP activate command ?

It’s when you attempt to (re-)use >an ASID that’s “dirty” that you should get the DF_FLUSH_REQUIRED error.

[Ashish] And we do SNP_DF_FLUSH/SEV_DF_FLUSH whenever ASIDs are reused/re-cycled.

If the host only wants to run SNP guests, there’s no need to do an SEV INIT or any other SEV operations. If the host DOES want to run SEV AND SNP guests, then the required sequence is to do the SNP_INIT before the SEV INIT. Doing the WBINVD/DF_FLUSH any time between the SNP_INIT and any ACTIVATE command should be sufficient.

Thanks,
Ashish

-----Original Message-----
From: Alper Gun <[e-mail]> 
Sent: Tuesday, June 14, 2022 1:58 PM
To: Kalra, Ashish <[e-mail]>
Cc: Peter Gonda <[e-mail]>; the arch/x86 maintainers <[e-mail]>; LKML <[e-mail]>; kvm list <[e-mail]>; [e-mail]; [e-mail]; Linux Crypto Mailing List <[e-mail]>; Thomas Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo Bonzini <[e-mail]>; Sean Christopherson <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave Hansen <[e-mail]>; Sergio Lopez <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas Pandruvada <[e-mail]>; David Rientjes <[e-mail]>; Dov Murik <[e-mail]>; Tobin Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; Roth, Michael <[e-mail]>; Vlastimil Babka <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr <[e-mail]>; Sathyanarayanan Kuppuswamy <[e-mail]>
Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command

Let me summarize what I tried.

1- when using psp_init_probe false, the SNP VM fails in SNP_LAUNCH_START step with error SEV_RET_DFFLUSH_REQUIRED(15).
2- added SEV_DF_FLUSH just after SNP platform init and it didn't fail in launch start but failed later during SNP_LAUNCH_UPDATE with
SEV_RET_INVALID_PARAM(22)
3- added SNP_DF_FLUSH just after SNP platform init and it failed again during SNP_LAUNCH_UPDATE with SEV_RET_INVALID_PARAM(22)
4- added sev_platform_init for SNP VMs and it worked.

For me DF_FLUSH alone didn' help boot a VM. I don't know yet why sev platform status impacts the SNP VM, but sev_platform_init fixes the problem.


On Tue, Jun 14, 2022 at 10:16 AM Kalra, Ashish <[e-mail]> wrote:
>
> [AMD Official Use Only - General]
>
> Hello Alper, Peter,
>
> -----Original Message-----
> From: Peter Gonda <[e-mail]>
> Sent: Tuesday, June 14, 2022 11:30 AM
> To: Kalra, Ashish <[e-mail]>
> Cc: Alper Gun <[e-mail]>; Brijesh Singh 
> <[e-mail]>; the arch/x86 maintainers <[e-mail]>; 
> LKML <[e-mail]>; kvm list <[e-mail]>; 
> [e-mail]; [e-mail]; Linux Crypto Mailing 
> List <[e-mail]>; Thomas Gleixner 
> <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel 
> <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. 
> Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo 
> Bonzini <[e-mail]>; Sean Christopherson 
> <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng 
> Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy 
> Lutomirski <[e-mail]>; Dave Hansen 
> <[e-mail]>; Sergio Lopez <[e-mail]>; Peter 
> Zijlstra <[e-mail]>; Srinivas Pandruvada 
> <[e-mail]>; David Rientjes 
> <[e-mail]>; Dov Murik <[e-mail]>; Tobin 
> Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; 
> Roth, Michael <[e-mail]>; Vlastimil Babka 
> <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi 
> Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr 
> <[e-mail]>; Sathyanarayanan Kuppuswamy 
> <[e-mail]>; Pavan Kumar Paluri 
> <[e-mail]>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <[e-mail]> wrote:
> >
> > [AMD Official Use Only - General]
> >
> >
> > -----Original Message-----
> > From: Peter Gonda <[e-mail]>
> > Sent: Tuesday, June 14, 2022 10:38 AM
> > To: Kalra, Ashish <[e-mail]>
> > Cc: Alper Gun <[e-mail]>; Brijesh Singh 
> > <[e-mail]>; Kalra, Ashish <[e-mail]>; the
> > arch/x86 maintainers <[e-mail]>; LKML 
> > <[e-mail]>; kvm list <[e-mail]>; 
> > [e-mail]; [e-mail]; Linux Crypto Mailing 
> > List <[e-mail]>; Thomas Gleixner 
> > <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel 
> > <[e-mail]>; Lendacky, Thomas <[e-mail]>; H.
> > Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo 
> > Bonzini <[e-mail]>; Sean Christopherson 
> > <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng 
> > Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy 
> > Lutomirski <[e-mail]>; Dave Hansen 
> > <[e-mail]>; Sergio Lopez <[e-mail]>; Peter 
> > Zijlstra <[e-mail]>; Srinivas Pandruvada 
> > <[e-mail]>; David Rientjes 
> > <[e-mail]>; Dov Murik <[e-mail]>; Tobin 
> > Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; 
> > Roth, Michael <[e-mail]>; Vlastimil Babka 
> > <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi 
> > Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc 
> > Orr <[e-mail]>; Sathyanarayanan Kuppuswamy 
> > <[e-mail]>; Pavan Kumar Paluri 
> > <[e-mail]>
> > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT 
> > command
> >
> > On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
> > >
> > >
> > > On 6/13/22 23:33, Alper Gun wrote:
> > > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> > > >> Hello Alper,
> > > >>
> > > >> On 6/13/22 20:58, Alper Gun wrote:
> > > >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd
> > > >>> *argp)
> > > >>>>    {
> > > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT ||
> > > >>>> + argp->id == KVM_SEV_SNP_INIT);
> > > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > > >>>>           int asid, ret;
> > > >>>>
> > > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ 
> > > >>>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > > >>>>                   return ret;
> > > >>>>
> > > >>>>           sev->es_active = es_active;
> > > >>>> +       sev->snp_active = snp_active;
> > > >>>>           asid = sev_asid_new(sev);
> > > >>>>           if (asid < 0)
> > > >>>>                   goto e_no_asid;
> > > >>>>           sev->asid = asid;
> > > >>>>
> > > >>>> -       ret = sev_platform_init(&argp->error);
> > > >>>> +       if (snp_active) {
> > > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > > >>>> +               if (ret)
> > > >>>> +                       goto e_free;
> > > >>>> +
> > > >>>> +               ret = sev_snp_init(&argp->error);
> > > >>>> +       } else {
> > > >>>> +               ret = sev_platform_init(&argp->error);
> > > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > >>> In my tests, if SEV has not been initialized in the platform 
> > > >>> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried 
> > > >>> calling SEV_DF_FLUSH right after the SNP platform init but 
> > > >>> this time it failed later on the SNP launch update command 
> > > >>> with SEV_RET_INVALID_PARAM error. Looks like there is another 
> > > >>> dependency on SEV platform initialization.
> > > >>>
> > > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > > >> Trying to get some more context for this issue.
> > > >>
> > > >> When you say after SEV_INIT_EX support patches, SEV may be 
> > > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > > >> sev_platform_init() code path has still not executed on the host BSP ?
> > > >>
> > > > Correct, INIT_EX requires the file system to be ready and there 
> > > > is a ccp module param to call it only when needed.
> > > >
> > > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be 
> > > > initialized on module init. Else the PSP will be initialized on 
> > > > the first command requiring it");
> > > >
> > > > If this module param is false, it won't initialize SEV on the 
> > > > platform until the first SEV VM.
> > > >
> > > Ok, that makes sense.
> > >
> > > So the fix will be to call sev_platform_init() unconditionally 
> > > here in sev_guest_init(), and both sev_snp_init() and 
> > > sev_platform_init() are protected from being called again, so 
> > > there won't be any issues if these functions are invoked again at 
> > > SNP/SEV VM launch if they have been invoked earlier during module init.
> >
> > >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
> >
> > Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.
>
> > But if psp_init_on_probe is false, sev_platform_init() isn't called down this path. Alper has suggested we always call sev_platform_init() but we could just place an SEV_DF_FLUSH command instead. Or am I still missing something?
>
> >After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > In my tests, if SEV has not been initialized in the platform  yet, 
> >SNP VMs fail with SEV_DF_FLUSH required error. I tried  calling 
> >SEV_DF_FLUSH right after the SNP platform init.
>
> Are you getting the DLFLUSH_REQUIRED error after the SNP activate command ?
>
> Also did you use the SEV_DF_FLUSH command or the SNP_DF_FLUSH command ?
>
> With SNP you need to use SNP_DF_FLUSH command.
>
> Thanks,
> Ashish

Peter Gonda June 14, 2022, 8:29 p.m. UTC | #16

On Tue, Jun 14, 2022 at 2:23 PM Kalra, Ashish <[e-mail]> wrote:
>
> [AMD Official Use Only - General]
>
> Hello Alper,
>
> Here is the feedback from the SEV/SNP firmware team:
>
> The SNP spec has this line in SNP_INIT_EX:
>
> “The firmware marks all encryption capable ASIDs as unusable for encrypted virtualization.”
>
> This is a back-handed way of saying that after SNP_INIT, all of the ASIDs are considered “dirty”. None are in use, but the FW can’t know if there’s any data in the caches left over from some prior guest. So after doing an SNP_INIT (or SNP_INIT_EX), a WBINVD (on all threads) and DF_FLUSH sequence is required. It doesn’t matter whether it’s an SEV DF_FLUSH or an SNP_DF_FLUSH… they both do EXACTLY the same thing.
>
> I don’t understand off hand why SNP_LAUNCH_START would require a DF_FLUSH… Usually, it’s only an “activate” command that requires the DF_FLUSH. ACTIVATE/ACTIVATE_EX/SNP_ACTIVATE/SNP_ACTIVATE_EX
>
> [Ashish] That's why I asked if you are getting the DLFLUSH_REQUIRED error after the SNP activate command ?
>
> It’s when you attempt to (re-)use >an ASID that’s “dirty” that you should get the DF_FLUSH_REQUIRED error.
>
> [Ashish] And we do SNP_DF_FLUSH/SEV_DF_FLUSH whenever ASIDs are reused/re-cycled.
>
> If the host only wants to run SNP guests, there’s no need to do an SEV INIT or any other SEV operations. If the host DOES want to run SEV AND SNP guests, then the required sequence is to do the SNP_INIT before the SEV INIT. Doing the WBINVD/DF_FLUSH any time between the SNP_INIT and any ACTIVATE command should be sufficient.

Thanks for the follow up Ashish! I was assuming there was some issue
with the ASIDs here but didn't have time to experiment yet. Is there
any downside to enabling SEV? If not these patches make sense but if
there is some cost should we make KVM capable of running SNP VMs
without enabling SEV?


>
> Thanks,
> Ashish
>
> -----Original Message-----
> From: Alper Gun <[e-mail]>
> Sent: Tuesday, June 14, 2022 1:58 PM
> To: Kalra, Ashish <[e-mail]>
> Cc: Peter Gonda <[e-mail]>; the arch/x86 maintainers <[e-mail]>; LKML <[e-mail]>; kvm list <[e-mail]>; [e-mail]; [e-mail]; Linux Crypto Mailing List <[e-mail]>; Thomas Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo Bonzini <[e-mail]>; Sean Christopherson <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave Hansen <[e-mail]>; Sergio Lopez <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas Pandruvada <[e-mail]>; David Rientjes <[e-mail]>; Dov Murik <[e-mail]>; Tobin Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; Roth, Michael <[e-mail]>; Vlastimil Babka <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr <[e-mail]>; Sathyanarayanan Kuppuswamy <[e-mail]>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> Let me summarize what I tried.
>
> 1- when using psp_init_probe false, the SNP VM fails in SNP_LAUNCH_START step with error SEV_RET_DFFLUSH_REQUIRED(15).
> 2- added SEV_DF_FLUSH just after SNP platform init and it didn't fail in launch start but failed later during SNP_LAUNCH_UPDATE with
> SEV_RET_INVALID_PARAM(22)
> 3- added SNP_DF_FLUSH just after SNP platform init and it failed again during SNP_LAUNCH_UPDATE with SEV_RET_INVALID_PARAM(22)
> 4- added sev_platform_init for SNP VMs and it worked.
>
> For me DF_FLUSH alone didn' help boot a VM. I don't know yet why sev platform status impacts the SNP VM, but sev_platform_init fixes the problem.
>
>
> On Tue, Jun 14, 2022 at 10:16 AM Kalra, Ashish <[e-mail]> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > Hello Alper, Peter,
> >
> > -----Original Message-----
> > From: Peter Gonda <[e-mail]>
> > Sent: Tuesday, June 14, 2022 11:30 AM
> > To: Kalra, Ashish <[e-mail]>
> > Cc: Alper Gun <[e-mail]>; Brijesh Singh
> > <[e-mail]>; the arch/x86 maintainers <[e-mail]>;
> > LKML <[e-mail]>; kvm list <[e-mail]>;
> > [e-mail]; [e-mail]; Linux Crypto Mailing
> > List <[e-mail]>; Thomas Gleixner
> > <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel
> > <[e-mail]>; Lendacky, Thomas <[e-mail]>; H.
> > Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo
> > Bonzini <[e-mail]>; Sean Christopherson
> > <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng
> > Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy
> > Lutomirski <[e-mail]>; Dave Hansen
> > <[e-mail]>; Sergio Lopez <[e-mail]>; Peter
> > Zijlstra <[e-mail]>; Srinivas Pandruvada
> > <[e-mail]>; David Rientjes
> > <[e-mail]>; Dov Murik <[e-mail]>; Tobin
> > Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>;
> > Roth, Michael <[e-mail]>; Vlastimil Babka
> > <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi
> > Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr
> > <[e-mail]>; Sathyanarayanan Kuppuswamy
> > <[e-mail]>; Pavan Kumar Paluri
> > <[e-mail]>
> > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
> >
> > On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <[e-mail]> wrote:
> > >
> > > [AMD Official Use Only - General]
> > >
> > >
> > > -----Original Message-----
> > > From: Peter Gonda <[e-mail]>
> > > Sent: Tuesday, June 14, 2022 10:38 AM
> > > To: Kalra, Ashish <[e-mail]>
> > > Cc: Alper Gun <[e-mail]>; Brijesh Singh
> > > <[e-mail]>; Kalra, Ashish <[e-mail]>; the
> > > arch/x86 maintainers <[e-mail]>; LKML
> > > <[e-mail]>; kvm list <[e-mail]>;
> > > [e-mail]; [e-mail]; Linux Crypto Mailing
> > > List <[e-mail]>; Thomas Gleixner
> > > <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel
> > > <[e-mail]>; Lendacky, Thomas <[e-mail]>; H.
> > > Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo
> > > Bonzini <[e-mail]>; Sean Christopherson
> > > <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng
> > > Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy
> > > Lutomirski <[e-mail]>; Dave Hansen
> > > <[e-mail]>; Sergio Lopez <[e-mail]>; Peter
> > > Zijlstra <[e-mail]>; Srinivas Pandruvada
> > > <[e-mail]>; David Rientjes
> > > <[e-mail]>; Dov Murik <[e-mail]>; Tobin
> > > Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>;
> > > Roth, Michael <[e-mail]>; Vlastimil Babka
> > > <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi
> > > Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc
> > > Orr <[e-mail]>; Sathyanarayanan Kuppuswamy
> > > <[e-mail]>; Pavan Kumar Paluri
> > > <[e-mail]>
> > > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT
> > > command
> > >
> > > On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
> > > >
> > > >
> > > > On 6/13/22 23:33, Alper Gun wrote:
> > > > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> > > > >> Hello Alper,
> > > > >>
> > > > >> On 6/13/22 20:58, Alper Gun wrote:
> > > > >>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd
> > > > >>> *argp)
> > > > >>>>    {
> > > > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT ||
> > > > >>>> + argp->id == KVM_SEV_SNP_INIT);
> > > > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > > > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > > > >>>>           int asid, ret;
> > > > >>>>
> > > > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@
> > > > >>>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > > > >>>>                   return ret;
> > > > >>>>
> > > > >>>>           sev->es_active = es_active;
> > > > >>>> +       sev->snp_active = snp_active;
> > > > >>>>           asid = sev_asid_new(sev);
> > > > >>>>           if (asid < 0)
> > > > >>>>                   goto e_no_asid;
> > > > >>>>           sev->asid = asid;
> > > > >>>>
> > > > >>>> -       ret = sev_platform_init(&argp->error);
> > > > >>>> +       if (snp_active) {
> > > > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > > > >>>> +               if (ret)
> > > > >>>> +                       goto e_free;
> > > > >>>> +
> > > > >>>> +               ret = sev_snp_init(&argp->error);
> > > > >>>> +       } else {
> > > > >>>> +               ret = sev_platform_init(&argp->error);
> > > > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > > >>> In my tests, if SEV has not been initialized in the platform
> > > > >>> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried
> > > > >>> calling SEV_DF_FLUSH right after the SNP platform init but
> > > > >>> this time it failed later on the SNP launch update command
> > > > >>> with SEV_RET_INVALID_PARAM error. Looks like there is another
> > > > >>> dependency on SEV platform initialization.
> > > > >>>
> > > > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > > > >> Trying to get some more context for this issue.
> > > > >>
> > > > >> When you say after SEV_INIT_EX support patches, SEV may be
> > > > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > > > >> sev_platform_init() code path has still not executed on the host BSP ?
> > > > >>
> > > > > Correct, INIT_EX requires the file system to be ready and there
> > > > > is a ccp module param to call it only when needed.
> > > > >
> > > > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be
> > > > > initialized on module init. Else the PSP will be initialized on
> > > > > the first command requiring it");
> > > > >
> > > > > If this module param is false, it won't initialize SEV on the
> > > > > platform until the first SEV VM.
> > > > >
> > > > Ok, that makes sense.
> > > >
> > > > So the fix will be to call sev_platform_init() unconditionally
> > > > here in sev_guest_init(), and both sev_snp_init() and
> > > > sev_platform_init() are protected from being called again, so
> > > > there won't be any issues if these functions are invoked again at
> > > > SNP/SEV VM launch if they have been invoked earlier during module init.
> > >
> > > >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
> > >
> > > Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.
> >
> > > But if psp_init_on_probe is false, sev_platform_init() isn't called down this path. Alper has suggested we always call sev_platform_init() but we could just place an SEV_DF_FLUSH command instead. Or am I still missing something?
> >
> > >After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > In my tests, if SEV has not been initialized in the platform  yet,
> > >SNP VMs fail with SEV_DF_FLUSH required error. I tried  calling
> > >SEV_DF_FLUSH right after the SNP platform init.
> >
> > Are you getting the DLFLUSH_REQUIRED error after the SNP activate command ?
> >
> > Also did you use the SEV_DF_FLUSH command or the SNP_DF_FLUSH command ?
> >
> > With SNP you need to use SNP_DF_FLUSH command.
> >
> > Thanks,
> > Ashish

Kalra, Ashish June 14, 2022, 8:39 p.m. UTC | #17

[AMD Official Use Only - General]

Hello Peter,

>> Here is the feedback from the SEV/SNP firmware team:
>>
>> The SNP spec has this line in SNP_INIT_EX:
>>
>> “The firmware marks all encryption capable ASIDs as unusable for encrypted virtualization.”
>>
>> This is a back-handed way of saying that after SNP_INIT, all of the ASIDs are considered “dirty”. None are in use, but the FW can’t know if there’s any data in the caches left over from some prior guest. So after doing an SNP_INIT (or SNP_INIT_EX), a WBINVD (on all threads) and DF_FLUSH sequence is required. It doesn’t matter whether it’s an SEV DF_FLUSH or an SNP_DF_FLUSH… they both do EXACTLY the same thing.
>>
>> I don’t understand off hand why SNP_LAUNCH_START would require a 
>> DF_FLUSH… Usually, it’s only an “activate” command that requires the 
>> DF_FLUSH. ACTIVATE/ACTIVATE_EX/SNP_ACTIVATE/SNP_ACTIVATE_EX
>>
>> [Ashish] That's why I asked if you are getting the DLFLUSH_REQUIRED error after the SNP activate command ?
>>
>> It’s when you attempt to (re-)use >an ASID that’s “dirty” that you should get the DF_FLUSH_REQUIRED error.
>>
>> [Ashish] And we do SNP_DF_FLUSH/SEV_DF_FLUSH whenever ASIDs are reused/re-cycled.
>>
>> If the host only wants to run SNP guests, there’s no need to do an SEV INIT or any other SEV operations. If the host DOES want to run SEV AND SNP guests, then the required sequence is to do the SNP_INIT before the SEV INIT. Doing the >>WBINVD/DF_FLUSH any time between the SNP_INIT and any ACTIVATE command should be sufficient.

>Thanks for the follow up Ashish! I was assuming there was some issue with the ASIDs here but didn't have time to experiment yet.

Yes, it looks to be some issue with ASIDs.

> Is there any downside to enabling SEV? If not these patches make sense but if there is some cost should we make KVM capable of running SNP VMs without enabling SEV?

As mentioned above we only need to do this if the host wants to run both SEV and SNP guests. I assume the cost should not be considered as this is only a init or guest launch time addition.

Thanks,
Ashish

> -----Original Message-----
> From: Alper Gun <[e-mail]>
> Sent: Tuesday, June 14, 2022 1:58 PM
> To: Kalra, Ashish <[e-mail]>
> Cc: Peter Gonda <[e-mail]>; the arch/x86 maintainers 
> <[e-mail]>; LKML <[e-mail]>; kvm list 
> <[e-mail]>; [e-mail]; [e-mail]; 
> Linux Crypto Mailing List <[e-mail]>; Thomas 
> Gleixner <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg 
> Roedel <[e-mail]>; Lendacky, Thomas <[e-mail]>; 
> H. Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; 
> Paolo Bonzini <[e-mail]>; Sean Christopherson 
> <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng 
> Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy 
> Lutomirski <[e-mail]>; Dave Hansen 
> <[e-mail]>; Sergio Lopez <[e-mail]>; Peter 
> Zijlstra <[e-mail]>; Srinivas Pandruvada 
> <[e-mail]>; David Rientjes 
> <[e-mail]>; Dov Murik <[e-mail]>; Tobin 
> Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; 
> Roth, Michael <[e-mail]>; Vlastimil Babka 
> <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi 
> Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc Orr 
> <[e-mail]>; Sathyanarayanan Kuppuswamy 
> <[e-mail]>
> Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT command
>
> Let me summarize what I tried.
>
> 1- when using psp_init_probe false, the SNP VM fails in SNP_LAUNCH_START step with error SEV_RET_DFFLUSH_REQUIRED(15).
> 2- added SEV_DF_FLUSH just after SNP platform init and it didn't fail 
> in launch start but failed later during SNP_LAUNCH_UPDATE with
> SEV_RET_INVALID_PARAM(22)
> 3- added SNP_DF_FLUSH just after SNP platform init and it failed again 
> during SNP_LAUNCH_UPDATE with SEV_RET_INVALID_PARAM(22)
> 4- added sev_platform_init for SNP VMs and it worked.
>
> For me DF_FLUSH alone didn' help boot a VM. I don't know yet why sev platform status impacts the SNP VM, but sev_platform_init fixes the problem.
>
>
> On Tue, Jun 14, 2022 at 10:16 AM Kalra, Ashish <[e-mail]> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > Hello Alper, Peter,
> >
> > -----Original Message-----
> > From: Peter Gonda <[e-mail]>
> > Sent: Tuesday, June 14, 2022 11:30 AM
> > To: Kalra, Ashish <[e-mail]>
> > Cc: Alper Gun <[e-mail]>; Brijesh Singh 
> > <[e-mail]>; the arch/x86 maintainers <[e-mail]>; 
> > LKML <[e-mail]>; kvm list <[e-mail]>; 
> > [e-mail]; [e-mail]; Linux Crypto Mailing 
> > List <[e-mail]>; Thomas Gleixner 
> > <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel 
> > <[e-mail]>; Lendacky, Thomas <[e-mail]>; H.
> > Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; Paolo 
> > Bonzini <[e-mail]>; Sean Christopherson 
> > <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; Wanpeng 
> > Li <[e-mail]>; Jim Mattson <[e-mail]>; Andy 
> > Lutomirski <[e-mail]>; Dave Hansen 
> > <[e-mail]>; Sergio Lopez <[e-mail]>; Peter 
> > Zijlstra <[e-mail]>; Srinivas Pandruvada 
> > <[e-mail]>; David Rientjes 
> > <[e-mail]>; Dov Murik <[e-mail]>; Tobin 
> > Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; 
> > Roth, Michael <[e-mail]>; Vlastimil Babka 
> > <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi 
> > Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc 
> > Orr <[e-mail]>; Sathyanarayanan Kuppuswamy 
> > <[e-mail]>; Pavan Kumar Paluri 
> > <[e-mail]>
> > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT 
> > command
> >
> > On Tue, Jun 14, 2022 at 10:11 AM Kalra, Ashish <[e-mail]> wrote:
> > >
> > > [AMD Official Use Only - General]
> > >
> > >
> > > -----Original Message-----
> > > From: Peter Gonda <[e-mail]>
> > > Sent: Tuesday, June 14, 2022 10:38 AM
> > > To: Kalra, Ashish <[e-mail]>
> > > Cc: Alper Gun <[e-mail]>; Brijesh Singh 
> > > <[e-mail]>; Kalra, Ashish <[e-mail]>; the
> > > arch/x86 maintainers <[e-mail]>; LKML 
> > > <[e-mail]>; kvm list <[e-mail]>; 
> > > [e-mail]; [e-mail]; Linux Crypto 
> > > Mailing List <[e-mail]>; Thomas Gleixner 
> > > <[e-mail]>; Ingo Molnar <[e-mail]>; Joerg Roedel 
> > > <[e-mail]>; Lendacky, Thomas <[e-mail]>; H.
> > > Peter Anvin <[e-mail]>; Ard Biesheuvel <[e-mail]>; 
> > > Paolo Bonzini <[e-mail]>; Sean Christopherson 
> > > <[e-mail]>; Vitaly Kuznetsov <[e-mail]>; 
> > > Wanpeng Li <[e-mail]>; Jim Mattson 
> > > <[e-mail]>; Andy Lutomirski <[e-mail]>; Dave 
> > > Hansen <[e-mail]>; Sergio Lopez 
> > > <[e-mail]>; Peter Zijlstra <[e-mail]>; Srinivas 
> > > Pandruvada <[e-mail]>; David Rientjes 
> > > <[e-mail]>; Dov Murik <[e-mail]>; Tobin 
> > > Feldman-Fitzthum <[e-mail]>; Borislav Petkov <[e-mail]>; 
> > > Roth, Michael <[e-mail]>; Vlastimil Babka 
> > > <[e-mail]>; Kirill A . Shutemov <[e-mail]>; Andi 
> > > Kleen <[e-mail]>; Tony Luck <[e-mail]>; Marc 
> > > Orr <[e-mail]>; Sathyanarayanan Kuppuswamy 
> > > <[e-mail]>; Pavan Kumar Paluri 
> > > <[e-mail]>
> > > Subject: Re: [PATCH Part2 v5 23/45] KVM: SVM: Add KVM_SNP_INIT 
> > > command
> > >
> > > On Mon, Jun 13, 2022 at 6:21 PM Ashish Kalra <[e-mail]> wrote:
> > > >
> > > >
> > > > On 6/13/22 23:33, Alper Gun wrote:
> > > > > On Mon, Jun 13, 2022 at 4:15 PM Ashish Kalra <[e-mail]> wrote:
> > > > >> Hello Alper,
> > > > >>
> > > > >> On 6/13/22 20:58, Alper Gun wrote:
> > > > >>> static int sev_guest_init(struct kvm *kvm, struct 
> > > > >>> kvm_sev_cmd
> > > > >>> *argp)
> > > > >>>>    {
> > > > >>>> +       bool es_active = (argp->id == KVM_SEV_ES_INIT ||
> > > > >>>> + argp->id == KVM_SEV_SNP_INIT);
> > > > >>>>           struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > > >>>> -       bool es_active = argp->id == KVM_SEV_ES_INIT;
> > > > >>>> +       bool snp_active = argp->id == KVM_SEV_SNP_INIT;
> > > > >>>>           int asid, ret;
> > > > >>>>
> > > > >>>>           if (kvm->created_vcpus) @@ -249,12 +269,22 @@ 
> > > > >>>> static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > > > >>>>                   return ret;
> > > > >>>>
> > > > >>>>           sev->es_active = es_active;
> > > > >>>> +       sev->snp_active = snp_active;
> > > > >>>>           asid = sev_asid_new(sev);
> > > > >>>>           if (asid < 0)
> > > > >>>>                   goto e_no_asid;
> > > > >>>>           sev->asid = asid;
> > > > >>>>
> > > > >>>> -       ret = sev_platform_init(&argp->error);
> > > > >>>> +       if (snp_active) {
> > > > >>>> +               ret = verify_snp_init_flags(kvm, argp);
> > > > >>>> +               if (ret)
> > > > >>>> +                       goto e_free;
> > > > >>>> +
> > > > >>>> +               ret = sev_snp_init(&argp->error);
> > > > >>>> +       } else {
> > > > >>>> +               ret = sev_platform_init(&argp->error);
> > > > >>> After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > > >>> In my tests, if SEV has not been initialized in the platform 
> > > > >>> yet, SNP VMs fail with SEV_DF_FLUSH required error. I tried 
> > > > >>> calling SEV_DF_FLUSH right after the SNP platform init but 
> > > > >>> this time it failed later on the SNP launch update command 
> > > > >>> with SEV_RET_INVALID_PARAM error. Looks like there is 
> > > > >>> another dependency on SEV platform initialization.
> > > > >>>
> > > > >>> Calling sev_platform_init for SNP VMs fixes the problem in our tests.
> > > > >> Trying to get some more context for this issue.
> > > > >>
> > > > >> When you say after SEV_INIT_EX support patches, SEV may be 
> > > > >> initialized in the platform late, do you mean sev_pci_init()->sev_snp_init() ...
> > > > >> sev_platform_init() code path has still not executed on the host BSP ?
> > > > >>
> > > > > Correct, INIT_EX requires the file system to be ready and 
> > > > > there is a ccp module param to call it only when needed.
> > > > >
> > > > > MODULE_PARM_DESC(psp_init_on_probe, " if true, the PSP will be 
> > > > > initialized on module init. Else the PSP will be initialized 
> > > > > on the first command requiring it");
> > > > >
> > > > > If this module param is false, it won't initialize SEV on the 
> > > > > platform until the first SEV VM.
> > > > >
> > > > Ok, that makes sense.
> > > >
> > > > So the fix will be to call sev_platform_init() unconditionally 
> > > > here in sev_guest_init(), and both sev_snp_init() and
> > > > sev_platform_init() are protected from being called again, so 
> > > > there won't be any issues if these functions are invoked again 
> > > > at SNP/SEV VM launch if they have been invoked earlier during module init.
> > >
> > > >That's one solution. I don't know if there is a downside to the system for enabling SEV if SNP is being enabled but another solution could be to just directly place a DF_FLUSH command instead of calling sev_platform_init().
> > >
> > > Actually sev_platform_init() is already called on module init if psp_init_on_probe is not false. Only need to ensure that SNP firmware is initialized first with SNP_INIT command.
> >
> > > But if psp_init_on_probe is false, sev_platform_init() isn't called down this path. Alper has suggested we always call sev_platform_init() but we could just place an SEV_DF_FLUSH command instead. Or am I still missing something?
> >
> > >After SEV INIT_EX support patches, SEV may be initialized in the platform late.
> > > In my tests, if SEV has not been initialized in the platform  yet, 
> > >SNP VMs fail with SEV_DF_FLUSH required error. I tried  calling 
> > >SEV_DF_FLUSH right after the SNP platform init.
> >
> > Are you getting the DLFLUSH_REQUIRED error after the SNP activate command ?
> >
> > Also did you use the SEV_DF_FLUSH command or the SNP_DF_FLUSH command ?
> >
> > With SNP you need to use SNP_DF_FLUSH command.
> >
> > Thanks,
> > Ashish

Patch

diff --git a/Documentation/virt/kvm/amd-memory-encryption.rst b/Documentation/virt/kvm/amd-memory-encryption.rst
index 5c081c8c7164..7b1d32fb99a8 100644
--- a/Documentation/virt/kvm/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/amd-memory-encryption.rst
@@ -427,6 +427,33 @@  issued by the hypervisor to make the guest ready for execution.
 
 Returns: 0 on success, -negative on error
 
+18. KVM_SNP_INIT
+----------------
+
+The KVM_SNP_INIT command can be used by the hypervisor to initialize SEV-SNP
+context. In a typical workflow, this command should be the first command issued.
+
+Parameters (in/out): struct kvm_snp_init
+
+Returns: 0 on success, -negative on error
+
+::
+
+        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)
+
+If the specified flags is not supported then return -EOPNOTSUPP, and the supported
+flags are returned.
+
 References
 ==========
 
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index 44a3f920f886..a39e31845a33 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -218,6 +218,8 @@  struct __attribute__ ((__packed__)) vmcb_control_area {
 #define SVM_NESTED_CTL_SEV_ENABLE	BIT(1)
 #define SVM_NESTED_CTL_SEV_ES_ENABLE	BIT(2)
 
+#define SVM_SEV_FEAT_SNP_ACTIVE		BIT(0)
+
 struct vmcb_seg {
 	u16 selector;
 	u16 attrib;
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 50fddbe56981..93da463545ef 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -235,10 +235,30 @@  static void sev_unbind_asid(struct kvm *kvm, unsigned int handle)
 	sev_decommission(handle);
 }
 
+static int verify_snp_init_flags(struct kvm *kvm, struct kvm_sev_cmd *argp)
+{
+	struct kvm_snp_init params;
+	int ret = 0;
+
+	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data, sizeof(params)))
+		return -EFAULT;
+
+	if (params.flags & ~SEV_SNP_SUPPORTED_FLAGS)
+		ret = -EOPNOTSUPP;
+
+	params.flags = SEV_SNP_SUPPORTED_FLAGS;
+
+	if (copy_to_user((void __user *)(uintptr_t)argp->data, &params, sizeof(params)))
+		ret = -EFAULT;
+
+	return ret;
+}
+
 static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 {
+	bool es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
-	bool es_active = argp->id == KVM_SEV_ES_INIT;
+	bool snp_active = argp->id == KVM_SEV_SNP_INIT;
 	int asid, ret;
 
 	if (kvm->created_vcpus)
@@ -249,12 +269,22 @@  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		return ret;
 
 	sev->es_active = es_active;
+	sev->snp_active = snp_active;
 	asid = sev_asid_new(sev);
 	if (asid < 0)
 		goto e_no_asid;
 	sev->asid = asid;
 
-	ret = sev_platform_init(&argp->error);
+	if (snp_active) {
+		ret = verify_snp_init_flags(kvm, argp);
+		if (ret)
+			goto e_free;
+
+		ret = sev_snp_init(&argp->error);
+	} else {
+		ret = sev_platform_init(&argp->error);
+	}
+
 	if (ret)
 		goto e_free;
 
@@ -600,6 +630,10 @@  static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	save->pkru = svm->vcpu.arch.pkru;
 	save->xss  = svm->vcpu.arch.ia32_xss;
 
+	/* Enable the SEV-SNP feature */
+	if (sev_snp_guest(svm->vcpu.kvm))
+		save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
+
 	return 0;
 }
 
@@ -1532,6 +1566,12 @@  int svm_mem_enc_op(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 01953522097d..57c3c404b0b3 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -69,6 +69,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 */
@@ -81,6 +84,7 @@  struct kvm_sev_info {
 	u64 ap_jump_table;	/* SEV-ES AP Jump Table address */
 	struct kvm *enc_context_owner; /* Owner of copied encryption context */
 	struct misc_cg *misc_cg; /* For misc cgroup accounting */
+	u64 snp_init_flags;
 };
 
 struct kvm_svm {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index d9e4aabcb31a..944e2bf601fe 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1712,6 +1712,9 @@  enum sev_cmd_id {
 	/* Guest Migration Extension */
 	KVM_SEV_SEND_CANCEL,
 
+	/* SNP specific commands */
+	KVM_SEV_SNP_INIT,
+
 	KVM_SEV_NR_MAX,
 };
 
@@ -1808,6 +1811,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)