Re: [RFC PATCH 06/21] crypto: ccp: Enable SEV-TIO feature in the PSP when supported

[Dan Williams <dan.j.williams@intel.com>]

Alexey Kardashevskiy wrote:
> 
> 
> On 4/9/24 07:27, Dan Williams wrote:
> > Alexey Kardashevskiy wrote:
> >> The PSP advertises the SEV-TIO support via the FEATURE_INFO command
> >> support of which is advertised via SNP_PLATFORM_STATUS.
> >>
> >> Add FEATURE_INFO and use it to detect the TIO support in the PSP.
> >> If present, enable TIO in the SNP_INIT_EX call.
> >>
> >> While at this, add new bits to sev_data_snp_init_ex() from SEV-SNP 1.55.
> >>
> >> Note that this tests the PSP firmware support but not if the feature
> >> is enabled in the BIOS.
> >>
> >> While at this, add new sev_data_snp_shutdown_ex::x86_snp_shutdown
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
[..]
> > Why use CPU register names in C structures? I would hope the spec
> > renames these parameters to something meaninful?
> 
> This mimics the CPUID instruction and (my guess) x86 people are used to 
> "CPUID's ECX" == "Subfunction index". The spec (the one I mention below) 
> calls it precisely "ECX_IN".

Oh, I never would have guessed that "snp feature info" mimicked CPUID,
but then again, no one has ever accused me of being an "x86 people".

> >>   /**
> >>    * struct sev_data_snp_launch_start - SNP_LAUNCH_START command params
> >>    *
> >> @@ -745,10 +765,14 @@ struct sev_data_snp_guest_request {
> > 
> > Would be nice to have direct pointer to the spec and spec chapter
> > documented for these command structure fields.
> 
> For every command? Seems overkill. Any good example?
> 
> Although the file could have mentioned in the header that SNP_xxx are 
> from "SEV Secure Nested Paging Firmware ABI Specification" which google 
> easily finds, and search on that pdf for "SNP_INIT_EX" finds the 
> structure layout. Using the exact chapter numbers/titles means they 
> cannot change, or someone has to track the changes.

No need to go overboard, but you can grep for:
    "PCIe\ r\[0-9\]"
...or:
    "CXL\ \[12\]" 

...for some examples. Yes, these references can bit rot, but that can
also be good information "the last time this definition was touched was
in vN and vN+1 introduced some changes."

[..]
> >> +static int snp_get_feature_info(struct sev_device *sev, u32 ecx, struct sev_snp_feature_info *fi)
> > 
> > Why not make this bool...
> > 
> >> +{
> >> +	struct sev_user_data_snp_status status = { 0 };
> >> +	int psp_ret = 0, ret;
> >> +
> >> +	ret = snp_platform_status_locked(sev, &status, &psp_ret);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (ret != SEV_RET_SUCCESS)
> >> +		return -EFAULT;
> >> +	if (!status.feature_info)
> >> +		return -ENOENT;
> >> +
> >> +	ret = snp_feature_info_locked(sev, ecx, fi, &psp_ret);
> >> +	if (ret)
> >> +		return ret;
> >> +	if (ret != SEV_RET_SUCCESS)
> >> +		return -EFAULT;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static bool sev_tio_present(struct sev_device *sev)
> >> +{
> >> +	struct sev_snp_feature_info fi = { 0 };
> >> +	bool present;
> >> +
> >> +	if (snp_get_feature_info(sev, 0, &fi))
> > 
> > ...since the caller does not care?
> 
> sev_tio_present() does not but other users of snp_get_feature_info() 
> (one is coming sooner that TIO) might, WIP.

...not a huge deal, but it definitely looked odd to see so much care to
return distinct error codes only to throw away the distinction.