Message-ID: <de2a6758-a906-4dc0-b481-6ce73aba24b9@amd.com>
Date: Wed, 30 Oct 2024 11:35:55 -0500
From: "Pratik R. Sampat" <pratikrajesh.sampat@....com>
To: Sean Christopherson <seanjc@...gle.com>
CC: <kvm@...r.kernel.org>, <pbonzini@...hat.com>, <pgonda@...gle.com>,
	<thomas.lendacky@....com>, <michael.roth@....com>, <shuah@...nel.org>,
	<linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: 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
> case.  But I vote to omit the extra #define entirely and just open code the shift
> in the SNP_FW_VER_{MAJOR,MINOR} macros.

Sure, I'll get rid of those couple of #defines and use them directly in
the macros.

Thanks!
Pratik

> 
>  #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)

Powered by blists - more mailing lists