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@xxxxxxx>
[..]
> > 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.
