diff mbox series

[RFC,v7,37/64] KVM: SVM: Add KVM_SNP_INIT command

Message ID 20221214194056.161492-38-michael.roth@amd.com (mailing list archive)
State New
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) Hypervisor Support | expand

Commit Message

Michael Roth Dec. 14, 2022, 7:40 p.m. UTC
From: Brijesh Singh <brijesh.singh@amd.com>

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 <papaluri@amd.com>
Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 .../virt/kvm/x86/amd-memory-encryption.rst    | 27 ++++++++++++
 arch/x86/include/asm/svm.h                    |  1 +
 arch/x86/kvm/svm/sev.c                        | 44 ++++++++++++++++++-
 arch/x86/kvm/svm/svm.h                        |  4 ++
 include/uapi/linux/kvm.h                      | 13 ++++++
 5 files changed, 87 insertions(+), 2 deletions(-)

Comments

Jarkko Sakkinen Dec. 31, 2022, 2:27 p.m. UTC | #1
On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
>  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  {
>  	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>  		return ret;
>  
>  	sev->active = true;
> -	sev->es_active = argp->id == KVM_SEV_ES_INIT;
> +	sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
> +	sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
>  	asid = sev_asid_new(sev);
>  	if (asid < 0)
>  		goto e_no_asid;
>  	sev->asid = asid;
>  
> -	ret = sev_platform_init(&argp->error);
> +	if (sev->snp_active) {
> +		ret = verify_snp_init_flags(kvm, argp);
> +		if (ret)
> +			goto e_free;
> +
> +		ret = sev_snp_init(&argp->error, false);
> +	} else {
> +		ret = sev_platform_init(&argp->error);
> +	}

Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
in order?

Since there is a hardware constraint that SNP init needs to always happen
before platform init, shouldn't SNP init happen as part of
__sev_platform_init_locked() instead?

I found these call sites for __sev_platform_init_locked(), none of which
follow the correct call order:

* sev_guest_init()
* sev_ioctl_do_pek_csr
* sev_ioctl_do_pdh_export()
* sev_ioctl_do_pek_import()
* sev_ioctl_do_pek_pdh_gen()
* sev_pci_init()

For me it looks like a bit flakky API use to have sev_snp_init() as an API
call.

I would suggest to make SNP init internal to the ccp driver and take care
of the correct orchestration over there.

Also, how it currently works in this patch set, if the firmware did not
load correctly, SNP init halts the whole system. The version check needs
to be in all call paths.

BR, Jarkko
From c189db485a4162f401f351d2b1842c7f66f17ae6 Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko@profian.com>
Date: Sun, 4 Dec 2022 06:17:07 +0000
Subject: [PATCH] crypto: ccp: Prevent a spurious SEV_CMD_SNP_INIT triggered by
 sev_guest_init()

Move the firmware version check from sev_pci_init() to sev_snp_init().

Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 drivers/crypto/ccp/sev-dev.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 6c4fdcaed72b..462c9aaa2e7e 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1381,6 +1381,12 @@ static int __sev_snp_init_locked(int *error)
 	if (sev->snp_initialized)
 		return 0;
 
+	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
+		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
+			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
+		return -ENODEV;
+	}
+
 	/*
 	 * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h
 	 * across all cores.
@@ -2313,25 +2319,19 @@ void sev_pci_init(void)
 		}
 	}
 
+	rc = sev_snp_init(&error, true);
+	if (rc != -ENODEV)
+		/*
+		 * Don't abort the probe if SNP INIT failed,
+		 * continue to initialize the legacy SEV firmware.
+		 */
+		dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error);
+
 	/*
 	 * If boot CPU supports SNP, then first attempt to initialize
 	 * the SNP firmware.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) {
-		if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
-			dev_err(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
-				SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
-		} else {
-			rc = sev_snp_init(&error, true);
-			if (rc) {
-				/*
-				 * Don't abort the probe if SNP INIT failed,
-				 * continue to initialize the legacy SEV firmware.
-				 */
-				dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error);
-			}
-		}
-
 		/*
 		 * Allocate the intermediate buffers used for the legacy command handling.
 		 */
