Re: [PATCH v5 07/10] PCI/IDE: Add IDE establishment helpers

[Alexey Kardashevskiy <aik@amd.com>]

On 6/9/25 12:00, dan.j.williams@intel.com 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.
> 
>>>> 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).

(I know it is not a real patch but I gave it a try anyway)

> 
> -- 8< --
> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
> index 0183ca6f6954..2dd90c0703e0 100644
> --- a/drivers/pci/Kconfig
> +++ b/drivers/pci/Kconfig
> @@ -125,17 +125,6 @@ config PCI_ATS
>   config PCI_IDE
>   	bool
>   
> -config PCI_IDE_STREAM_MAX
> -	int "Maximum number of Selective IDE Streams supported per host bridge" if EXPERT
> -	depends on PCI_IDE
> -	range 1 256
> -	default 64
> -	help
> -	  Set a kernel max for the number of IDE streams the PCI core supports
> -	  per device. While the PCI specification max is 256, the hardware
> -	  platform capability for the foreseeable future is 4 to 8 streams. Bump
> -	  this value up if you have an expert testing need.
> -
>   config PCI_TSM
>   	bool "PCI TSM: Device security protocol support"
>   	select PCI_IDE
> diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> index 610603865d9e..e8a9c5fd8a36 100644
> --- a/drivers/pci/ide.c
> +++ b/drivers/pci/ide.c
> @@ -36,7 +36,7 @@ static int sel_ide_offset(struct pci_dev *pdev,
>   
>   void pci_ide_init(struct pci_dev *pdev)
>   {
> -	u8 nr_link_ide, nr_ide_mem, nr_streams;
> +	u8 nr_link_ide, nr_ide_mem, nr_streams, reserved_id;
>   	u16 ide_cap;
>   	u32 val;
>   
> @@ -74,14 +74,13 @@ void pci_ide_init(struct pci_dev *pdev)
>   		nr_link_ide = 0;
>   
>   	nr_ide_mem = 0;
> -	nr_streams = min(1 + FIELD_GET(PCI_IDE_CAP_SEL_NUM, val),
> -			 CONFIG_PCI_IDE_STREAM_MAX);
> +	nr_streams = 1 + FIELD_GET(PCI_IDE_CAP_SEL_NUM, val);
>   	for (u8 i = 0; i < nr_streams; i++) {
>   		int pos = __sel_ide_offset(ide_cap, nr_link_ide, i, nr_ide_mem);
>   		int nr_assoc;
>   		u32 val;
>   
> -		pci_read_config_dword(pdev, pos, &val);
> +		pci_read_config_dword(pdev, pos + PCI_IDE_SEL_CAP, &val);
>   
>   		/*
>   		 * Let's not entertain streams that do not have a
> @@ -95,7 +94,65 @@ void pci_ide_init(struct pci_dev *pdev)
>   		}
>   
>   		nr_ide_mem = nr_assoc;
> +
> +		/* Reserve stream-ids that are already active on the device */
> +		pci_read_config_dword(pdev, pos + PCI_IDE_SEL_CAP, &val);


PCI_IDE_SEL_CTL


> +		if (val & PCI_IDE_SEL_CTL_EN) {
> +			u8 id = FIELD_GET(PCI_IDE_SEL_CTL_ID, val);
> +
> +			pci_info(pdev, "Selective Stream %d id: %d active at init\n", i, id);
> +			set_bit(id, pdev->ide_stream_map);
> +		}
> +	}
> +
> +	/* Reserve link stream-ids that are already active on the device */
> +	for (int i = 0; i < nr_link_ide; ++i) {
> +		int pos = ide_cap + PCI_IDE_LINK_STREAM_0 + i * PCI_IDE_LINK_BLOCK_SIZE;
> +
> +		pci_read_config_dword(pdev, pos, &val);
> +		if (val & PCI_IDE_LINK_CTL_EN) {
> +			u8 id = FIELD_GET(PCI_IDE_LINK_CTL_ID, val);
> +
> +			pci_info(pdev, "Link Stream %d id: %d active at init\n",
> +				 i, id);
> +			set_bit(id, pdev->ide_stream_map);
> +		}
> +	}
> +
> +	/*
> +	 * Now that in use ids are known, grab and assign a free id for idle
> +	 * streams to remove ambiguity of which key slot is being activated by a
> +	 * K_SET_GO event (PCIe r7.0 section 6.33.3 IDE Key Management (IDE_KM))
> +	 */
> +	reserved_id = find_first_zero_bit(pdev->ide_stream_map, U8_MAX);


pci_ide_init() is called when PCI is probing and at that point no stream is enabled so reserved_id ends up as 0.

Since my platform only wants 1 stream, should I have called pci_ide_init_nr_streams() with "2", not "1"?

pci_ide_stream_alloc() fails to find a stream - fails in:

  struct stream_index *ep_stream __free(free_stream) = alloc_stream_index(
          pdev->ide_stream_map, pdev->nr_sel_ide, &__stream[PCI_IDE_EP]);

as nr_sel_ide==1 (my EP has just one stream). If I go with reserved_id==0 and set ide->stream_id=1 in the PSP TSM driver, then streamindex#1 gets programmed (which is beyond the end of IDE cap) with streamid=0. As if reserved_id is actually reserved_index but the device wants me to reserve an ID.

We could simplify it by assigning something like "255" to all unused+disabled streams. Thanks,


> +	if (reserved_id == U8_MAX) {
> +		pci_info(pdev, "No available Stream IDs, disable IDE\n");
> +		return;
> +	}
> +
> +	for (u8 i = 0; i < nr_streams; i++) {
> +		int pos = __sel_ide_offset(ide_cap, nr_link_ide, i, nr_ide_mem);
> +
> +		pci_read_config_dword(pdev, pos + PCI_IDE_SEL_CAP, &val);
> +		if (val & PCI_IDE_SEL_CTL_EN)
> +			continue;
> +		val &= ~PCI_IDE_SEL_CTL_ID;
> +		val |= FIELD_PREP(PCI_IDE_SEL_CTL_ID, reserved_id);
> +		pci_write_config_dword(pdev, pos + PCI_IDE_SEL_CTL, val);
> +	}
> +
> +	for (int i = 0; i < nr_link_ide; ++i) {
> +		int pos = ide_cap + PCI_IDE_LINK_STREAM_0 +
> +			  i * PCI_IDE_LINK_BLOCK_SIZE;
> +
> +		pci_read_config_dword(pdev, pos, &val);
> +		if (val & PCI_IDE_LINK_CTL_EN)
> +			continue;
> +		val &= ~PCI_IDE_LINK_CTL_ID;
> +		val |= FIELD_PREP(PCI_IDE_LINK_CTL_ID, reserved_id);
> +		pci_write_config_dword(pdev, pos, val);
>   	}
> +	set_bit(reserved_id, pdev->ide_stream_map);
>   
>   	pdev->ide_cap = ide_cap;
>   	pdev->nr_link_ide = nr_link_ide;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 45360ba87538..6d16278e2d94 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -545,7 +545,7 @@ struct pci_dev {
>   	u8		nr_ide_mem;	/* Address association resources for streams */
>   	u8		nr_link_ide;	/* Link Stream count (Selective Stream offset) */
>   	u8		nr_sel_ide;	/* Selective Stream count (register block allocator) */
> -	DECLARE_BITMAP(ide_stream_map, CONFIG_PCI_IDE_STREAM_MAX);
> +	DECLARE_BITMAP(ide_stream_map, U8_MAX);
>   	unsigned int	ide_cfg:1;	/* Config cycles over IDE */
>   	unsigned int	ide_tee_limit:1; /* Disallow T=0 traffic over IDE */
>   #endif
> @@ -617,7 +617,7 @@ struct pci_host_bridge {
>   	struct list_head dma_ranges;	/* dma ranges resource list */
>   #ifdef CONFIG_PCI_IDE
>   	u8 nr_ide_streams; /* Max streams possibly active in @ide_stream_map */
> -	DECLARE_BITMAP(ide_stream_map, CONFIG_PCI_IDE_STREAM_MAX);
> +	DECLARE_BITMAP(ide_stream_map, U8_MAX);
>   #endif
>   	u8 (*swizzle_irq)(struct pci_dev *, u8 *); /* Platform IRQ swizzler */
>   	int (*map_irq)(const struct pci_dev *, u8, u8);

-- 
Alexey