mbox series

[v7,00/10] Basic SEV-SNP Selftests

Message ID 20250221210200.244405-1-prsampat@amd.com
Headers show
Series Basic SEV-SNP Selftests | expand

Message

Pratik R. Sampat Feb. 21, 2025, 9:01 p.m. UTC 
This patch series extends the sev_init2 and the sev_smoke test to
exercise the SEV-SNP VM launch workflow.

Primarily, it introduces the architectural defines, its support in the
SEV library and extends the tests to interact with the SEV-SNP ioctl()
wrappers.

Patch 1  - Do not advertize SNP on initialization failure
Patch 2  - SNP test for KVM_SEV_INIT2
Patch 3  - Add vmgexit helper
Patch 4  - Add SMT control interface helper
Patch 5  - Replace assert() with TEST_ASSERT_EQ()
Patch 6  - Introduce SEV+ VM type check
Patch 7  - SNP iotcl() plumbing for the SEV library
Patch 8  - Force set GUEST_MEMFD for SNP
Patch 9  - Cleanups of smoke test - Decouple policy from type
Patch 10 - SNP smoke test

The series is based on
	git.kernel.org/pub/scm/virt/kvm/kvm.git next

v6..v7:
Based on comments from Sean -
* Replaced FW check with sev->snp_initialized
* Dropped the patch which removes SEV+ KVM advertizement if INIT fails.
  This should be now be resolved by the combination of the patches [1,2]
  from Ashish.
* Change vmgexit to an inline function
* Export SMT control parsing interface to kvm_util
  Note: hyperv_cpuid KST only compile testeworkbench.editor.empty.hintd
* Replace assert() with TEST_ASSERT_EQ() within SEV library
* Define KVM_SEV_PAGE_TYPE_INVALID for SEV call of encrypt_region()
* Parameterize encrypt_region() to include privatize_region()
* Deduplication of sev test calls between SEV,SEV-ES and SNP
* Removed FW version tests for SNP
* Included testing of SNP_POLICY_DBG
* Dropped most tags from patches that have been changed or indirectly
  affected

[1] https://lore.kernel.org/all/d6d08c6b-9602-4f3d-92c2-8db6d50a1b92@amd.com
[2] https://lore.kernel.org/all/f78ddb64087df27e7bcb1ae0ab53f55aa0804fab.1739226950.git.ashish.kalra@amd.com

v5..v6:
https://lore.kernel.org/kvm/ab433246-e97c-495b-ab67-b0cb1721fb99@amd.com/
* Rename is_sev_platform_init to sev_fw_initialized (Nikunj)
* Rename KVM CPU feature X86_FEATURE_SNP to X86_FEATURE_SEV_SNP (Nikunj)
* Collected Tags from Nikunj, Pankaj, Srikanth.

v4..v5:
https://lore.kernel.org/kvm/8e7d8172-879e-4a28-8438-343b1c386ec9@amd.com/
* Introduced a check to disable advertising support for SEV, SEV-ES
  and SNP when platform initialization fails (Nikunj)
* Remove the redundant SNP check within is_sev_vm() (Nikunj)
* Cleanup of the encrypt_region flow for better readability (Nikunj)
* Refactor paths to use the canonical $(ARCH) to rebase for kvm/next

v3..v4:
https://lore.kernel.org/kvm/20241114234104.128532-1-pratikrajesh.sampat@amd.com/
* Remove SNP FW API version check in the test and ensure the KVM
  capability advertizes the presence of the feature. Retain the minimum
  version definitions to exercise these API versions in the smoke test
* Retained only the SNP smoke test and SNP_INIT2 test
* The SNP architectural defined merged with SNP_INIT2 test patch
* SNP shutdown merged with SNP smoke test patch
* Add SEV VM type check to abstract comparisons and reduce clutter
* Define a SNP default policy which sets bits based on the presence of
  SMT
* Decouple privatization and encryption for it to be SNP agnostic
* Assert for only positive tests using vm_ioctl()
* Dropped tested-by tags

In summary - based on comments from Sean, I have primarily reduced the
scope of this patch series to focus on breaking down the SNP smoke test
patch (v3 - patch2) to first introduce SEV-SNP support and use this
interface to extend the sev_init2 and the sev_smoke test.

