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

[<dan.j.williams@intel.com>]

Arto Merilainen wrote:
> On 17.7.2025 21.33, Dan Williams wrote:
> > +static void set_ide_sel_ctl(struct pci_dev *pdev, struct pci_ide *ide, int pos,
> > +			    bool enable)
> > +{
> > +	u32 val = FIELD_PREP(PCI_IDE_SEL_CTL_ID_MASK, ide->stream_id) |
> > +		  FIELD_PREP(PCI_IDE_SEL_CTL_DEFAULT, 1) |
> 
> If I recall correctly, setting the DEFAULT bit is allowed only for one 
> SEL_SID instance at a time. If we consider the root port, wouldn't this 
> prevent having multiple IDE capable devices under the same RP?

True, I'll drop this from the next version. We can circle back to this
when ATS is considered, but that is not in scope for initial enabling.

> > +		  FIELD_PREP(PCI_IDE_SEL_CTL_CFG_EN, pdev->ide_cfg) |
> > +		  FIELD_PREP(PCI_IDE_SEL_CTL_TEE_LIMITED, pdev->ide_tee_limit) |
> > +		  FIELD_PREP(PCI_IDE_SEL_CTL_EN, enable);
> > +
> > +	pci_write_config_dword(pdev, pos + PCI_IDE_SEL_CTL, val);
> > +}
> > +
> > +/**
> > + * pci_ide_stream_setup() - program settings to Selective IDE Stream registers
> > + * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> > + * @ide: registered IDE settings descriptor
> > + *
> > + * When @pdev is a PCI_EXP_TYPE_ENDPOINT then the PCI_IDE_EP partner
> > + * settings are written to @pdev's Selective IDE Stream register block,
> > + * and when @pdev is a PCI_EXP_TYPE_ROOT_PORT, the PCI_IDE_RP settings
> > + * are selected.
> > + */
> > +void pci_ide_stream_setup(struct pci_dev *pdev, struct pci_ide *ide)
> > +{
> > +	struct pci_ide_partner *settings = pci_ide_to_settings(pdev, ide);
> > +	int pos;
> > +	u32 val;
> > +
> > +	if (!settings)
> > +		return;
> > +
> > +	pos = sel_ide_offset(pdev, settings);
> > +
> > +	val = FIELD_PREP(PCI_IDE_SEL_RID_1_LIMIT_MASK, settings->rid_end);
> > +	pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_1, val);
> > +
> > +	val = FIELD_PREP(PCI_IDE_SEL_RID_2_VALID, 1) |
> > +	      FIELD_PREP(PCI_IDE_SEL_RID_2_BASE_MASK, settings->rid_start) |
> > +	      FIELD_PREP(PCI_IDE_SEL_RID_2_SEG_MASK, pci_ide_domain(pdev));
> > +
> > +	pci_write_config_dword(pdev, pos + PCI_IDE_SEL_RID_2, val);
> > +
> > +	/*
> > +	 * Setup control register early for devices that expect
> > +	 * stream_id is set during key programming.
> > +	 */
> > +	set_ide_sel_ctl(pdev, ide, pos, false);
> > +	settings->setup = 1;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_ide_stream_setup);
> 
> The first revision of this patch had address association register 
> programming but it has since been removed. Could you comment if there is 
> a reason for this change?

We chatted about it around this point in the original review thread [1].
tl;dr SEV-TIO and TDX Connect did not see a strict need for it. However,
the expectation was always to circle back and revive it if it turned out
later to be required.

[1]: http://lore.kernel.org/67bcf19bd1c7a_1c530f29449@dwillia2-xfh.jf.intel.com.notmuch

> Some background: This might be problematic for ARM CCA. I recall seeing 
> a comment stating that the address association register programming can 
> be skipped on some architectures (e.g., apparently AMD uses a separate 
> table that contains the StreamID) but on ARM CCA the StreamID 
> association AFAIK happens through these registers.

Can you confirm and perhaps work with Aneesh to propose an incremental
patch to add that support back? It might be something that we let the
low level TSM driver control. Like an additional address association
object that can be attached to 'struct pci_ide' by the low level TSM
driver.

The messy part is sparse device MMIO layout vs limited association
blocks and this is where SEV-TIO and TDX Connect have other mechanisms
to do that stream-id association.