Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On 11/4/2024 5:47 PM, Sean Christopherson wrote:
> On Mon, Nov 04, 2024, Pratik R. Sampat wrote:
>>
>>
>> On 10/31/2024 11:27 AM, Sean Christopherson wrote:
>>> On Thu, Oct 31, 2024, Pratik R. Sampat wrote:
Hi Sean,
On 10/30/2024 12:57 PM, Sean Christopherson wrote:
> On Wed, Oct 30, 2024, Pratik R. Sampat wrote:
>> On 10/30/2024 8:46 AM, Sean Christopherson wrote:
>>> +/* Minimum firmware version required for the SEV-SNP support */
>>> +#define SNP_FW_REQ_VER_MAJOR 1
>>> +#define SNP_FW_REQ_VER_MINOR 51
>>>
>>> Side topic, why are these hardcoded? And where did they come from? If
>>> they're
>>> arbitrary KVM selftests values, make that super duper clear.
>>
>> Well, it's not entirely arbitrary. This was the version that SNP GA'd
>> with first so that kind of became the minimum required version needed.
>>
>> I think the only place we've documented this is here -
>> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
>>
>> Maybe, I can modify the comment above to say something like -
>> Minimum general availability release firmware required for SEV-SNP
>> support.
>
> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51,
> why on
> earth is that not checked and enforced by the kernel? Relying on
> userspace to
> not crash the host (or worse) because of unsupported firmware is not a
> winning
> strategy.
We do check against the firmware level 1.51 while setting things up
first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail
out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any
other corresponding SNP calls should fail cleanly without any adverse
effects to the host.
>>>
>>> And I'm saying, that's not good enough. If the platform doesn't support
>>> SNP,
>>> the KVM *must not* advertise support for SNP.
>>>
>>
>> Sure, fair to expect this. Currently, if the FW check fails, SNP is not
>> setup and there is nothing that indicates in the KVM capabilities (apart
>> from one dmesg error) that the support does not exist.
>>
>> One thing I could do (as an independent patch) is to introduce a CC API
>> that abstracts the FW version check made by the CCP module. Since sev
>> platform status can be gotten before INIT to extract the major and minor
>> version numbers, KVM can also call into this API and use that to decide
>> if the KVM capabilities for SNP must be set or not.
>
> Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support
> SNP?
> KVM shouldn't have to care about some arbitrary firmware API version, the
> whole
> point of a driver is so that KVM doesn't have to deal with such details.
>
> I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM
> itself shouldn't need to manually check the firmware version.
Clearing CC_ATTR_HOST_SEV_SNP when the init fails is one approach to go
about it. Here we could clear it from here and eventually that would
prevent the the SNP feature being set in KVM capability.
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1099,6 +1099,7 @@ static int __sev_snp_init_locked(int *error)
return 0;
if (!sev_version_greater_or_equal(SNP_MIN_API_MAJOR,
SNP_MIN_API_MINOR)) {
+ cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
A suggestion where we could more directly approach this could be by
exporting an explicit check from ccp instead?
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -122,6 +122,12 @@ static inline bool sev_version_greater_or_equal(u8 maj, u8
min)
return false;
}
+bool sev_snp_fw_available(void)
+{
+return sev_version_greater_or_equal(SNP_MIN_API_MAJOR, SNP_MIN_API_MINOR);
+}
+EXPORT_SYMBOL_GPL(sev_snp_fw_available);
which could be then called on will as follows:
--- 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) && sev_snp_fw_available();
out:
if (boot_cpu_has(X86_FEATURE_SEV))
This would ensure that we could enable and disable the SNP capability
even in the case where maybe the firmware can get hotloaded using the
proposed download_firmware_ex[1] or in cases where the INIT could be
deferred; all while KVM wouldn't need to be bothered with the API
details.
[1]:
https://lore.kernel.org/lkml/20241029183907.3536683-1-dionnagl...@google.com/
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On Mon, Nov 04, 2024, Pratik R. Sampat wrote: > > > On 10/31/2024 11:27 AM, Sean Christopherson wrote: > > On Thu, Oct 31, 2024, Pratik R. Sampat wrote: > >> Hi Sean, > >> > >> On 10/30/2024 12:57 PM, Sean Christopherson wrote: > >>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > On 10/30/2024 8:46 AM, Sean Christopherson wrote: > > +/* Minimum firmware version required for the SEV-SNP support */ > > +#define SNP_FW_REQ_VER_MAJOR 1 > > +#define SNP_FW_REQ_VER_MINOR 51 > > > > Side topic, why are these hardcoded? And where did they come from? If > > they're > > arbitrary KVM selftests values, make that super duper clear. > > Well, it's not entirely arbitrary. This was the version that SNP GA'd > with first so that kind of became the minimum required version needed. > > I think the only place we've documented this is here - > https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > > Maybe, I can modify the comment above to say something like - > Minimum general availability release firmware required for SEV-SNP > support. > >>> > >>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, > >>> why on > >>> earth is that not checked and enforced by the kernel? Relying on > >>> userspace to > >>> not crash the host (or worse) because of unsupported firmware is not a > >>> winning > >>> strategy. > >> > >> We do check against the firmware level 1.51 while setting things up > >> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail > >> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any > >> other corresponding SNP calls should fail cleanly without any adverse > >> effects to the host. > > > > And I'm saying, that's not good enough. If the platform doesn't support > > SNP, > > the KVM *must not* advertise support for SNP. > > > > Sure, fair to expect this. Currently, if the FW check fails, SNP is not > setup and there is nothing that indicates in the KVM capabilities (apart > from one dmesg error) that the support does not exist. > > One thing I could do (as an independent patch) is to introduce a CC API > that abstracts the FW version check made by the CCP module. Since sev > platform status can be gotten before INIT to extract the major and minor > version numbers, KVM can also call into this API and use that to decide > if the KVM capabilities for SNP must be set or not. Why is CC_ATTR_HOST_SEV_SNP set if hardware/firmware can't actually support SNP? KVM shouldn't have to care about some arbitrary firmware API version, the whole point of a driver is so that KVM doesn't have to deal with such details. I'm a-ok with a KVM selftest *asserting* that the kernel isn't broken, but KVM itself shouldn't need to manually check the firmware version.
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On 10/31/2024 11:27 AM, Sean Christopherson wrote: > On Thu, Oct 31, 2024, Pratik R. Sampat wrote: >> Hi Sean, >> >> On 10/30/2024 12:57 PM, Sean Christopherson wrote: >>> On Wed, Oct 30, 2024, Pratik R. Sampat wrote: On 10/30/2024 8:46 AM, Sean Christopherson wrote: > +/* Minimum firmware version required for the SEV-SNP support */ > +#define SNP_FW_REQ_VER_MAJOR 1 > +#define SNP_FW_REQ_VER_MINOR 51 > > Side topic, why are these hardcoded? And where did they come from? If > they're > arbitrary KVM selftests values, make that super duper clear. Well, it's not entirely arbitrary. This was the version that SNP GA'd with first so that kind of became the minimum required version needed. I think the only place we've documented this is here - https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. Maybe, I can modify the comment above to say something like - Minimum general availability release firmware required for SEV-SNP support. >>> >>> Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why >>> on >>> earth is that not checked and enforced by the kernel? Relying on userspace >>> to >>> not crash the host (or worse) because of unsupported firmware is not a >>> winning >>> strategy. >> >> We do check against the firmware level 1.51 while setting things up >> first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail >> out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any >> other corresponding SNP calls should fail cleanly without any adverse >> effects to the host. > > And I'm saying, that's not good enough. If the platform doesn't support SNP, > the KVM *must not* advertise support for SNP. > Sure, fair to expect this. Currently, if the FW check fails, SNP is not setup and there is nothing that indicates in the KVM capabilities (apart from one dmesg error) that the support does not exist. One thing I could do (as an independent patch) is to introduce a CC API that abstracts the FW version check made by the CCP module. Since sev platform status can be gotten before INIT to extract the major and minor version numbers, KVM can also call into this API and use that to decide if the KVM capabilities for SNP must be set or not. Thanks! Pratik >> From the positive selftest perspective though, we want to make sure it's >> both supported and enabled, and skip the test if not. > > No, we want the test to assert that KVM reports SNP support if and only if SNP > is 100% supported. > >> I believe we can tell if it's supported by the platform using the MSR - >> MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM >> capabilities. However, to determine if it's enabled from the kernel, I >> made this check here. Having said that, I do agree that there should >> probably be a better way to expose this support to the userspace. >> >> Thanks >> Pratik
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On Thu, Oct 31, 2024, Pratik R. Sampat wrote: > Hi Sean, > > On 10/30/2024 12:57 PM, Sean Christopherson wrote: > > On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > >> On 10/30/2024 8:46 AM, Sean Christopherson wrote: > >>> +/* Minimum firmware version required for the SEV-SNP support */ > >>> +#define SNP_FW_REQ_VER_MAJOR 1 > >>> +#define SNP_FW_REQ_VER_MINOR 51 > >>> > >>> Side topic, why are these hardcoded? And where did they come from? If > >>> they're > >>> arbitrary KVM selftests values, make that super duper clear. > >> > >> Well, it's not entirely arbitrary. This was the version that SNP GA'd > >> with first so that kind of became the minimum required version needed. > >> > >> I think the only place we've documented this is here - > >> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > >> > >> Maybe, I can modify the comment above to say something like - > >> Minimum general availability release firmware required for SEV-SNP support. > > > > Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why > > on > > earth is that not checked and enforced by the kernel? Relying on userspace > > to > > not crash the host (or worse) because of unsupported firmware is not a > > winning > > strategy. > > We do check against the firmware level 1.51 while setting things up > first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail > out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any > other corresponding SNP calls should fail cleanly without any adverse > effects to the host. And I'm saying, that's not good enough. If the platform doesn't support SNP, the KVM *must not* advertise support for SNP. > From the positive selftest perspective though, we want to make sure it's > both supported and enabled, and skip the test if not. No, we want the test to assert that KVM reports SNP support if and only if SNP is 100% supported. > I believe we can tell if it's supported by the platform using the MSR - > MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM > capabilities. However, to determine if it's enabled from the kernel, I > made this check here. Having said that, I do agree that there should > probably be a better way to expose this support to the userspace. > > Thanks > Pratik
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
Hi Sean, On 10/30/2024 12:57 PM, Sean Christopherson wrote: > On Wed, Oct 30, 2024, Pratik R. Sampat wrote: >> On 10/30/2024 8:46 AM, Sean Christopherson wrote: >>> +/* Minimum firmware version required for the SEV-SNP support */ >>> +#define SNP_FW_REQ_VER_MAJOR 1 >>> +#define SNP_FW_REQ_VER_MINOR 51 >>> >>> Side topic, why are these hardcoded? And where did they come from? If >>> they're >>> arbitrary KVM selftests values, make that super duper clear. >> >> Well, it's not entirely arbitrary. This was the version that SNP GA'd >> with first so that kind of became the minimum required version needed. >> >> I think the only place we've documented this is here - >> https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. >> >> Maybe, I can modify the comment above to say something like - >> Minimum general availability release firmware required for SEV-SNP support. > > Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on > earth is that not checked and enforced by the kernel? Relying on userspace to > not crash the host (or worse) because of unsupported firmware is not a winning > strategy. We do check against the firmware level 1.51 while setting things up first (drivers/crypto/ccp/sev-dev.c:__sev_snp_init_locked()) and we bail out if it's otherwise. From the userspace, calls to KVM_SEV_INIT2 or any other corresponding SNP calls should fail cleanly without any adverse effects to the host. >From the positive selftest perspective though, we want to make sure it's both supported and enabled, and skip the test if not. I believe we can tell if it's supported by the platform using the MSR - MSR_AMD64_SEV_SNP_ENABLED or the X86_FEATURE_SEV_SNP from the KVM capabilities. However, to determine if it's enabled from the kernel, I made this check here. Having said that, I do agree that there should probably be a better way to expose this support to the userspace. Thanks Pratik
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On Wed, Oct 30, 2024, Pratik R. Sampat wrote: > On 10/30/2024 8:46 AM, Sean Christopherson wrote: > > +/* Minimum firmware version required for the SEV-SNP support */ > > +#define SNP_FW_REQ_VER_MAJOR 1 > > +#define SNP_FW_REQ_VER_MINOR 51 > > > > Side topic, why are these hardcoded? And where did they come from? If > > they're > > arbitrary KVM selftests values, make that super duper clear. > > Well, it's not entirely arbitrary. This was the version that SNP GA'd > with first so that kind of became the minimum required version needed. > > I think the only place we've documented this is here - > https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware. > > Maybe, I can modify the comment above to say something like - > Minimum general availability release firmware required for SEV-SNP support. Hmm, so if AMD says SNP is only supported for firmware version >= 1.51, why on earth is that not checked and enforced by the kernel? Relying on userspace to not crash the host (or worse) because of unsupported firmware is not a winning strategy.
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
Hi Sean,
On 10/30/2024 8:46 AM, Sean Christopherson wrote:
> On Mon, Oct 28, 2024, Pratik R. Sampat wro4te:
>> On 10/28/2024 12:55 PM, Sean Christopherson wrote:
>>> On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
>> +if (unlikely(!is_smt_active()))
>> +snp_policy &= ~SNP_POLICY_SMT;
>
> Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this?
>
> u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
>
I think most systems support SMT so I enabled the bit in by default and
only unset it when there isn't any support.
>>>
>>> That's confusing though, because you're mixing architectural defines with
>>> semi-
>>> arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly
>>> coupled
>>> with SNP, i.e. SNP can't exist without that bit, so it makes sense that
>>> RSVD_MBO
>>> needs to be part of SNP_POLICY
>>>
>>> If you want to have a *software*-defined default policy, then make it
>>> obvious that
>>> it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply
>>> SNP_POLICY, because the latter is too easily misconstrued as the base SNP
>>> policy,
>>> which it is not. That said, IIUC, SMT *must* match the host configuration,
>>> i.e.
>>> whether or not SMT is set is non-negotiable. In that case, there's zero
>>> value in
>>> defining SNP_DEFAULT_POLICY, because it can't be a sane default for all
>>> systems.
>>>
>>
>> Right, SMT should match the host configuration. Would a
>> SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?
>>
>> Instead of,
>> #define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
>>
>> Have something like this instead to make it generic and less ambiguous?
>> #define SNP_DEFAULT_POLICY()\
>> ({ \
>> SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0); \
>> })
>
> No, unless it's the least awful option, don't hide dynamic functionality in a
> macro
> that looks like it holds static data. The idea is totally fine, but put it
> in an
> actual helper, not a macro, _if_ there's actually a need for a default policy.
> If there's only ever one main path that creates SNP VMs, then I don't see the
> point
> in specifying a default policy.
>
Currently, there just seems to be one path of doing (later the prefault
tests exercise it) so I'm not too averse to just dropping it and having
what bits needs to be set during the main path.
I had only introduced it so that it would be easy to specify a minimal
working SNP policy as it was for SEV and SEV-ES without too much
ambiguity. But if it's causing more issues than resolving, I can
definitely get rid of it.
>>> Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be
>>> specified,
>>> and that they are mutualy exclusive? E.g. what happens if the full policy
>>> is simply
>>> SNP_POLICY_RSVD_MBO?
>>
>> SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
>> SEV-ES - pg 31, Table 2
>> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf
>>
>> and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
>> 27, Table 9
>> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
>>
>> In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
>> set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.
>
> Ugh, one is SEV_xxx, the other is SNP_xxx. Argh! And IIUC, they are mutually
> exclusive (totally separate thigns?), because SNP guests use an 8-byte
> structure,
> whereas SEV/SEV-ES use a 4-byte structure, and with different layouts.
>
> That means this is _extremely_ confusing. Separate the SEV_xxx defines from
> the
> SNP_xxx defines, because other than a name, they have nothing in common.
>
Right. I see how that can be confusing. Sure I can make sure not to
bundle up these defines together.
> +/* Minimum firmware version required for the SEV-SNP support */
> +#define SNP_FW_REQ_VER_MAJOR 1
> +#define SNP_FW_REQ_VER_MINOR 51
>
> Side topic, why are these hardcoded? And where did they come from? If
> they're
> arbitrary KVM selftests values, make that super duper clear.
Well, it's not entirely arbitrary. This was the version that SNP GA'd
with first so that kind of became the minimum required version needed.
I think the only place we've documented this is here -
https://github.com/AMDESE/AMDSEV/tree/snp-latest?tab=readme-ov-file#upgrade-sev-firmware.
Maybe, I can modify the comment above to say something like -
Minimum general availability release firmware required for SEV-SNP support.
>
> +#define SNP_POLICY_MINOR_BIT 0
> +#define SNP_POLICY_MAJOR_BIT 8
>
> s/BIT/SHIFT. "BIT" implies they are a single bit, which is obviously not the
> c
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On Mon, Oct 28, 2024, Pratik R. Sampat wro4te:
> On 10/28/2024 12:55 PM, Sean Christopherson wrote:
> > On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
> +if (unlikely(!is_smt_active()))
> +snp_policy &= ~SNP_POLICY_SMT;
> >>>
> >>> Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this?
> >>>
> >>> u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
> >>>
> >>
> >> I think most systems support SMT so I enabled the bit in by default and
> >> only unset it when there isn't any support.
> >
> > That's confusing though, because you're mixing architectural defines with
> > semi-
> > arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly
> > coupled
> > with SNP, i.e. SNP can't exist without that bit, so it makes sense that
> > RSVD_MBO
> > needs to be part of SNP_POLICY
> >
> > If you want to have a *software*-defined default policy, then make it
> > obvious that
> > it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply
> > SNP_POLICY, because the latter is too easily misconstrued as the base SNP
> > policy,
> > which it is not. That said, IIUC, SMT *must* match the host configuration,
> > i.e.
> > whether or not SMT is set is non-negotiable. In that case, there's zero
> > value in
> > defining SNP_DEFAULT_POLICY, because it can't be a sane default for all
> > systems.
> >
>
> Right, SMT should match the host configuration. Would a
> SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?
>
> Instead of,
> #define SNP_POLICY(SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
>
> Have something like this instead to make it generic and less ambiguous?
> #define SNP_DEFAULT_POLICY() \
> ({ \
> SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0); \
> })
No, unless it's the least awful option, don't hide dynamic functionality in a
macro
that looks like it holds static data. The idea is totally fine, but put it in
an
actual helper, not a macro, _if_ there's actually a need for a default policy.
If there's only ever one main path that creates SNP VMs, then I don't see the
point
in specifying a default policy.
> > Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be
> > specified,
> > and that they are mutualy exclusive? E.g. what happens if the full policy
> > is simply
> > SNP_POLICY_RSVD_MBO?
>
> SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
> SEV-ES - pg 31, Table 2
> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf
>
> and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
> 27, Table 9
> https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
>
> In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
> set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.
Ugh, one is SEV_xxx, the other is SNP_xxx. Argh! And IIUC, they are mutually
exclusive (totally separate thigns?), because SNP guests use an 8-byte
structure,
whereas SEV/SEV-ES use a 4-byte structure, and with different layouts.
That means this is _extremely_ confusing. Separate the SEV_xxx defines from the
SNP_xxx defines, because other than a name, they have nothing in common.
+/* Minimum firmware version required for the SEV-SNP support */
+#define SNP_FW_REQ_VER_MAJOR 1
+#define SNP_FW_REQ_VER_MINOR 51
Side topic, why are these hardcoded? And where did they come from? If they're
arbitrary KVM selftests values, make that super duper clear.
+#define SNP_POLICY_MINOR_BIT 0
+#define SNP_POLICY_MAJOR_BIT 8
s/BIT/SHIFT. "BIT" implies they are a single bit, which is obviously not the
case. But I vote to omit the extra #define entirely and just open code the
shift
in the SNP_FW_VER_{MAJOR,MINOR} macros.
#define SEV_POLICY_NO_DBG (1UL << 0)
#define SEV_POLICY_ES (1UL << 2)
+#define SNP_POLICY_SMT (1ULL << 16)
+#define SNP_POLICY_RSVD_MBO(1ULL << 17)
+#define SNP_POLICY_DBG (1ULL << 19)
+#define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
+
+#define SNP_FW_VER_MAJOR(maj) ((uint8_t)(maj) << SNP_POLICY_MAJOR_BIT)
+#define SNP_FW_VER_MINOR(min) ((uint8_t)(min) << SNP_POLICY_MINOR_BIT)
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
Hi Sean,
On 10/28/2024 12:55 PM, Sean Christopherson wrote:
> On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
+ test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
test_sev_es_shutdown();
if (kvm_has_cap(KVM_CAP_XCRS) &&
(xgetbv(0) & XFEATURE_MASK_X87_AVX) ==
XFEATURE_MASK_X87_AVX) {
- test_sync_vmsa(0);
- test_sync_vmsa(SEV_POLICY_NO_DBG);
+ test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
+ test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES |
SEV_POLICY_NO_DBG);
+ }
+ }
+
+ if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
>>>
>>> Why do we need both? KVM shouldn't advertise SNP if it's not supported.
>>
>> My rationale behind needing this was that the feature can be advertised
>> but it may not have the right API major or minor release which could be
>> updated post boot and could determine it's support during runtime.
>
> KVM will never determine support after KVM has been loaded. If *KVM* has a
> dependency on the API major.minor, then X86_FEATURE_SNP must be set if and
> only
> if the supported API version is available.
>
> If the API major.minor is purely a userspace thing, then
> is_kvm_snp_supported()
> is misnamed, because the check has nothing to do with KVM. E.g. something
> like
> is_snp_api_version_supported() would be more appropriate.
That's fair. It is related to the FW supplied to it from userspace and
naming it with kvm prefix is misleading. I'll change that.
>
+ unsigned long snp_policy = SNP_POLICY;
>>>
>>> u64, no?
>>
>> Yes, sorry for the oversight. Will change it to u64.
>>
>>>
+
+ if (unlikely(!is_smt_active()))
+ snp_policy &= ~SNP_POLICY_SMT;
>>>
>>> Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this?
>>>
>>> u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
>>>
>>
>> I think most systems support SMT so I enabled the bit in by default and
>> only unset it when there isn't any support.
>
> That's confusing though, because you're mixing architectural defines with
> semi-
> arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly
> coupled
> with SNP, i.e. SNP can't exist without that bit, so it makes sense that
> RSVD_MBO
> needs to be part of SNP_POLICY
>
> If you want to have a *software*-defined default policy, then make it obvious
> that
> it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply
> SNP_POLICY, because the latter is too easily misconstrued as the base SNP
> policy,
> which it is not. That said, IIUC, SMT *must* match the host configuration,
> i.e.
> whether or not SMT is set is non-negotiable. In that case, there's zero
> value in
> defining SNP_DEFAULT_POLICY, because it can't be a sane default for all
> systems.
>
Right, SMT should match the host configuration. Would a
SNP_DEFAULT_POLICY work if we made it check for SMT too in the macro?
Instead of,
#define SNP_POLICY (SNP_POLICY_SMT | SNP_POLICY_RSVD_MBO)
Have something like this instead to make it generic and less ambiguous?
#define SNP_DEFAULT_POLICY() \
({ \
SNP_POLICY_RSVD_MBO | (is_smt_active() ? SNP_POLICY_SMT : 0); \
})
> Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be
> specified,
> and that they are mutualy exclusive? E.g. what happens if the full policy is
> simply
> SNP_POLICY_RSVD_MBO?
SEV_POLICY_NO_DBG is mainly for the guest policy structure of SEV and
SEV-ES - pg 31, Table 2
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/programmer-references/55766_SEV-KM_API_Specification.pdf
and, SNP_POLICY_DBG is a bit in the guest policy structure of SNP - pg
27, Table 9
https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
In the former, a SEV guest disables debugging if SEV_POLICY_NO_DBG is
set. Similarly, a SNP guest enables debugging if SNP_POLICY_DBG is set.
An SNP guest can certainly just have the policy SNP_POLICY_RSVD_MBO,
barring the case on a SMT system where that bit must be set too for a
successful launch.
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On Mon, Oct 21, 2024, Pratik R. Sampat wrote:
> >> + test_sev(guest_sev_es_code, KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> >>
> >>test_sev_es_shutdown();
> >>
> >>if (kvm_has_cap(KVM_CAP_XCRS) &&
> >>(xgetbv(0) & XFEATURE_MASK_X87_AVX) ==
> >> XFEATURE_MASK_X87_AVX) {
> >> - test_sync_vmsa(0);
> >> - test_sync_vmsa(SEV_POLICY_NO_DBG);
> >> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES);
> >> + test_sync_vmsa(KVM_X86_SEV_ES_VM, SEV_POLICY_ES |
> >> SEV_POLICY_NO_DBG);
> >> + }
> >> + }
> >> +
> >> + if (kvm_cpu_has(X86_FEATURE_SNP) && is_kvm_snp_supported()) {
> >
> > Why do we need both? KVM shouldn't advertise SNP if it's not supported.
>
> My rationale behind needing this was that the feature can be advertised
> but it may not have the right API major or minor release which could be
> updated post boot and could determine it's support during runtime.
KVM will never determine support after KVM has been loaded. If *KVM* has a
dependency on the API major.minor, then X86_FEATURE_SNP must be set if and only
if the supported API version is available.
If the API major.minor is purely a userspace thing, then is_kvm_snp_supported()
is misnamed, because the check has nothing to do with KVM. E.g. something like
is_snp_api_version_supported() would be more appropriate.
> >> + unsigned long snp_policy = SNP_POLICY;
> >
> > u64, no?
>
> Yes, sorry for the oversight. Will change it to u64.
>
> >
> >> +
> >> + if (unlikely(!is_smt_active()))
> >> + snp_policy &= ~SNP_POLICY_SMT;
> >
> > Why does SNP_POLICY assume SMT? And what is RSVD_MBO? E.g. why not this?
> >
> > u64 policy = is_smt_active() ? SNP_POLICY_SMT : SNP_POLICY;
> >
>
> I think most systems support SMT so I enabled the bit in by default and
> only unset it when there isn't any support.
That's confusing though, because you're mixing architectural defines with semi-
arbitrary selftests behavior. RSVD_MBO on the other is apparently tightly
coupled
with SNP, i.e. SNP can't exist without that bit, so it makes sense that RSVD_MBO
needs to be part of SNP_POLICY
If you want to have a *software*-defined default policy, then make it obvious
that
it's software defined. E.g. name the #define SNP_DEFAULT_POLICY, not simply
SNP_POLICY, because the latter is too easily misconstrued as the base SNP
policy,
which it is not. That said, IIUC, SMT *must* match the host configuration, i.e.
whether or not SMT is set is non-negotiable. In that case, there's zero value
in
defining SNP_DEFAULT_POLICY, because it can't be a sane default for all systems.
Side topic, I assume one of SEV_POLICY_NO_DBG or SNP_POLICY_DBG *must* be
specified,
and that they are mutualy exclusive? E.g. what happens if the full policy is
simply
SNP_POLICY_RSVD_MBO?
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
Hi Sean,
Thank you for your comments.
...
>> .../selftests/kvm/include/x86_64/processor.h | 1 +
>> .../selftests/kvm/include/x86_64/sev.h| 54 +++-
>> tools/testing/selftests/kvm/lib/kvm_util.c| 8 +-
>> .../selftests/kvm/lib/x86_64/processor.c | 6 +-
>> tools/testing/selftests/kvm/lib/x86_64/sev.c | 116 +-
>> .../selftests/kvm/x86_64/sev_smoke_test.c | 67 --
>> 6 files changed, 230 insertions(+), 22 deletions(-)
>
> There is *way* too much going on in this one patch. There's at least 6+
> patches
> worth of stuff here:
>
> 1. Add x86 architectural defines
> 2. SNP ioctl() plumbing
> 3..N. Other misc plumbing, e.g. is_smt_active()
> N+1. __vm_create() change to force GUEST_MEMFD for SNP
> N+2. Whatever the ASSERT_SEV_POLICY() thing is doing, if it's actually
> necessary
> N+3..M. Refactorings to existing code to prep for SNP
> M+1. SNP support
>
> In general, if you feel the urge to start a changelog paragraph with "Also,"
> that's a sign you need to break up the patch.
Sure. I will split this into multiple patches which should
form the basis of the #1 patch series.
>
>> @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
>> __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm); \
>> })
>>
>> +/* Ensure policy is within bounds for SEV, SEV-ES */
>> +#define ASSERT_SEV_POLICY(type, policy) \
>> +({ \
>> +if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) { \
>> +TEST_ASSERT(policy < ((uint32_t)~0U), \
>> +"Policy beyond bounds for SEV");\
>
> This is asinine. First, there's one user, i.e. I see no reason to have a
> funky
> macro to assert on the type. Second, _if_ this is a common macro, "type" can
> and
> should be incorporated into the assert. Third, I have no idea why selftests
> is
> validating its own inputs to KVM.
It wasn't strictly necessary to validate this. Since the function
vm_sev_launch() now took a u64 in policy (for SNP), I thought it maybe
useful to validate u32 for the rest, as library function can be called
by other selftests as well. However, I do see your point that it doesn't
make too much sense for selftests to try and validate it's own inputs.
I'm open to both - reducing the macro to a just a check within the
function or removing the macro altogether.
>
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> index 974bcd2df6d7..981f3c9fd1cf 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
>> @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
>> sync_global_to_guest(vm, host_cpu_is_amd);
>> sync_global_to_guest(vm, is_forced_emulation_enabled);
>>
>> -if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
>> +if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
>> +vm->type == KVM_X86_SNP_VM) {
>
> Probably time to add a helper, e.g. is_sev_vm() or something. If we follow
> KVM's
> lead, then we'd have is_sev_vm(), is_sev_es_vm(), and is_sev_snp_vm(), where
> an
> SNP VM returns true for all of them. Not sure I love that idea, just
> throwing it
> out there as one possibility.
Agreed. It will be cleaner to have helpers since similar checks are
being made in multiple places.
>
>> struct kvm_sev_init init = { 0 };
>>
>> vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
>> @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits,
>> unsigned int *va_bits)
>>
>> void kvm_init_vm_address_properties(struct kvm_vm *vm)
>> {
>> -if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
>> +if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
>> +vm->type == KVM_X86_SNP_VM) {
>> vm->arch.sev_fd = open_sev_dev_path_or_exit();
>> vm->arch.c_bit =
>> BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT));
>> vm->gpa_tag_mask = vm->arch.c_bit;
>> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> index 125a72246e09..ff3824564854 100644
>> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
>> @@ -14,7 +14,8 @@
>> * and find the first range, but that's correct because the condition
>> * expression would cause us to quit the loop.
>> */
>> -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region
>> *region)
>> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region
>> *region,
>> + uint8_t page_type)
>> {
>> const struct sparsebit *protected_phy_pages =
>> region->protected_phy_pages;
>>
Re: [PATCH v3 2/9] KVM: selftests: Add a basic SNP smoke test
On Thu, Sep 05, 2024, Pratik R. Sampat wrote:
> Extend sev_smoke_test to also run a minimal SEV-SNP smoke test that
> initializes and sets up private memory regions required to run a simple
> SEV-SNP guest.
>
> Similar to its SEV-ES smoke test counterpart, this also does not
> support GHCB and ucall yet and uses the GHCB MSR protocol to trigger an
> exit of the type KVM_EXIT_SYSTEM_EVENT.
>
> Also, decouple policy and type and require functions to provide both
> such that there is no assumption regarding the type using policy.
>
> Signed-off-by: Pratik R. Sampat
> Tested-by: Peter Gonda
> Tested-by: Srikanth Aithal
> ---
> .../selftests/kvm/include/x86_64/processor.h | 1 +
> .../selftests/kvm/include/x86_64/sev.h| 54 +++-
> tools/testing/selftests/kvm/lib/kvm_util.c| 8 +-
> .../selftests/kvm/lib/x86_64/processor.c | 6 +-
> tools/testing/selftests/kvm/lib/x86_64/sev.c | 116 +-
> .../selftests/kvm/x86_64/sev_smoke_test.c | 67 --
> 6 files changed, 230 insertions(+), 22 deletions(-)
There is *way* too much going on in this one patch. There's at least 6+ patches
worth of stuff here:
1. Add x86 architectural defines
2. SNP ioctl() plumbing
3..N. Other misc plumbing, e.g. is_smt_active()
N+1. __vm_create() change to force GUEST_MEMFD for SNP
N+2. Whatever the ASSERT_SEV_POLICY() thing is doing, if it's actually
necessary
N+3..M. Refactorings to existing code to prep for SNP
M+1. SNP support
In general, if you feel the urge to start a changelog paragraph with "Also,"
that's a sign you need to break up the patch.
> @@ -74,8 +95,18 @@ kvm_static_assert(SEV_RET_SUCCESS == 0);
> __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm); \
> })
>
> +/* Ensure policy is within bounds for SEV, SEV-ES */
> +#define ASSERT_SEV_POLICY(type, policy) \
> +({ \
> + if (type == KVM_X86_SEV_VM || type == KVM_X86_SEV_ES_VM) { \
> + TEST_ASSERT(policy < ((uint32_t)~0U), \
> + "Policy beyond bounds for SEV");\
This is asinine. First, there's one user, i.e. I see no reason to have a funky
macro to assert on the type. Second, _if_ this is a common macro, "type" can
and
should be incorporated into the assert. Third, I have no idea why selftests is
validating its own inputs to KVM.
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> index 974bcd2df6d7..981f3c9fd1cf 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/processor.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/processor.c
> @@ -625,7 +625,8 @@ void kvm_arch_vm_post_create(struct kvm_vm *vm)
> sync_global_to_guest(vm, host_cpu_is_amd);
> sync_global_to_guest(vm, is_forced_emulation_enabled);
>
> - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
> + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
> + vm->type == KVM_X86_SNP_VM) {
Probably time to add a helper, e.g. is_sev_vm() or something. If we follow
KVM's
lead, then we'd have is_sev_vm(), is_sev_es_vm(), and is_sev_snp_vm(), where an
SNP VM returns true for all of them. Not sure I love that idea, just throwing
it
out there as one possibility.
> struct kvm_sev_init init = { 0 };
>
> vm_sev_ioctl(vm, KVM_SEV_INIT2, &init);
> @@ -1134,7 +1135,8 @@ void kvm_get_cpu_address_width(unsigned int *pa_bits,
> unsigned int *va_bits)
>
> void kvm_init_vm_address_properties(struct kvm_vm *vm)
> {
> - if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM) {
> + if (vm->type == KVM_X86_SEV_VM || vm->type == KVM_X86_SEV_ES_VM ||
> + vm->type == KVM_X86_SNP_VM) {
> vm->arch.sev_fd = open_sev_dev_path_or_exit();
> vm->arch.c_bit =
> BIT_ULL(this_cpu_property(X86_PROPERTY_SEV_C_BIT));
> vm->gpa_tag_mask = vm->arch.c_bit;
> diff --git a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> index 125a72246e09..ff3824564854 100644
> --- a/tools/testing/selftests/kvm/lib/x86_64/sev.c
> +++ b/tools/testing/selftests/kvm/lib/x86_64/sev.c
> @@ -14,7 +14,8 @@
> * and find the first range, but that's correct because the condition
> * expression would cause us to quit the loop.
> */
> -static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region
> *region)
> +static int encrypt_region(struct kvm_vm *vm, struct userspace_mem_region
> *region,
> + uint8_t page_type)
> {
> const struct sparsebit *protected_phy_pages =
> region->protected_phy_pages;
> const vm_paddr_t gpa_base = region->region.guest_phys_addr;
> @@ -25,12 +26,23 @@ static int encrypt_region(struct kvm_vm *vm, struct
> userspace_mem_region *region
> if (!sparsebit_