The rest of the v3 patchset that introduces ioctl, pre fault, fallocate
and negative tests, will be re-worked and re-introduced subsequently in
future patch series post addressing the issues discussed.

v2..v3:
https://lore.kernel.org/kvm/20240905124107.6954-1-pratikrajesh.sampat@amd.com/
* Remove the assignments for the prefault and fallocate test type
  enums.
* Fix error message for sev launch measure and finish.
* Collect tested-by tags [Peter, Srikanth]](<This patch series extends the sev_init2 and the sev_smoke test to
exercise the SEV-SNP VM launch workflow.

Primarily, it introduces the architectural defines, its support in the SEV
library and extends the tests to interact with the SEV-SNP ioctl()
wrappers.

Patch 1  - Do not advertize SNP on initialization failure
Patch 2  - SNP test for KVM_SEV_INIT2
Patch 3  - Add vmgexit helper
Patch 4  - Helper for SMT control interface
Patch 5  - Replace assert() with TEST_ASSERT_EQ()
Patch 6  - Introduce SEV+ VM type check
Patch 7  - SNP iotcl() plumbing for the SEV library
Patch 8  - Force set GUEST_MEMFD for SNP
Patch 9  - Cleanups of smoke test - Decouple policy from type
Patch 10 - SNP smoke test

The series is based on
	git.kernel.org/pub/scm/virt/kvm/kvm.git next

v6..v7
Based on comments from Sean -
* Replaced FW check with sev-%3Esnp_initialized
* Dropped the patch which removes SEV+ KVM advertizement if INIT fails
  This should be resolved by the combination of [1][2] from Ashish:
* Change vmgexit to an inline function
* Export SMT control parsing interface to kvm_util
* Replace assert() with TEST_ASSERT_EQ() within SEV library
* Define KVM_SEV_PAGE_TYPE_INVALID for SEV to use it with
  encrypt_region()
* Parameterize encrypt_region() to include privatize_region()
  functionality
* Deduplication of sev test calls between SEV,SEV-ES and SNP
* Removed FW version tests for SNP
* Included testing of SNP_POLICY_DBG
* Dropped most tags from patches that have directly / indirectly
  changed.

[1] https://lore.kernel.org/all/d6d08c6b-9602-4f3d-92c2-8db6d50a1b92@amd.com
[2] https://lore.kernel.org/all/f78ddb64087df27e7bcb1ae0ab53f55aa0804fab.1739226950.git.ashish.kalra@amd.com

v5..v6
https://lore.kernel.org/kvm/ab433246-e97c-495b-ab67-b0cb1721fb99@amd.com/
* Rename is_sev_platform_init to sev_fw_initialized (Nikunj)
* Rename KVM CPU feature X86_FEATURE_SNP to X86_FEATURE_SEV_SNP (Nikunj)
* Collected Tags from Nikunj, Pankaj, Srikanth.

v4..v5:
https://lore.kernel.org/kvm/8e7d8172-879e-4a28-8438-343b1c386ec9@amd.com/
* Introduced a check to disable advertising support for SEV, SEV-ES
  and SNP when platform initialization fails (Nikunj)
* Remove the redundant SNP check within is_sev_vm() (Nikunj)
* Cleanup of the encrypt_region flow for better readability (Nikunj)
* Refactor paths to use the canonical $(ARCH) to rebase for kvm/next

v3..v4:
https://lore.kernel.org/kvm/20241114234104.128532-1-pratikrajesh.sampat@amd.com/
* Remove SNP FW API version check in the test and ensure the KVM
  capability advertizes the presence of the feature. Retain the minimum
  version definitions to exercise these API versions in the smoke test
* Retained only the SNP smoke test and SNP_INIT2 test
* The SNP architectural defined merged with SNP_INIT2 test patch
* SNP shutdown merged with SNP smoke test patch
* Add SEV VM type check to abstract comparisons and reduce clutter
* Define a SNP default policy which sets bits based on the presence of
  SMT
* Decouple privatization and encryption for it to be SNP agnostic
* Assert for only positive tests using vm_ioctl()
* Dropped tested-by tags

In summary - based on comments from Sean, I have primarily reduced the
scope of this patch series to focus on breaking down the SNP smoke test
patch (v3 - patch2) to first introduce SEV-SNP support and use this
interface to extend the sev_init2 and the sev_smoke test.

The rest of the v3 patchset that introduces ioctl, pre fault, fallocate
and negative tests, will be re-worked and re-introduced subsequently in
future patch series post addressing the issues discussed.

v2..v3:
https://lore.kernel.org/kvm/20240905124107.6954-1-pratikrajesh.sampat@amd.com/
* Remove the assignments for the prefault and fallocate test type
  enums.
* Fix error message for sev launch measure and finish.
* Collect tested-by tags [Peter, Srikanth]

Pratik R. Sampat (10):
  KVM: SEV: Disable SEV-SNP support on initialization failure
  KVM: selftests: SEV-SNP test for KVM_SEV_INIT2
  KVM: selftests: Add vmgexit helper
  KVM: selftests: Add SMT control state helper
  KVM: selftests: Replace assert() with TEST_ASSERT_EQ()
  KVM: selftests: Introduce SEV VM type check
  KVM: selftests: Add library support for interacting with SNP
  KVM: selftests: Force GUEST_MEMFD flag for SNP VM type
  KVM: selftests: Abstractions for SEV to decouple policy from type
  KVM: selftests: Add a basic SEV-SNP smoke test

 arch/x86/include/uapi/asm/kvm.h               |  1 +
 arch/x86/kvm/svm/sev.c                        |  4 +-
 drivers/crypto/ccp/sev-dev.c                  |  8 ++
 include/linux/psp-sev.h                       |  3 +
 tools/arch/x86/include/uapi/asm/kvm.h         |  1 +
 .../testing/selftests/kvm/include/kvm_util.h  | 35 +++++++
 .../selftests/kvm/include/x86/processor.h     |  1 +
 tools/testing/selftests/kvm/include/x86/sev.h | 42 ++++++++-
 tools/testing/selftests/kvm/lib/kvm_util.c    |  7 +-
 .../testing/selftests/kvm/lib/x86/processor.c |  4 +-
 tools/testing/selftests/kvm/lib/x86/sev.c     | 93 +++++++++++++++++--
 .../testing/selftests/kvm/x86/hyperv_cpuid.c  | 19 ----
 .../selftests/kvm/x86/sev_init2_tests.c       | 13 +++
 .../selftests/kvm/x86/sev_smoke_test.c        | 75 +++++++++------
 14 files changed, 246 insertions(+), 60 deletions(-)

Comments

Liam Merwick Feb. 24, 2025, 7:01 p.m. UTC | #1 
On 21/02/2025 21:01, Pratik R. Sampat wrote:
> During platform init, SNP initialization may fail for several reasons,
> such as firmware command failures and incompatible versions. However,
> the KVM capability may continue to advertise support for it. Export this
> information to KVM and withdraw SEV-SNP support if has not been
> successfully initialized.
> 
> Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support")
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
> ---
> v6..v7:
> 
> * Replace FW version check with sev->snp_initialized (Sean)
> ---
>   arch/x86/kvm/svm/sev.c       | 4 +++-
>   drivers/crypto/ccp/sev-dev.c | 8 ++++++++
>   include/linux/psp-sev.h      | 3 +++
>   3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0dbb25442ec1..87b5d63a5817 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void)
>   	sev_es_asid_count = min_sev_asid - 1;
>   	WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>   	sev_es_supported = true;
> -	sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> +	sev_snp_supported = (sev_snp_enabled &&
> +			    cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
> +			    snp_initialized());
>   
>   out:
>   	if (boot_cpu_has(X86_FEATURE_SEV))
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2e87ca0e292a..8d2cf8552bc2 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1352,6 +1352,14 @@ int sev_platform_init(struct sev_platform_init_args *args)
>   }
>   EXPORT_SYMBOL_GPL(sev_platform_init);
>   
> +bool snp_initialized(void)
> +{
> +	struct sev_device *sev = psp_master->sev_data;


Should check psp_master isn't NULL before accessing just in case
(particularly for future potential callers).

(e.g. see ccb88e9549e7 ("crypto: ccp - Fix null pointer dereference in 
__sev_platform_shutdown_locked")




> +
> +	return sev->snp_initialized;
> +}
> +EXPORT_SYMBOL_GPL(snp_initialized);
> +
>   static int __sev_platform_shutdown_locked(int *error)
>   {
>   	struct psp_device *psp = psp_master;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index f3cad182d4ef..d34068c87a28 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret);
>   void *psp_copy_user_blob(u64 uaddr, u32 len);
>   void *snp_alloc_firmware_page(gfp_t mask);
>   void snp_free_firmware_page(void *addr);
> +bool snp_initialized(void);
>   
>   #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>   
> @@ -988,6 +989,8 @@ static inline void *snp_alloc_firmware_page(gfp_t mask)
>   
>   static inline void snp_free_firmware_page(void *addr) { }
>   
> +static inline bool snp_initialized(void) { return false; }
> +
>   #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
>   
>   #endif	/* __PSP_SEV_H__ */
Tom Lendacky Feb. 24, 2025, 9:28 p.m. UTC | #2 
On 2/21/25 15:01, Pratik R. Sampat wrote:
> During platform init, SNP initialization may fail for several reasons,
> such as firmware command failures and incompatible versions. However,
> the KVM capability may continue to advertise support for it. Export this
> information to KVM and withdraw SEV-SNP support if has not been
> successfully initialized.

Hmmm... rather than creating a new API, can you just issue an
SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state?

Thanks,
Tom

> 
> Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support")
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
> ---
> v6..v7:
> 
> * Replace FW version check with sev->snp_initialized (Sean)
> ---
>  arch/x86/kvm/svm/sev.c       | 4 +++-
>  drivers/crypto/ccp/sev-dev.c | 8 ++++++++
>  include/linux/psp-sev.h      | 3 +++
>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 0dbb25442ec1..87b5d63a5817 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void)
>  	sev_es_asid_count = min_sev_asid - 1;
>  	WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES, sev_es_asid_count));
>  	sev_es_supported = true;
> -	sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> +	sev_snp_supported = (sev_snp_enabled &&
> +			    cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
> +			    snp_initialized());
>  
>  out:
>  	if (boot_cpu_has(X86_FEATURE_SEV))
> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
> index 2e87ca0e292a..8d2cf8552bc2 100644
> --- a/drivers/crypto/ccp/sev-dev.c
> +++ b/drivers/crypto/ccp/sev-dev.c
> @@ -1352,6 +1352,14 @@ int sev_platform_init(struct sev_platform_init_args *args)
>  }
>  EXPORT_SYMBOL_GPL(sev_platform_init);
>  
> +bool snp_initialized(void)
> +{
> +	struct sev_device *sev = psp_master->sev_data;
> +
> +	return sev->snp_initialized;
> +}
> +EXPORT_SYMBOL_GPL(snp_initialized);
> +
>  static int __sev_platform_shutdown_locked(int *error)
>  {
>  	struct psp_device *psp = psp_master;
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index f3cad182d4ef..d34068c87a28 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret);
>  void *psp_copy_user_blob(u64 uaddr, u32 len);
>  void *snp_alloc_firmware_page(gfp_t mask);
>  void snp_free_firmware_page(void *addr);
> +bool snp_initialized(void);
>  
>  #else	/* !CONFIG_CRYPTO_DEV_SP_PSP */
>  
> @@ -988,6 +989,8 @@ static inline void *snp_alloc_firmware_page(gfp_t mask)
>  
>  static inline void snp_free_firmware_page(void *addr) { }
>  
> +static inline bool snp_initialized(void) { return false; }
> +
>  #endif	/* CONFIG_CRYPTO_DEV_SP_PSP */
>  
>  #endif	/* __PSP_SEV_H__ */
Pratik R. Sampat Feb. 25, 2025, 4:41 p.m. UTC | #3 
Hi Tom,

