Re: [PATCH kernel v2 5/5] crypto/ccp: Implement SEV-TIO PCIe IDE (phase1)

[Tom Lendacky]

On 12/1/25 20:04, Alexey Kardashevskiy wrote:
> On 2/12/25 02:23, Tom Lendacky wrote:
>> On 11/21/25 02:06, Alexey Kardashevskiy wrote:

>>> +struct sla_addr_t {
>>> +    union {
>>> +        u64 sla;
>>> +        struct {
>>> +            u64 page_type:1;
>>> +            u64 page_size:1;
>>> +            u64 reserved1:10;
>>> +            u64 pfn:40;
>>> +            u64 reserved2:12;
>>
>>     u64 page_type    :1,
>>         page_size    :1,
>>         reserved1    :10,
>>         pfn        :40,
>>         reserved2    :12;
> 
> okay for formatting but...
> 
>>
>> This makes it easier to understand. Please do this everywhere you define
>> bitfields.
> 
> ...I really want to keep the union here (do not care in other places
> though) for easier comparison of a whole structure.

Yes, the union is fine, I was only referring to how to represent the
bitfields.

> 
> 

>>> @@ -1439,8 +1446,14 @@ static int __sev_snp_init_locked(int *error,
>>> unsigned int max_snp_asid)
>>>           data.init_rmp = 1;
>>>           data.list_paddr_en = 1;
>>>           data.list_paddr = __psp_pa(snp_range_list);
>>> +
>>> +#if defined(CONFIG_PCI_TSM)
>>>           data.tio_en = sev_tio_present(sev) &&
>>> +            sev_tio_enabled && psp_init_on_probe &&
>>
>> Why add the psp_init_on_probe check here? Why is it not compatible?
>> psp_init_on_probe is for SEV and SEV-ES, not SNP.
> 
> If psp_init_on_probe is not set, then systemd (or modprobe?) loads
> kvm_amd and at that point SEV init is delayed but SNP init is not so
> SEV-TIO gets enabled.
> 
> Then, there is some systemd service in my test Ubuntu which:
> 1) runs QEMU to discover something, with SEV enabled, that trigger
> SEV_PDH_CERT_EXPORT
> 2) the kernel ioctl handler has to initialize SEV
> 3) sev_move_to_init_state() returns shutdown_required=true (it does not
> distinguish SEV and SNP)
> 4) the SEV_PDH_CERT_EXPORT handler shuts down both SEV and SNP (which
> includes SEV-TIO).

That seems like bad behavior. It should only shutdown SEV, not SNP.
> 
> The right thing to do is just not use psp_init_on_probe as it is really
> a debugging knob. But people are going to use it while DOWNLOAD_EX

It's not a debugging knob, there are customers that use it.
> (which we need this psp_init_on_probe thing for) and SEV-TIO are still
> in their infancy. It took me half a day to sort this all in my head,
> hence the check.
> 
> I will remove it from the above but leave the warning below and add the
> comment:
> 
> /*
>  * When psp_init_on_probe is disabled, the userspace calling SEV ioctl
>  * can inadvertently shut down SNP and SEV-TIO during initialization,
>  * causing unexpected state loss.
>  */

Maybe a follow-on patch can fix the behavior so that SNP isn't shutdown
if it was already initialized.

> 
> 
>> Instead of the #if, please use IS_ENABLED(CONFIG_PCI_TSM) so that the
>> #ifdefs can be eliminated from the code.
>>
>> Having all these checks in sev_tio_supported() (comment from earlier
>> patch) will simplify things.
> 
> I am open coding sev_tio_supported(), and ditching 4/5, seems pointless
> as hardly anyone will want to enable just TIO in the PSP without the
> host os support for it, right?

Ok, I'll check out the new version.

Thanks,
Tom

>