[PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge

Alexey Kardashevskiy posted 2 patches 2 weeks, 2 days ago
[PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
Posted by Alexey Kardashevskiy 2 weeks, 2 days ago
IDE stream IDs are responsibility of a platform and in some cases TSM
allocates the numbers. AMD SEV TIO though leaves it to the host OS.
Mistakenly stream ID is hard coded to be the same as a traffic class.

Use the host bridge stream index for a newly allocated stream ID.

Fixes: 4be423572da1 ("crypto/ccp: Implement SEV-TIO PCIe IDE (phase1)")
Signed-off-by: Alexey Kardashevskiy <aik@amd.com>
---
 drivers/crypto/ccp/sev-dev-tsm.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/sev-dev-tsm.c b/drivers/crypto/ccp/sev-dev-tsm.c
index 7407b77c2ef2..40d02adaf3f6 100644
--- a/drivers/crypto/ccp/sev-dev-tsm.c
+++ b/drivers/crypto/ccp/sev-dev-tsm.c
@@ -198,8 +198,7 @@ static int stream_alloc(struct pci_dev *pdev, struct pci_ide **ide,
 	if (!ide1)
 		return -EFAULT;
 
-	/* Blindly assign streamid=0 to TC=0, and so on */
-	ide1->stream_id = tc;
+	ide1->stream_id = ide1->host_bridge_stream;
 
 	ide[tc] = ide1;
 
-- 
2.52.0
Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
Posted by dan.j.williams@intel.com 2 weeks, 1 day ago
Alexey Kardashevskiy wrote:
> IDE stream IDs are responsibility of a platform and in some cases TSM
> allocates the numbers. AMD SEV TIO though leaves it to the host OS.
> Mistakenly stream ID is hard coded to be the same as a traffic class.

I scratched my head at this comment, but now realize that you are saying
the existing code used the local @tc, not that the hardware stream ID is
in any way related to traffic class, right?

It would help to detail what the end user visible effects of this bug
are. The TSM framework does not allow for multiple streams per PF, so I
wonder what scenario is being fixed?

Lastly, are you expecting tsm.git#fixes to pick this up? I am assuming
that this goes through crypto.git and tsm.git can just stay focused on
core fixes.
Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
Posted by Alexey Kardashevskiy 1 week, 6 days ago

On 24/1/26 09:59, dan.j.williams@intel.com wrote:
> Alexey Kardashevskiy wrote:
>> IDE stream IDs are responsibility of a platform and in some cases TSM
>> allocates the numbers. AMD SEV TIO though leaves it to the host OS.
>> Mistakenly stream ID is hard coded to be the same as a traffic class.
> 
> I scratched my head at this comment, but now realize that you are saying
> the existing code used the local @tc, not that the hardware stream ID is
> in any way related to traffic class, right?

When I did that in the first place, I also wanted to try different traffic classes so I just took a shortcut here.

> It would help to detail what the end user visible effects of this bug
> are. The TSM framework does not allow for multiple streams per PF, so I
> wonder what scenario is being fixed?

There is no way in the current upstream code to specify this TC so the only visible effect is that 2 devices under the same bridge can work now, previously the second device would fail to allocate a stream.

> Lastly, are you expecting tsm.git#fixes to pick this up? I am assuming
> that this goes through crypto.git and tsm.git can just stay focused on
> core fixes.

I was kinda hoping that Tom acks these (as he did) and you could take them. Thanks,

-- 
Alexey
Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
Posted by dan.j.williams@intel.com 1 week, 5 days ago
Alexey Kardashevskiy wrote:
> On 24/1/26 09:59, dan.j.williams@intel.com wrote:
> > Alexey Kardashevskiy wrote:
> >> IDE stream IDs are responsibility of a platform and in some cases
> >> TSM allocates the numbers. AMD SEV TIO though leaves it to the host
> >> OS.  Mistakenly stream ID is hard coded to be the same as a traffic
> >> class.
> > 
> > I scratched my head at this comment, but now realize that you are
> > saying the existing code used the local @tc, not that the hardware
> > stream ID is in any way related to traffic class, right?
> 
> When I did that in the first place, I also wanted to try different
> traffic classes so I just took a shortcut here.
> 
> > It would help to detail what the end user visible effects of this
> > bug are. The TSM framework does not allow for multiple streams per
> > PF, so I wonder what scenario is being fixed?
> 
> There is no way in the current upstream code to specify this TC so the
> only visible effect is that 2 devices under the same bridge can work
> now, previously the second device would fail to allocate a stream.
> 
> > Lastly, are you expecting tsm.git#fixes to pick this up? I am
> > assuming that this goes through crypto.git and tsm.git can just stay
> > focused on core fixes.
> 
> I was kinda hoping that Tom acks these (as he did) and you could take
> them. Thanks,

Ok, so can you refresh the changelog to call out the user visible
effects?  Something like:

---
With SEV-TIO the low-level TSM driver is responsible for allocating a
Stream ID. The Stream ID needs to be unique within each IDE partner
port. Fix the Stream ID selection to reuse the host bridge stream
resource id which is a pool of 256 ids per host bridge on AMD platforms.
Otherwise, only one device per-host bridge can establish Selective
Stream IDE.
---

Send a v2, and I will pick it up.
Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
Posted by Alexey Kardashevskiy 1 week, 2 days ago

On 27/1/26 17:59, dan.j.williams@intel.com wrote:
> Alexey Kardashevskiy wrote:
>> On 24/1/26 09:59, dan.j.williams@intel.com wrote:
>>> Alexey Kardashevskiy wrote:
>>>> IDE stream IDs are responsibility of a platform and in some cases
>>>> TSM allocates the numbers. AMD SEV TIO though leaves it to the host
>>>> OS.  Mistakenly stream ID is hard coded to be the same as a traffic
>>>> class.
>>>
>>> I scratched my head at this comment, but now realize that you are
>>> saying the existing code used the local @tc, not that the hardware
>>> stream ID is in any way related to traffic class, right?
>>
>> When I did that in the first place, I also wanted to try different
>> traffic classes so I just took a shortcut here.
>>
>>> It would help to detail what the end user visible effects of this
>>> bug are. The TSM framework does not allow for multiple streams per
>>> PF, so I wonder what scenario is being fixed?
>>
>> There is no way in the current upstream code to specify this TC so the
>> only visible effect is that 2 devices under the same bridge can work
>> now, previously the second device would fail to allocate a stream.
>>
>>> Lastly, are you expecting tsm.git#fixes to pick this up? I am
>>> assuming that this goes through crypto.git and tsm.git can just stay
>>> focused on core fixes.
>>
>> I was kinda hoping that Tom acks these (as he did) and you could take
>> them. Thanks,
> 
> Ok, so can you refresh the changelog to call out the user visible
> effects?  Something like:
> 
> ---
> With SEV-TIO the low-level TSM driver is responsible for allocating a
> Stream ID. The Stream ID needs to be unique within each IDE partner
> port. Fix the Stream ID selection to reuse the host bridge stream
> resource id which is a pool of 256 ids per host bridge on AMD platforms.
> Otherwise, only one device per-host bridge can establish Selective
> Stream IDE.
> ---
> 
> Send a v2, and I will pick it up.

Please squash it in the v1, if possible.

Acked-by: Alexey Kardashevskiy <aik@amd.com>

thanks!

ps sorry missed that on time, I do suck at multitasking :(


-- 
Alexey