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). -- 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); + 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); + 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);
