arch/x86/include/uapi/asm/kvm.h | 1 + arch/x86/kvm/svm/sev.c | 30 +++++- 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 +++++++++------ 12 files changed, 261 insertions(+), 60 deletions(-)
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 advertise 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
v7..v8:
* Dropped exporting the SNP initialized API from ccp to KVM. Instead
call SNP_PLATFORM_STATUS within KVM to query the initialization. (Tom)
While it may be cheaper to query sev->snp_initialized from ccp, making
the SNP platform call within KVM does away with any dependencies.
v6..v7:
https://lore.kernel.org/kvm/20250221210200.244405-7-prsampat@amd.com/
Based on comments from Sean -
* Replaced FW check with sev->snp_initialized
* Dropped the patch which removes SEV+ KVM advertisement 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 tested
* 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 advertises 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 | 30 +++++-
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 +++++++++------
12 files changed, 261 insertions(+), 60 deletions(-)
--
2.43.0
On Wed, 05 Mar 2025 16:59:50 -0600, Pratik R. Sampat wrote:
> 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.
>
> [...]
Applied 2-9 to kvm-x86 selftests. AIUI, the KVM side of things should already
be fixed. If KVM isn't fixed, I want to take that discussion/patch to a
separate thread.
I made minor changes along the way (some details in the commits' []), please
holler if you disagree with the end result.
[01/10] KVM: SEV: Disable SEV-SNP support on initialization failure
(no commit info)
[02/10] KVM: selftests: SEV-SNP test for KVM_SEV_INIT2
https://github.com/kvm-x86/linux/commit/68ed692e3954
[03/10] KVM: selftests: Add vmgexit helper
https://github.com/kvm-x86/linux/commit/c4e1a848d721
[04/10] KVM: selftests: Add SMT control state helper
https://github.com/kvm-x86/linux/commit/acf064345018
[05/10] KVM: selftests: Replace assert() with TEST_ASSERT_EQ()
https://github.com/kvm-x86/linux/commit/f694f30e81c4
[06/10] KVM: selftests: Introduce SEV VM type check
https://github.com/kvm-x86/linux/commit/4a4e1e8e92eb
[07/10] KVM: selftests: Add library support for interacting with SNP
https://github.com/kvm-x86/linux/commit/3bf3e0a52123
[08/10] KVM: selftests: Force GUEST_MEMFD flag for SNP VM type
https://github.com/kvm-x86/linux/commit/b73a30cd9caa
[09/10] KVM: selftests: Abstractions for SEV to decouple policy from type
https://github.com/kvm-x86/linux/commit/a5d55f783fb7
[10/10] KVM: selftests: Add a basic SEV-SNP smoke test
https://github.com/kvm-x86/linux/commit/ada014f5fc67
--
https://github.com/kvm-x86/linux/tree/next
Hi Sean, On 5/2/25 4:50 PM, Sean Christopherson wrote: > On Wed, 05 Mar 2025 16:59:50 -0600, Pratik R. Sampat wrote: >> 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. >> >> [...] > > Applied 2-9 to kvm-x86 selftests. AIUI, the KVM side of things should already > be fixed. If KVM isn't fixed, I want to take that discussion/patch to a > separate thread. > Thanks for pulling these patches in. For 1 - Ashish's commit now returns failure for this case [1]. Although, it appears that the return code isn't checked within sev_platform_init()[2], so it shouldn't change existing behavior. In the kselftest case, if platform init fails, the selftest will also fail — just as it does currently too. Regardless of what we decide on what the right behavior is, fail vs skip (I don't mind the former) we can certainly do that over new patches rebased over the new series. [1]: https://lore.kernel.org/kvm/ab9a028cf232663f9fc839f48cfcf97694846c13.1742850400.git.ashish.kalra@amd.com/ [2]: https://lore.kernel.org/kvm/d8de6de80c36721ea3eb92ecac81b211f401c3b2.1742850400.git.ashish.kalra@amd.com/ > I made minor changes along the way (some details in the commits' []), please > holler if you disagree with the end result. Thank you for cleaning these up! Pratik > > [01/10] KVM: SEV: Disable SEV-SNP support on initialization failure > (no commit info) > [02/10] KVM: selftests: SEV-SNP test for KVM_SEV_INIT2 > https://github.com/kvm-x86/linux/commit/68ed692e3954 > [03/10] KVM: selftests: Add vmgexit helper > https://github.com/kvm-x86/linux/commit/c4e1a848d721 > [04/10] KVM: selftests: Add SMT control state helper > https://github.com/kvm-x86/linux/commit/acf064345018 > [05/10] KVM: selftests: Replace assert() with TEST_ASSERT_EQ() > https://github.com/kvm-x86/linux/commit/f694f30e81c4 > [06/10] KVM: selftests: Introduce SEV VM type check > https://github.com/kvm-x86/linux/commit/4a4e1e8e92eb > [07/10] KVM: selftests: Add library support for interacting with SNP > https://github.com/kvm-x86/linux/commit/3bf3e0a52123 > [08/10] KVM: selftests: Force GUEST_MEMFD flag for SNP VM type > https://github.com/kvm-x86/linux/commit/b73a30cd9caa > [09/10] KVM: selftests: Abstractions for SEV to decouple policy from type > https://github.com/kvm-x86/linux/commit/a5d55f783fb7 > [10/10] KVM: selftests: Add a basic SEV-SNP smoke test > https://github.com/kvm-x86/linux/commit/ada014f5fc67 > > -- > https://github.com/kvm-x86/linux/tree/next
On Mon, May 05, 2025, Pratik R. Sampat wrote:
> Hi Sean,
>
> On 5/2/25 4:50 PM, Sean Christopherson wrote:
> > On Wed, 05 Mar 2025 16:59:50 -0600, Pratik R. Sampat wrote:
> >> 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.
> >>
> >> [...]
> >
> > Applied 2-9 to kvm-x86 selftests. AIUI, the KVM side of things should already
> > be fixed. If KVM isn't fixed, I want to take that discussion/patch to a
> > separate thread.
> >
>
> Thanks for pulling these patches in.
>
> For 1 - Ashish's commit now returns failure for this case [1].
> Although, it appears that the return code isn't checked within
> sev_platform_init()[2], so it shouldn't change existing behavior. In the
> kselftest case, if platform init fails, the selftest will also fail — just as
> it does currently too.
Argh, now I remember the issue. But _sev_platform_init_locked() returns '0' if
psp_init_on_probe is true, and I don't see how deferring __sev_snp_init_locked()
will magically make it succeed the second time around.
So shouldn't the KVM code be this?
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index e0f446922a6e..dd04f979357d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3038,6 +3038,14 @@ void __init sev_hardware_setup(void)
sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
out:
+ if (sev_enabled) {
+ init_args.probe = true;
+ if (sev_platform_init(&init_args))
+ sev_supported = sev_es_supported = sev_snp_supported = false;
+ else
+ sev_snp_supported &= sev_is_snp_initialized();
+ }
+
if (boot_cpu_has(X86_FEATURE_SEV))
pr_info("SEV %s (ASIDs %u - %u)\n",
sev_supported ? min_sev_asid <= max_sev_asid ? "enabled" :
@@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
if (!sev_enabled)
return;
-
- /*
- * Do both SNP and SEV initialization at KVM module load.
- */
- init_args.probe = true;
- sev_platform_init(&init_args);
}
void sev_hardware_unsetup(void)
--
Ashish, what am I missing?
> Regardless of what we decide on what the right behavior is, fail vs skip (I
> don't mind the former) we can certainly do that over new patches rebased over
> the new series.
FAIL, for sure. Unless someone else pipes up with a good reason why they need
to defer INIT_EX, that's Google's problem to solve.
On 5/5/2025 6:15 PM, Sean Christopherson wrote:
> On Mon, May 05, 2025, Pratik R. Sampat wrote:
>> Hi Sean,
>>
>> On 5/2/25 4:50 PM, Sean Christopherson wrote:
>>> On Wed, 05 Mar 2025 16:59:50 -0600, Pratik R. Sampat wrote:
>>>> 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.
>>>>
>>>> [...]
>>>
>>> Applied 2-9 to kvm-x86 selftests. AIUI, the KVM side of things should already
>>> be fixed. If KVM isn't fixed, I want to take that discussion/patch to a
>>> separate thread.
>>>
>>
>> Thanks for pulling these patches in.
>>
>> For 1 - Ashish's commit now returns failure for this case [1].
>> Although, it appears that the return code isn't checked within
>> sev_platform_init()[2], so it shouldn't change existing behavior. In the
>> kselftest case, if platform init fails, the selftest will also fail — just as
>> it does currently too.
>
> Argh, now I remember the issue. But _sev_platform_init_locked() returns '0' if
> psp_init_on_probe is true, and I don't see how deferring __sev_snp_init_locked()
> will magically make it succeed the second time around.
>
> So shouldn't the KVM code be this?
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index e0f446922a6e..dd04f979357d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3038,6 +3038,14 @@ void __init sev_hardware_setup(void)
> sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>
> out:
> + if (sev_enabled) {
> + init_args.probe = true;
> + if (sev_platform_init(&init_args))
> + sev_supported = sev_es_supported = sev_snp_supported = false;
> + else
> + sev_snp_supported &= sev_is_snp_initialized();
> + }
> +
> if (boot_cpu_has(X86_FEATURE_SEV))
> pr_info("SEV %s (ASIDs %u - %u)\n",
> sev_supported ? min_sev_asid <= max_sev_asid ? "enabled" :
> @@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
>
> if (!sev_enabled)
> return;
> -
> - /*
> - * Do both SNP and SEV initialization at KVM module load.
> - */
> - init_args.probe = true;
> - sev_platform_init(&init_args);
> }
>
> void sev_hardware_unsetup(void)
> --
>
I agree with this approach. One thing maybe to consider further is to also call
into SEV_platform_status() to check for init so that SEV/SEV-ES is not
penalized and disabled for SNP's failures. Another approach could be to break
up the SEV and SNP init setup so that we can spare a couple of platform calls
in the process?
> Ashish, what am I missing?
>
>> Regardless of what we decide on what the right behavior is, fail vs skip (I
>> don't mind the former) we can certainly do that over new patches rebased over
>> the new series.
>
> FAIL, for sure. Unless someone else pipes up with a good reason why they need
> to defer INIT_EX, that's Google's problem to solve.
Ack!
Pratik
On Mon, May 05, 2025, Pratik R. Sampat wrote:
> On 5/5/2025 6:15 PM, Sean Christopherson wrote:
> > On Mon, May 05, 2025, Pratik R. Sampat wrote:
> > Argh, now I remember the issue. But _sev_platform_init_locked() returns '0' if
> > psp_init_on_probe is true, and I don't see how deferring __sev_snp_init_locked()
> > will magically make it succeed the second time around.
> >
> > So shouldn't the KVM code be this?
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index e0f446922a6e..dd04f979357d 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -3038,6 +3038,14 @@ void __init sev_hardware_setup(void)
> > sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
> >
> > out:
> > + if (sev_enabled) {
> > + init_args.probe = true;
> > + if (sev_platform_init(&init_args))
> > + sev_supported = sev_es_supported = sev_snp_supported = false;
> > + else
> > + sev_snp_supported &= sev_is_snp_initialized();
> > + }
> > +
> > if (boot_cpu_has(X86_FEATURE_SEV))
> > pr_info("SEV %s (ASIDs %u - %u)\n",
> > sev_supported ? min_sev_asid <= max_sev_asid ? "enabled" :
> > @@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
> >
> > if (!sev_enabled)
> > return;
> > -
> > - /*
> > - * Do both SNP and SEV initialization at KVM module load.
> > - */
> > - init_args.probe = true;
> > - sev_platform_init(&init_args);
> > }
> >
> > void sev_hardware_unsetup(void)
> > --
> >
>
> I agree with this approach. One thing maybe to consider further is to also call
> into SEV_platform_status() to check for init so that SEV/SEV-ES is not
> penalized and disabled for SNP's failures. Another approach could be to break
> up the SEV and SNP init setup so that we can spare a couple of platform calls
> in the process?
Nah, SNP initialization failure should be a rare occurence, I don't want to make
the "normal" flow more complex just to handle something that should never happen
in practice.
Hello Sean,
On 5/5/2025 6:15 PM, Sean Christopherson wrote:
> On Mon, May 05, 2025, Pratik R. Sampat wrote:
>> Hi Sean,
>>
>> On 5/2/25 4:50 PM, Sean Christopherson wrote:
>>> On Wed, 05 Mar 2025 16:59:50 -0600, Pratik R. Sampat wrote:
>>>> 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.
>>>>
>>>> [...]
>>>
>>> Applied 2-9 to kvm-x86 selftests. AIUI, the KVM side of things should already
>>> be fixed. If KVM isn't fixed, I want to take that discussion/patch to a
>>> separate thread.
>>>
>>
>> Thanks for pulling these patches in.
>>
>> For 1 - Ashish's commit now returns failure for this case [1].
>> Although, it appears that the return code isn't checked within
>> sev_platform_init()[2], so it shouldn't change existing behavior. In the
>> kselftest case, if platform init fails, the selftest will also fail — just as
>> it does currently too.
>
> Argh, now I remember the issue. But _sev_platform_init_locked() returns '0' if
> psp_init_on_probe is true, and I don't see how deferring __sev_snp_init_locked()
> will magically make it succeed the second time around.
>
> So shouldn't the KVM code be this?
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index e0f446922a6e..dd04f979357d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3038,6 +3038,14 @@ void __init sev_hardware_setup(void)
> sev_snp_supported = sev_snp_enabled && cc_platform_has(CC_ATTR_HOST_SEV_SNP);
>
> out:
> + if (sev_enabled) {
> + init_args.probe = true;
> + if (sev_platform_init(&init_args))
> + sev_supported = sev_es_supported = sev_snp_supported = false;
> + else
> + sev_snp_supported &= sev_is_snp_initialized();
> + }
> +
> if (boot_cpu_has(X86_FEATURE_SEV))
> pr_info("SEV %s (ASIDs %u - %u)\n",
> sev_supported ? min_sev_asid <= max_sev_asid ? "enabled" :
> @@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
>
> if (!sev_enabled)
> return;
> -
> - /*
> - * Do both SNP and SEV initialization at KVM module load.
> - */
> - init_args.probe = true;
> - sev_platform_init(&init_args);
> }
>
> void sev_hardware_unsetup(void)
> --
>
> Ashish, what am I missing?
>
As far as setting sev*_enabled is concerned, i believe they are more specific to SNP/SEV/SEV-ES being enabled in the system,
which is separate from SEV_INIT/SNP_INIT (SNP_INIT success indicates that RMP been initialized, SNP has to be already enabled via
MSR_SYSCFG before SNP_INIT is called), though SEV_INIT/SNP_INIT may fail but SEV/SNP support will still be enabled on the
system.
Additionally as SEV_INIT/SNP_INIT during sev_platform_init() have failed, so any SEV/SEV-ES/SNP VM launch will fail
as the firmware will return invalid platform state as INITs have failed.
From my understanding, sev*_enabled indicates the user support to enable/disable support for SEV/SEV-ES/SEV-SNP,
as the sev*_enabled are the KVM module parameters, while sev*_supported indicates if platform has that support enabled.
And before the SEV/SNP init support was moved to KVM from CCP module, doing SEV/SNP INIT could fail but that still
had KVM detecting SEV/SNP support enabled, so this moving SEV/SNP init stuff to KVM module from CCP driver is
consistent with the previous behavior.
Thanks,
Ashish
>> Regardless of what we decide on what the right behavior is, fail vs skip (I
>> don't mind the former) we can certainly do that over new patches rebased over
>> the new series.
>
> FAIL, for sure. Unless someone else pipes up with a good reason why they need
> to defer INIT_EX, that's Google's problem to solve.
On Mon, May 05, 2025, Ashish Kalra wrote:
> On 5/5/2025 6:15 PM, Sean Christopherson wrote:
> > @@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
> >
> > if (!sev_enabled)
> > return;
> > -
> > - /*
> > - * Do both SNP and SEV initialization at KVM module load.
> > - */
> > - init_args.probe = true;
> > - sev_platform_init(&init_args);
> > }
> >
> > void sev_hardware_unsetup(void)
> > --
> >
> > Ashish, what am I missing?
> >
>
> As far as setting sev*_enabled is concerned, i believe they are more specific
> to SNP/SEV/SEV-ES being enabled in the system, which is separate from
> SEV_INIT/SNP_INIT (SNP_INIT success indicates that RMP been initialized, SNP
> has to be already enabled via MSR_SYSCFG before SNP_INIT is called), though
> SEV_INIT/SNP_INIT may fail but SEV/SNP support will still be enabled on the
> system.
No, if SNP_INIT fails and has zero chance of succeeding, then SNP is *NOT*
supported *by KVM*. The platform may be configured to support SNP, but that
matters not at all if KVM can't actually use the functionality.
> Additionally as SEV_INIT/SNP_INIT during sev_platform_init() have failed, so
> any SEV/SEV-ES/SNP VM launch will fail as the firmware will return invalid
> platform state as INITs have failed.
Yeah, and that's *awful* behavior for KVM. Imagine if KVM did that for every
feature, i.e. enumerated hardware support irrespective of KVM support.
The API is KVM_GET_SUPPORTED_CPUID, not KVM_GET_MOSTLY_SUPPORTED_CPUID.
> >From my understanding, sev*_enabled indicates the user support to
> >enable/disable support for SEV/SEV-ES/SEV-SNP,
Yes, and they're also used to reflect and enumerate KVM support:
if (sev_enabled) {
kvm_cpu_cap_set(X86_FEATURE_SEV);
kvm_caps.supported_vm_types |= BIT(KVM_X86_SEV_VM);
}
if (sev_es_enabled) {
kvm_cpu_cap_set(X86_FEATURE_SEV_ES);
kvm_caps.supported_vm_types |= BIT(KVM_X86_SEV_ES_VM);
}
if (sev_snp_enabled) {
kvm_cpu_cap_set(X86_FEATURE_SEV_SNP);
kvm_caps.supported_vm_types |= BIT(KVM_X86_SNP_VM);
}
> as the sev*_enabled are the KVM module parameters, while sev*_supported
> indicates if platform has that support enabled.
sev*_supported are completely irrelevant. They are function local scratch variables
that exist so that KVM doesn't clobber userspace's inputs while computing what is
fully supported and enabled.
> And before the SEV/SNP init support was moved to KVM from CCP module, doing
> SEV/SNP INIT could fail but that still had KVM detecting SEV/SNP support
> enabled, so this moving SEV/SNP init stuff to KVM module from CCP driver is
> consistent with the previous behavior.
And one of my driving motivations for getting the initialization into KVM was to
fix that previous behavior.
Hello Sean,
On 5/5/2025 7:56 PM, Sean Christopherson wrote:
> On Mon, May 05, 2025, Ashish Kalra wrote:
>> On 5/5/2025 6:15 PM, Sean Christopherson wrote:
>>> @@ -3067,12 +3075,6 @@ void __init sev_hardware_setup(void)
>>>
>>> if (!sev_enabled)
>>> return;
>>> -
>>> - /*
>>> - * Do both SNP and SEV initialization at KVM module load.
>>> - */
>>> - init_args.probe = true;
>>> - sev_platform_init(&init_args);
>>> }
>>>
>>> void sev_hardware_unsetup(void)
>>> --
>>>
>>> Ashish, what am I missing?
>>>
>>
>> As far as setting sev*_enabled is concerned, i believe they are more specific
>> to SNP/SEV/SEV-ES being enabled in the system, which is separate from
>> SEV_INIT/SNP_INIT (SNP_INIT success indicates that RMP been initialized, SNP
>> has to be already enabled via MSR_SYSCFG before SNP_INIT is called), though
>> SEV_INIT/SNP_INIT may fail but SEV/SNP support will still be enabled on the
>> system.
>
> No, if SNP_INIT fails and has zero chance of succeeding, then SNP is *NOT*
> supported *by KVM*. The platform may be configured to support SNP, but that
> matters not at all if KVM can't actually use the functionality.
>
Yes, i agree.
>> Additionally as SEV_INIT/SNP_INIT during sev_platform_init() have failed, so
>> any SEV/SEV-ES/SNP VM launch will fail as the firmware will return invalid
>> platform state as INITs have failed.
>
> Yeah, and that's *awful* behavior for KVM. Imagine if KVM did that for every
> feature, i.e. enumerated hardware support irrespective of KVM support.
>
> The API is KVM_GET_SUPPORTED_CPUID, not KVM_GET_MOSTLY_SUPPORTED_CPUID.
>
>> >From my understanding, sev*_enabled indicates the user support to
>>> enable/disable support for SEV/SEV-ES/SEV-SNP,
>
> Yes, and they're also used to reflect and enumerate KVM support:
>
> if (sev_enabled) {
> kvm_cpu_cap_set(X86_FEATURE_SEV);
> kvm_caps.supported_vm_types |= BIT(KVM_X86_SEV_VM);
> }
> if (sev_es_enabled) {
> kvm_cpu_cap_set(X86_FEATURE_SEV_ES);
> kvm_caps.supported_vm_types |= BIT(KVM_X86_SEV_ES_VM);
> }
> if (sev_snp_enabled) {
> kvm_cpu_cap_set(X86_FEATURE_SEV_SNP);
> kvm_caps.supported_vm_types |= BIT(KVM_X86_SNP_VM);
> }
>
>> as the sev*_enabled are the KVM module parameters, while sev*_supported
>> indicates if platform has that support enabled.
>
> sev*_supported are completely irrelevant. They are function local scratch variables
> that exist so that KVM doesn't clobber userspace's inputs while computing what is
> fully supported and enabled.
>
Ok.
>> And before the SEV/SNP init support was moved to KVM from CCP module, doing
>> SEV/SNP INIT could fail but that still had KVM detecting SEV/SNP support
>> enabled, so this moving SEV/SNP init stuff to KVM module from CCP driver is
>> consistent with the previous behavior.
>
> And one of my driving motivations for getting the initialization into KVM was to
> fix that previous behavior.
Sure, i will test your patch and post it.
Thanks,
Ashish
A very gentle ping on this series. Thanks Pratik On 3/5/25 4:59 PM, Pratik R. Sampat wrote: > 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 advertise 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 > > v7..v8: > * Dropped exporting the SNP initialized API from ccp to KVM. Instead > call SNP_PLATFORM_STATUS within KVM to query the initialization. (Tom) > > While it may be cheaper to query sev->snp_initialized from ccp, making > the SNP platform call within KVM does away with any dependencies. > > v6..v7: > https://lore.kernel.org/kvm/20250221210200.244405-7-prsampat@amd.com/ > Based on comments from Sean - > * Replaced FW check with sev->snp_initialized > * Dropped the patch which removes SEV+ KVM advertisement 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 tested > * 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 advertises 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 | 30 +++++- > 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 +++++++++------ > 12 files changed, 261 insertions(+), 60 deletions(-) >
© 2016 - 2025 Red Hat, Inc.