On 6/9/25 12:00, dan.j.williams@xxxxxxxxx wrote:
Alexey Kardashevskiy wrote:
[..]
Ah this is an actual problem, this is not right. The PCIe r6.1 spec says:
"It is permitted, but strongly not recommended, to Set the Enable bit in the IDE Extended Capability
entry for a Stream prior to the completion of key programming for that Stream".
This ordering is controlled by the TSM driver though...
yes so pci_ide_stream_enable() should just do what it was asked -
enable the bit, the PCIe spec says the stream does not have to go to
the secure state right away.
That is reasonable, I will leave the error detection to the low-level
TSM driver.
The helper should still return an (unique) error though as this path is "not recommended" and the TSM driver might want to print a warning if it is not for a known device which does "not recommended" thing.
And I have a device like that where the links goes secure after the last
key is SET_GO. So it is okay to return an error here but not ok to clear
the Enabled bit.
...can you not simply wait to call pci_ide_stream_enable() until after the
SET_GO?
Nope, if they keys are programmed without the enabled bit set, the
stream never goes secure on this device.
The way to think about it (an AMD hw engineer told me): devices do not
have extra memory to store all these keys before deciding which stream
they are for, they really (really) want to write the keys to the PHYs
(or whatever that hardware piece is called) as they come. And after
the device reset, say, both link stream #0 and selective stream #0
have the same streamid=0.
Ah, ok.
Now, the devices need to know which stream it is - link or selective.
One way is: enable a stream beforehand and then the device will store
keys in that streams's slot. The other way is: wait till SET_GO but
before that every stream on the device needs an unique stream id
assigned to it.
I even have this in my tree (to fight another device):
https://github.com/AMDESE/linux-kvm/commit/ddd1f401665a4f0b6036330eea6662aec566986b
I recall we talked about this before, not liking the lack of tracking of
these placeholder ids which would need to be adjusted later, and not
understanding the need for uniqueness of idle ids.
It is also actively destructive to platform-firmware established IDE
which is possible on Intel platforms and part of the specification of
CXL TSP.
What about something like this (but I think it should be an incremental
patch that details this class of hardware problem that requires system
software to manage idle ids).
Yes, this one is better. Thanks,
--
Alexey
