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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <745851ab-7776-4875-a57c-978fb10239d9@amd.com>
Date: Tue, 2 Dec 2025 08:24:16 -0600
From: Tom Lendacky <thomas.lendacky@....com>
To: Alexey Kardashevskiy <aik@....com>, linux-kernel@...r.kernel.org
Cc: linux-crypto@...r.kernel.org, John Allen <john.allen@....com>,
 Herbert Xu <herbert@...dor.apana.org.au>,
 "David S. Miller" <davem@...emloft.net>, Ashish Kalra
 <ashish.kalra@....com>, Joerg Roedel <joro@...tes.org>,
 Suravee Suthikulpanit <suravee.suthikulpanit@....com>,
 Will Deacon <will@...nel.org>, Robin Murphy <robin.murphy@....com>,
 "Borislav Petkov (AMD)" <bp@...en8.de>, Kim Phillips <kim.phillips@....com>,
 Jerry Snitselaar <jsnitsel@...hat.com>, Vasant Hegde <vasant.hegde@....com>,
 Jason Gunthorpe <jgg@...pe.ca>, Gao Shiyuan <gaoshiyuan@...du.com>,
 Sean Christopherson <seanjc@...gle.com>, Nikunj A Dadhania <nikunj@....com>,
 Michael Roth <michael.roth@....com>, Amit Shah <amit.shah@....com>,
 Peter Gonda <pgonda@...gle.com>, iommu@...ts.linux.dev
Subject: Re: [PATCH kernel v2 5/5] crypto/ccp: Implement SEV-TIO PCIe IDE
 (phase1)

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

>