Jarkko Sakkinen Dec. 31, 2022, 2:47 p.m. UTC | #2
A couple of fixups.

On Sat, Dec 31, 2022 at 02:27:57PM +0000, Jarkko Sakkinen wrote:
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 6c4fdcaed72b..462c9aaa2e7e 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1381,6 +1381,12 @@ static int __sev_snp_init_locked(int *error)
>  	if (sev->snp_initialized)
>  		return 0;
>  
> +	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
> +		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
> +			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
> +		return -ENODEV;

        return 0;

It is not a failure case anyway.

> +	}
> +
>  	/*
>  	 * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h
>  	 * across all cores.
> @@ -2313,25 +2319,19 @@ void sev_pci_init(void)
>  		}
>  	}
>  
> +	rc = sev_snp_init(&error, true);
> +	if (rc != -ENODEV)


if (rc)

Because other wise there would need to be nasty "if (rc && rc != ENODEV)"
so that this does not happen:

[    9.321588] ccp 0000:49:00.1: SEV-SNP: failed to INIT error 0x0

BR, Jarkko
Jarkko Sakkinen Dec. 31, 2022, 3:16 p.m. UTC | #3
On Sat, Dec 31, 2022 at 02:47:29PM +0000, Jarkko Sakkinen wrote:
> A couple of fixups.
> 
> On Sat, Dec 31, 2022 at 02:27:57PM +0000, Jarkko Sakkinen wrote:
> > diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> > index 6c4fdcaed72b..462c9aaa2e7e 100644
> > --- a/drivers/crypto/ccp/sev-dev.c
> > +++ b/drivers/crypto/ccp/sev-dev.c
> > @@ -1381,6 +1381,12 @@ static int __sev_snp_init_locked(int *error)
> >  	if (sev->snp_initialized)
> >  		return 0;
> >  
> > +	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
> > +		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
> > +			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
> > +		return -ENODEV;
> 
>         return 0;
> 
> It is not a failure case anyway.
> 
> > +	}
> > +
> >  	/*
> >  	 * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h
> >  	 * across all cores.
> > @@ -2313,25 +2319,19 @@ void sev_pci_init(void)
> >  		}
> >  	}
> >  
> > +	rc = sev_snp_init(&error, true);
> > +	if (rc != -ENODEV)
> 
> 
> if (rc)
> 
> Because other wise there would need to be nasty "if (rc && rc != ENODEV)"
> so that this does not happen:
> 
> [    9.321588] ccp 0000:49:00.1: SEV-SNP: failed to INIT error 0x0
> 
> BR, Jarkko

This patch (not dependent on the series) is kind of related to my
feedback. Since platform init can span from quite many locations
it would be useful to get errors reported from all locations:

https://www.lkml.org/lkml/2022/12/31/175

Would be IMHO good to have this in the baseline when testing SNP init
functionality.

BR, Jarkko
Kalra, Ashish Jan. 5, 2023, 11:37 p.m. UTC | #4
Hello Jarkko,

On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote:
> On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
>>   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   {
>>   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   		return ret;
>>   
>>   	sev->active = true;
>> -	sev->es_active = argp->id == KVM_SEV_ES_INIT;
>> +	sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
>> +	sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
>>   	asid = sev_asid_new(sev);
>>   	if (asid < 0)
>>   		goto e_no_asid;
>>   	sev->asid = asid;
>>   
>> -	ret = sev_platform_init(&argp->error);
>> +	if (sev->snp_active) {
>> +		ret = verify_snp_init_flags(kvm, argp);
>> +		if (ret)
>> +			goto e_free;
>> +
>> +		ret = sev_snp_init(&argp->error, false);
>> +	} else {
>> +		ret = sev_platform_init(&argp->error);
>> +	}
> 
> Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
> in order?
> 
> Since there is a hardware constraint that SNP init needs to always happen
> before platform init, shouldn't SNP init happen as part of
> __sev_platform_init_locked() instead?
> 

On Genoa there is currently an issue that if we do an SNP_INIT before an 
SEV_INIT and then attempt to launch a SEV guest that may fail, so we 
need to keep SNP INIT and SEV INIT separate.

We need to provide a way to run (existing) SEV guests on a system that 
supports SNP without doing an SNP_INIT at all.

This is done using psp_init_on_probe parameter of the CCP module to 
avoid doing either SNP/SEV firmware initialization during module load 
and then defer the firmware initialization till someone launches a guest 
of one flavor or the other.

And then sev_guest_init() does either SNP or SEV firmware init depending 
on the type of the guest being launched.

> I found these call sites for __sev_platform_init_locked(), none of which
> follow the correct call order:
> 
> * sev_guest_init()

As explained above, this call site is important for deferring the 
firmware initialization to an actual guest launch.

> * sev_ioctl_do_pek_csr
> * sev_ioctl_do_pdh_export()
> * sev_ioctl_do_pek_import()
> * sev_ioctl_do_pek_pdh_gen()
> * sev_pci_init()
> 
> For me it looks like a bit flakky API use to have sev_snp_init() as an API
> call.
> 
> I would suggest to make SNP init internal to the ccp driver and take care
> of the correct orchestration over there.
>

Due to Genoa issue, we may still need SNP init and SEV init to be 
invoked separately outside the CCP driver.

> Also, how it currently works in this patch set, if the firmware did not
> load correctly, SNP init halts the whole system. The version check needs
> to be in all call paths.
> 

Yes, i agree with that.

Thanks,
Ashish
Jarkko Sakkinen Jan. 20, 2023, 11:17 p.m. UTC | #5
On Thu, Jan 05, 2023 at 05:37:20PM -0600, Kalra, Ashish wrote:
> Hello Jarkko,
> 
> On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote:
> > On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
> > >   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >   {
> > >   	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> > > @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
> > >   		return ret;
> > >   	sev->active = true;
> > > -	sev->es_active = argp->id == KVM_SEV_ES_INIT;
> > > +	sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
> > > +	sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
> > >   	asid = sev_asid_new(sev);
> > >   	if (asid < 0)
> > >   		goto e_no_asid;
> > >   	sev->asid = asid;
> > > -	ret = sev_platform_init(&argp->error);
> > > +	if (sev->snp_active) {
> > > +		ret = verify_snp_init_flags(kvm, argp);
> > > +		if (ret)
> > > +			goto e_free;
> > > +
> > > +		ret = sev_snp_init(&argp->error, false);
> > > +	} else {
> > > +		ret = sev_platform_init(&argp->error);
> > > +	}
> > 
> > Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
> > in order?
> > 
> > Since there is a hardware constraint that SNP init needs to always happen
> > before platform init, shouldn't SNP init happen as part of
> > __sev_platform_init_locked() instead?
> > 
> 
> On Genoa there is currently an issue that if we do an SNP_INIT before an
> SEV_INIT and then attempt to launch a SEV guest that may fail, so we need to
> keep SNP INIT and SEV INIT separate.
> 
> We need to provide a way to run (existing) SEV guests on a system that
> supports SNP without doing an SNP_INIT at all.
> 
> This is done using psp_init_on_probe parameter of the CCP module to avoid
> doing either SNP/SEV firmware initialization during module load and then
> defer the firmware initialization till someone launches a guest of one
> flavor or the other.
> 
> And then sev_guest_init() does either SNP or SEV firmware init depending on
> the type of the guest being launched.

OK, got it, thank you. I have not noticed the init_on_probe for
sev_snp_init() before. Was it in earlier patch set version?

The benefit of having everything in __sev_platform_init_lock() would be first 
less risk of shooting yourself into foot, and also no need to pass
init_on_probe to sev_snp_init() as it would be internal to sev-dev.c, and
no need for special cases for callers. It is in my opinion internals of the
SEV driver to guarantee the order.

E.g. changes to svm/sev.c would be then quite trivial.

> > I found these call sites for __sev_platform_init_locked(), none of which
> > follow the correct call order:
> > 
> > * sev_guest_init()
> 
> As explained above, this call site is important for deferring the firmware
> initialization to an actual guest launch.
> 
> > * sev_ioctl_do_pek_csr
> > * sev_ioctl_do_pdh_export()
> > * sev_ioctl_do_pek_import()
> > * sev_ioctl_do_pek_pdh_gen()

What happens if any of these are called before sev_guest_init()? They only
call __sev_platform_init_locked().

> > * sev_pci_init()
> > 
> > For me it looks like a bit flakky API use to have sev_snp_init() as an API
> > call.
> > 
> > I would suggest to make SNP init internal to the ccp driver and take care
> > of the correct orchestration over there.
> > 
> 
> Due to Genoa issue, we may still need SNP init and SEV init to be invoked
> separately outside the CCP driver.
> 
> > Also, how it currently works in this patch set, if the firmware did not
> > load correctly, SNP init halts the whole system. The version check needs
> > to be in all call paths.
> > 
> 
> Yes, i agree with that.

Attached the fix I sent in private earlier.

> Thanks,
> Ashish

BR, Jarkko
From f24054af9eeeb0314bbd0c37bd97ff38e2ded717 Mon Sep 17 00:00:00 2001
From: Jarkko Sakkinen <jarkko@profian.com>
Date: Sun, 4 Dec 2022 06:17:07 +0000
Subject: [PATCH] crypto: ccp: Prevent a spurious SEV_CMD_SNP_INIT triggered by
 sev_guest_init()

Move the firmware version check from sev_pci_init() to sev_snp_init().

Signed-off-by: Jarkko Sakkinen <jarkko@profian.com>
---
 drivers/crypto/ccp/sev-dev.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 7c5698bde655..08787d998f15 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1387,6 +1387,12 @@ static int __sev_snp_init_locked(int *error)
 	if (sev->snp_initialized)
 		return 0;
 
+	if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
+		dev_dbg(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
+			SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
+		return 0;
+	}
+
 	/*
 	 * The SNP_INIT requires the MSR_VM_HSAVE_PA must be set to 0h
 	 * across all cores.
@@ -2319,25 +2325,19 @@ void sev_pci_init(void)
 		}
 	}
 
+	rc = sev_snp_init(&error, true);
+	if (rc)
+		/*
+		 * Don't abort the probe if SNP INIT failed,
+		 * continue to initialize the legacy SEV firmware.
+		 */
+		dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error);
+
 	/*
 	 * If boot CPU supports SNP, then first attempt to initialize
 	 * the SNP firmware.
 	 */
 	if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) {
-		if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR)) {
-			dev_err(sev->dev, "SEV-SNP support requires firmware version >= %d:%d\n",
-				SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
-		} else {
-			rc = sev_snp_init(&error, true);
-			if (rc) {
-				/*
-				 * Don't abort the probe if SNP INIT failed,
-				 * continue to initialize the legacy SEV firmware.
-				 */
-				dev_err(sev->dev, "SEV-SNP: failed to INIT error %#x\n", error);
-			}
-		}
-
 		/*
 		 * Allocate the intermediate buffers used for the legacy command handling.
 		 */
Kalra, Ashish Jan. 23, 2023, 10:49 p.m. UTC | #6
There was an early firmware issue on Genoa which supported only SNP_INIT 
or SEV_INIT, but this issue is resolved now.

Now, the main constraints are that SNP_INIT is always required before 
SEV_INIT in case we want to launch SNP guests. In other words, if only 
SEV_INIT is done on a platform which supports SNP we won't be able to 
launch SNP guests after that.

So once we have RMP table setup (in BIOS) we will always do an SNP_INIT 
and SEV_INIT will be ideally done only (on demand) when an SEV guest is 
launched.

Thanks,
Ashish

On 1/5/2023 5:37 PM, Kalra, Ashish wrote:
> Hello Jarkko,
> 
> On 12/31/2022 8:27 AM, Jarkko Sakkinen wrote:
>> On Wed, Dec 14, 2022 at 01:40:29PM -0600, Michael Roth wrote:
>>>   static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>>   {
>>>       struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
>>> @@ -260,13 +279,23 @@ static int sev_guest_init(struct kvm *kvm, 
>>> struct kvm_sev_cmd *argp)
>>>           return ret;
>>>       sev->active = true;
>>> -    sev->es_active = argp->id == KVM_SEV_ES_INIT;
>>> +    sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == 
>>> KVM_SEV_SNP_INIT);
>>> +    sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
>>>       asid = sev_asid_new(sev);
>>>       if (asid < 0)
>>>           goto e_no_asid;
>>>       sev->asid = asid;
>>> -    ret = sev_platform_init(&argp->error);
>>> +    if (sev->snp_active) {
>>> +        ret = verify_snp_init_flags(kvm, argp);
>>> +        if (ret)
>>> +            goto e_free;
>>> +
>>> +        ret = sev_snp_init(&argp->error, false);
>>> +    } else {
>>> +        ret = sev_platform_init(&argp->error);
>>> +    }
>>
>> Couldn't sev_snp_init() and sev_platform_init() be called unconditionally
>> in order?
>>
>> Since there is a hardware constraint that SNP init needs to always happen
>> before platform init, shouldn't SNP init happen as part of
>> __sev_platform_init_locked() instead?
>>
> 
> On Genoa there is currently an issue that if we do an SNP_INIT before an 
> SEV_INIT and then attempt to launch a SEV guest that may fail, so we 
> need to keep SNP INIT and SEV INIT separate.
> 
> We need to provide a way to run (existing) SEV guests on a system that 
> supports SNP without doing an SNP_INIT at all.
> 
> This is done using psp_init_on_probe parameter of the CCP module to 
> avoid doing either SNP/SEV firmware initialization during module load 
> and then defer the firmware initialization till someone launches a guest 
> of one flavor or the other.
> 
> And then sev_guest_init() does either SNP or SEV firmware init depending 
> on the type of the guest being launched.
> 
>> I found these call sites for __sev_platform_init_locked(), none of which
>> follow the correct call order:
>>
>> * sev_guest_init()
> 
> As explained above, this call site is important for deferring the 
> firmware initialization to an actual guest launch.
> 
>> * sev_ioctl_do_pek_csr
>> * sev_ioctl_do_pdh_export()
>> * sev_ioctl_do_pek_import()
>> * sev_ioctl_do_pek_pdh_gen()
>> * sev_pci_init()
>>
>> For me it looks like a bit flakky API use to have sev_snp_init() as an 
>> API
>> call.
>>
>> I would suggest to make SNP init internal to the ccp driver and take care
>> of the correct orchestration over there.
>>
> 
> Due to Genoa issue, we may still need SNP init and SEV init to be 
> invoked separately outside the CCP driver.
> 
>> Also, how it currently works in this patch set, if the firmware did not
>> load correctly, SNP init halts the whole system. The version check needs
>> to be in all call paths.
>>
> 
> Yes, i agree with that.
> 
> Thanks,
> Ashish
Jarkko Sakkinen Jan. 26, 2023, 9:25 p.m. UTC | #7
On Mon, Jan 23, 2023 at 04:49:14PM -0600, Kalra, Ashish wrote:
> There was an early firmware issue on Genoa which supported only SNP_INIT or
> SEV_INIT, but this issue is resolved now.
> 
> Now, the main constraints are that SNP_INIT is always required before
> SEV_INIT in case we want to launch SNP guests. In other words, if only
> SEV_INIT is done on a platform which supports SNP we won't be able to launch
> SNP guests after that.
> 
> So once we have RMP table setup (in BIOS) we will always do an SNP_INIT and
> SEV_INIT will be ideally done only (on demand) when an SEV guest is
> launched.

OK, thanks for the clarification!

BR, Jarkko
diff mbox series

Patch

diff --git a/Documentation/virt/kvm/x86/amd-memory-encryption.rst b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
index 935aaeb97fe6..2432213bd0ea 100644
--- a/Documentation/virt/kvm/x86/amd-memory-encryption.rst
+++ b/Documentation/virt/kvm/x86/amd-memory-encryption.rst
@@ -434,6 +434,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 cb1ee53ad3b1..c18d78d5e505 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -278,6 +278,7 @@  enum avic_ipi_failure_cause {
 #define AVIC_HPA_MASK	~((0xFFFULL << 52) | 0xFFF)
 #define VMCB_AVIC_APIC_BAR_MASK		0xFFFFFFFFFF000ULL
 
+#define SVM_SEV_FEAT_SNP_ACTIVE		BIT(0)
 
 struct vmcb_seg {
 	u16 selector;
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f34da1203e09..e3f857cde8c0 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -247,6 +247,25 @@  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)
 {
 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
@@ -260,13 +279,23 @@  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 		return ret;
 
 	sev->active = true;
-	sev->es_active = argp->id == KVM_SEV_ES_INIT;
+	sev->es_active = (argp->id == KVM_SEV_ES_INIT || argp->id == KVM_SEV_SNP_INIT);
+	sev->snp_active = argp->id == KVM_SEV_SNP_INIT;
 	asid = sev_asid_new(sev);
 	if (asid < 0)
 		goto e_no_asid;
 	sev->asid = asid;
 
-	ret = sev_platform_init(&argp->error);
+	if (sev->snp_active) {
+		ret = verify_snp_init_flags(kvm, argp);
+		if (ret)
+			goto e_free;
+
+		ret = sev_snp_init(&argp->error, false);
+	} else {
+		ret = sev_platform_init(&argp->error);
+	}
+
 	if (ret)
 		goto e_free;
 
@@ -281,6 +310,7 @@  static int sev_guest_init(struct kvm *kvm, struct kvm_sev_cmd *argp)
 	sev_asid_free(sev);
 	sev->asid = 0;
 e_no_asid:
+	sev->snp_active = false;
 	sev->es_active = false;
 	sev->active = false;
 	return ret;
@@ -741,6 +771,10 @@  static int sev_es_sync_vmsa(struct vcpu_svm *svm)
 	save->xss  = svm->vcpu.arch.ia32_xss;
 	save->dr6  = svm->vcpu.arch.dr6;
 
+	/* Enable the SEV-SNP feature */
+	if (sev_snp_guest(svm->vcpu.kvm))
+		save->sev_features |= SVM_SEV_FEAT_SNP_ACTIVE;
+
 	pr_debug("Virtual Machine Save Area (VMSA):\n");
 	print_hex_dump_debug("", DUMP_PREFIX_NONE, 16, 1, save, sizeof(*save), false);
 
@@ -1993,6 +2027,12 @@  int sev_mem_enc_ioctl(struct kvm *kvm, void __user *argp)
 	}
 
 	switch (sev_cmd.id) {
+	case KVM_SEV_SNP_INIT:
+		if (!sev_snp_enabled) {
+			r = -ENOTTY;
+			goto out;
+		}
+		fallthrough;
 	case KVM_SEV_ES_INIT:
 		if (!sev_es_enabled) {
 			r = -ENOTTY;
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index a48fe5d2bea5..379b253d2464 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -80,6 +80,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 */
@@ -95,6 +98,7 @@  struct kvm_sev_info {
 	struct list_head mirror_entry; /* Use as a list entry of mirrors */
 	struct misc_cg *misc_cg; /* For misc cgroup accounting */
 	atomic_t migration_in_progress;
+	u64 snp_init_flags;
 };
 
 struct kvm_svm {
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index cc9424ccf9b2..a6c73297a62d 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1938,6 +1938,9 @@  enum sev_cmd_id {
 	/* Guest Migration Extension */
 	KVM_SEV_SEND_CANCEL,
 
+	/* SNP specific commands */
+	KVM_SEV_SNP_INIT,
+
 	KVM_SEV_NR_MAX,
 };
 
@@ -2034,6 +2037,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)