Re: [PATCH 2/6] PCI/IDE: Add Address Association Register setup for downstream MMIO

dan.j.williams
2025-11-05T23:04:10Z

Jonathan Cameron wrote:
> On Tue, 4 Nov 2025 20:00:51 -0800
> Dan Williams <dan.j.williams@intel.com> wrote:
>
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> >
> > The address ranges for downstream Address Association Registers need to
> > cover memory addresses for all functions (PFs/VFs/downstream devices)
> > managed by a Device Security Manager (DSM). The proposed solution is get
> > the memory (32-bit only) range and prefetchable-memory (64-bit capable)
> > range from the immediate ancestor downstream port (either the direct-attach
> > RP or deepest switch port when switch attached).
> >
> > Similar to RID association, address associations will be set by default if
> > hardware sets 'Number of Address Association Register Blocks' in the
> > 'Selective IDE Stream Capability Register' to a non-zero value. TSM drivers
> > can opt-out of the settings by zero'ing out unwanted / unsupported address
> > ranges. E.g. TDX Connect only supports prefetachable (64-bit capable)
> > memory ranges for the Address Association setting.
> >
> > If the immediate downstream port provides both a memory range and
> > prefetchable-memory range, but the IDE partner port only provides 1 Address
> > Association Register block then the TSM driver can pick which range to
> > associate, or let the PCI core prioritize memory.
> >
> > Note, the Address Association Register setup for upstream requests is still
> > uncertain so is not included.
> >
> > Co-developed-by: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
> > Co-developed-by: Arto Merilainen <amerilainen@nvidia.com>
> > Signed-off-by: Arto Merilainen <amerilainen@nvidia.com>
> > Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> > Co-developed-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > include/linux/pci-ide.h | 27 ++++++++++
> > include/linux/pci.h | 5 ++
> > drivers/pci/ide.c | 115 ++++++++++++++++++++++++++++++++++++----
> > 3 files changed, 138 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> > index d0f10f3c89fc..55283c8490e4 100644
> > --- a/include/linux/pci-ide.h
> > +++ b/include/linux/pci-ide.h
> > @@ -28,6 +28,9 @@ enum pci_ide_partner_select {
> > * @rid_start: Partner Port Requester ID range start
> > * @rid_end: Partner Port Requester ID range end
> > * @stream_index: Selective IDE Stream Register Block selection
> > + * @mem_assoc: PCI bus memory address association for targeting peer partner
>
> The text above about TDX only support prefetchable to me suggestions this
> is optional so should be marked so like pref_assoc?

Maybe... I think I was more considering the fact that PCI compliant
devices always have a 32-bit MMIO range. Given both are optional it
might be better to detail that in the Description section:

diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
index 40f0be185120..37a1ad9501b0 100644
--- a/include/linux/pci-ide.h
+++ b/include/linux/pci-ide.h
@@ -29,13 +29,18 @@ enum pci_ide_partner_select {
* @rid_end: Partner Port Requester ID range end
* @stream_index: Selective IDE Stream Register Block selection
* @mem_assoc: PCI bus memory address association for targeting peer partner
- * @pref_assoc: (optional) PCI bus prefetchable memory address association for
+ * @pref_assoc: PCI bus prefetchable memory address association for
* targeting peer partner
* @default_stream: Endpoint uses this stream for all upstream TLPs regardless of
* address and RID association registers
* @setup: flag to track whether to run pci_ide_stream_teardown() for this
* partner slot
* @enable: flag whether to run pci_ide_stream_disable() for this partner slot
+ *
+ * By default, pci_ide_stream_alloc() initializes @mem_assoc and @pref_assoc
+ * with the immediate ancestor downstream port memory ranges (i.e. Type 1
+ * Configuration Space Header values). Caller may zero size ({0, -1}) the range
+ * to drop it from consideration at pci_ide_stream_setup() time.
*/
struct pci_ide_partner {
u16 rid_start;

>
> > + * @pref_assoc: (optional) PCI bus prefetchable memory address association for
> > + * targeting peer partner
> > * @default_stream: Endpoint uses this stream for all upstream TLPs regardless of
> > * address and RID association registers
> > * @setup: flag to track whether to run pci_ide_stream_teardown() for this
> > @@ -38,11 +41,35 @@ struct pci_ide_partner {
> > u16 rid_start;
> > u16 rid_end;
> > u8 stream_index;
> > + struct pci_bus_region mem_assoc;
> > + struct pci_bus_region pref_assoc;
> > unsigned int default_stream:1;
> > unsigned int setup:1;
> > unsigned int enable:1;
> > };
>
>
> > diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> > index da5b1acccbb4..d7fc741f3a26 100644
> > --- a/drivers/pci/ide.c
> > +++ b/drivers/pci/ide.c
>
>
> > @@ -385,6 +408,61 @@ static void set_ide_sel_ctl(struct pci_dev *pdev, struct pci_ide *ide,
> > pci_write_config_dword(pdev, pos + PCI_IDE_SEL_CTL, val);
> > }
> >
> > +#define SEL_ADDR1_LOWER GENMASK(31, 20)
> > +#define SEL_ADDR_UPPER GENMASK_ULL(63, 32)
> > +#define PREP_PCI_IDE_SEL_ADDR1(base, limit) \
> > + (FIELD_PREP(PCI_IDE_SEL_ADDR_1_VALID, 1) | \
> > + FIELD_PREP(PCI_IDE_SEL_ADDR_1_BASE_LOW, \
> > + FIELD_GET(SEL_ADDR1_LOWER, (base))) | \
> > + FIELD_PREP(PCI_IDE_SEL_ADDR_1_LIMIT_LOW, \
> > + FIELD_GET(SEL_ADDR1_LOWER, (limit))))
>
> Whilst complex, if it is only going to get one use, I'd just put the
> complexity inline. If it's getting lots of use in later patches then
> fair enough having this macro.

Not sure if that buys much just to move this down a few lines into
mem_assoc_to_regs().

> > +static void mem_assoc_to_regs(struct pci_bus_region *region,
> > + struct pci_ide_regs *regs, int idx)
> > +{
> > + regs->addr[idx].assoc1 =
> > + PREP_PCI_IDE_SEL_ADDR1(region->start, region->end);
> > + regs->addr[idx].assoc2 = FIELD_GET(SEL_ADDR_UPPER, region->end);
> > + regs->addr[idx].assoc3 = FIELD_GET(SEL_ADDR_UPPER, region->start);
> > +}
>
> > /**
> > * pci_ide_stream_setup() - program settings to Selective IDE Stream registers
> > * @pdev: PCIe device object for either a Root Port or Endpoint Partner Port
> > @@ -398,22 +476,34 @@ static void set_ide_sel_ctl(struct pci_dev *pdev, struct pci_ide *ide,
> > void pci_ide_stream_setup(struct pci_dev *pdev, struct pci_ide *ide)
> > {
> > struct pci_ide_partner *settings = pci_ide_to_settings(pdev, ide);
> > + struct pci_ide_regs regs;
> > int pos;
> > - u32 val;
> >
> > if (!settings)
> > return;
> >
> > + pci_ide_stream_to_regs(pdev, ide, &regs);
>
> If I were being super fussy, I'd suggest doing the factor out to a structure
> + helper as a precursor patch then just add the new stuff here.
> meh. I'm not that bothered but it would slightly simply review.
>
> I'm not entirely convinced by the helper as a readability improvement
> but don't hate it.

Note that the helper has an ulterior motive. TDX Connect wants to have a
copy of the desired register settings to pass to a TSM ABI for root port
setup. That will become clearer / documented later when this helper gets
an export.