On 2/24/2025 3:28 PM, Tom Lendacky wrote:
> On 2/21/25 15:01, Pratik R. Sampat wrote:
>> During platform init, SNP initialization may fail for several reasons,
>> such as firmware command failures and incompatible versions. However,
>> the KVM capability may continue to advertise support for it. Export this
>> information to KVM and withdraw SEV-SNP support if has not been
>> successfully initialized.
> 
> Hmmm... rather than creating a new API, can you just issue an
> SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state?
> 

Although reading sev->snp_initialized is probably cheaper to do, it is
cleaner to query the platform status.

Querying SNP_PLATFORM_STATUS requires the pages to transition to
firmware-owned and back, and the helpers for it are implemented within
sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is
probably better to create the snp_platform_status() API as well and use
that within KVM to check the state.

Thanks!
Pratik
Pratik R. Sampat Feb. 25, 2025, 4:50 p.m. UTC | #4 
Hi Liam,

Thanks for review!

On 2/24/2025 1:01 PM, Liam Merwick wrote:
> 
> 
> On 21/02/2025 21:01, Pratik R. Sampat wrote:
>> During platform init, SNP initialization may fail for several reasons,
>> such as firmware command failures and incompatible versions. However,
>> the KVM capability may continue to advertise support for it. Export this
>> information to KVM and withdraw SEV-SNP support if has not been
>> successfully initialized.
>>
>> Fixes: 1dfe571c12cf ("KVM: SEV: Add initial SEV-SNP support")
>> Suggested-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
>> ---
>> v6..v7:
>>
>> * Replace FW version check with sev->snp_initialized (Sean)
>> ---
>>   arch/x86/kvm/svm/sev.c       | 4 +++-
>>   drivers/crypto/ccp/sev-dev.c | 8 ++++++++
>>   include/linux/psp-sev.h      | 3 +++
>>   3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 0dbb25442ec1..87b5d63a5817 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -3050,7 +3050,9 @@ void __init sev_hardware_setup(void)
>>       sev_es_asid_count = min_sev_asid - 1;
>>       WARN_ON_ONCE(misc_cg_set_capacity(MISC_CG_RES_SEV_ES,
>> sev_es_asid_count));
>>       sev_es_supported = true;
>> -    sev_snp_supported = sev_snp_enabled &&
>> cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>> +    sev_snp_supported = (sev_snp_enabled &&
>> +                cc_platform_has(CC_ATTR_HOST_SEV_SNP) &&
>> +                snp_initialized());
>>     out:
>>       if (boot_cpu_has(X86_FEATURE_SEV))
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index 2e87ca0e292a..8d2cf8552bc2 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -1352,6 +1352,14 @@ int sev_platform_init(struct
>> sev_platform_init_args *args)
>>   }
>>   EXPORT_SYMBOL_GPL(sev_platform_init);
>>   +bool snp_initialized(void)
>> +{
>> +    struct sev_device *sev = psp_master->sev_data;
> 
> 
> Should check psp_master isn't NULL before accessing just in case
> (particularly for future potential callers).
> 
> (e.g. see ccb88e9549e7 ("crypto: ccp - Fix null pointer dereference in
> __sev_platform_shutdown_locked")
> 

Thanks for pointing this out, if I end up using this interface, I'll put
the NULL check in.

Thanks!
Pratik
> 
> 
> 
>> +
>> +    return sev->snp_initialized;
>> +}
>> +EXPORT_SYMBOL_GPL(snp_initialized);
>> +
>>   static int __sev_platform_shutdown_locked(int *error)
>>   {
>>       struct psp_device *psp = psp_master;
>> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
>> index f3cad182d4ef..d34068c87a28 100644
>> --- a/include/linux/psp-sev.h
>> +++ b/include/linux/psp-sev.h
>> @@ -954,6 +954,7 @@ int sev_do_cmd(int cmd, void *data, int *psp_ret);
>>   void *psp_copy_user_blob(u64 uaddr, u32 len);
>>   void *snp_alloc_firmware_page(gfp_t mask);
>>   void snp_free_firmware_page(void *addr);
>> +bool snp_initialized(void);
>>     #else    /* !CONFIG_CRYPTO_DEV_SP_PSP */
>>   @@ -988,6 +989,8 @@ static inline void
>> *snp_alloc_firmware_page(gfp_t mask)
>>     static inline void snp_free_firmware_page(void *addr) { }
>>   +static inline bool snp_initialized(void) { return false; }
>> +
>>   #endif    /* CONFIG_CRYPTO_DEV_SP_PSP */
>>     #endif    /* __PSP_SEV_H__ */
>
Pratik R. Sampat Feb. 25, 2025, 5:45 p.m. UTC | #5 
On 2/25/2025 10:41 AM, Pratik R. Sampat wrote:
> Hi Tom,
> 
> On 2/24/2025 3:28 PM, Tom Lendacky wrote:
>> On 2/21/25 15:01, Pratik R. Sampat wrote:
>>> During platform init, SNP initialization may fail for several reasons,
>>> such as firmware command failures and incompatible versions. However,
>>> the KVM capability may continue to advertise support for it. Export this
>>> information to KVM and withdraw SEV-SNP support if has not been
>>> successfully initialized.
>>
>> Hmmm... rather than creating a new API, can you just issue an
>> SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state?
>>
> 
> Although reading sev->snp_initialized is probably cheaper to do, it is
> cleaner to query the platform status.
> 
> Querying SNP_PLATFORM_STATUS requires the pages to transition to
> firmware-owned and back, and the helpers for it are implemented within
> sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is
> probably better to create the snp_platform_status() API as well and use
> that within KVM to check the state.
> 

Although I am guessing the initial intent was to not have an API exposed
at all from CCP and only make the SNP_PLATFORM_STATUS call instead?

Since that may not be cleanly possible (we have helpers for page state
conversions such as rmp_mark_pages_firmware() in ccp) without
duplicating functionality in KVM as well, I guess the question really
boils down to whether we export the cheaper snp_initialized() or the
snp_platform_status() API instead?

Thanks again!
Pratik
Pratik R. Sampat Feb. 25, 2025, 7:45 p.m. UTC | #6 
On 2/25/2025 1:09 PM, Tom Lendacky wrote:
> On 2/25/25 11:45, Pratik R. Sampat wrote:
>> On 2/25/2025 10:41 AM, Pratik R. Sampat wrote:
>>> Hi Tom,
>>>
>>> On 2/24/2025 3:28 PM, Tom Lendacky wrote:
>>>> On 2/21/25 15:01, Pratik R. Sampat wrote:
>>>>> During platform init, SNP initialization may fail for several reasons,
>>>>> such as firmware command failures and incompatible versions. However,
>>>>> the KVM capability may continue to advertise support for it. Export this
>>>>> information to KVM and withdraw SEV-SNP support if has not been
>>>>> successfully initialized.
>>>>
>>>> Hmmm... rather than creating a new API, can you just issue an
>>>> SNP_PLATFORM_STATUS command and see if the SNP is not in the UNINIT state?
>>>>
>>>
>>> Although reading sev->snp_initialized is probably cheaper to do, it is
>>> cleaner to query the platform status.
>>>
>>> Querying SNP_PLATFORM_STATUS requires the pages to transition to
>>> firmware-owned and back, and the helpers for it are implemented within
>>> sev-dev.c. So, similar to sev_platform_status(), I'm thinking it is
>>> probably better to create the snp_platform_status() API as well and use
>>> that within KVM to check the state.
>>>
>>
>> Although I am guessing the initial intent was to not have an API exposed
>> at all from CCP and only make the SNP_PLATFORM_STATUS call instead?
>>
>> Since that may not be cleanly possible (we have helpers for page state
>> conversions such as rmp_mark_pages_firmware() in ccp) without
>> duplicating functionality in KVM as well, I guess the question really
>> boils down to whether we export the cheaper snp_initialized() or the
>> snp_platform_status() API instead?
> 
> Taking a closer look, we do already have APIs that KVM uses to allocate
> firmware pages (output pages for SNP APIs) that can be used:
> snp_alloc_firmware_page() and snp_free_firmware_page().
> 
> I think that should be enough to use sev_do_cmd() to perform the
> SEV_CMD_SNP_PLATFORM_STATUS command without exposing a new API.
> 

Ah, I had missed that! Calling the SNP_PLATFORM_STATUS this way works.

Pratik

> Thanks,
> Tom
> 
>>
>> Thanks again!
>> Pratik