Re: [PATCH 2/3] PCI/IDE: Add Address Association Register setup for RP

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

Ilpo Järvinen wrote:
> On Sun, 28 Sep 2025, Xu Yilun wrote:
> 
> > Add Address Association Register setup for Root Ports.
> > 
> > The address ranges for RP side Address Association Registers should
> > cover memory addresses for all PFs/VFs/downstream devices of the DSM
> > device. A simple solution is to get the aggregated 32-bit and 64-bit
> > address ranges from directly connected downstream port (either an RP or
> > a switch port) and set into 2 Address Association Register blocks.
> > 
> > There is a case the platform doesn't require Address Association
> > Registers setup and provides no register block for RP (AMD). Will skip
> > the setup in pci_ide_stream_setup().
> > 
> > Also imaging another case where there is only one block for RP.
> > Prioritize 64-bit address ranges setup for it. No strong reason for the
> > preference until a real use case comes.
> > 
> > The Address Association Register setup for Endpoint Side is still
> > uncertain so isn't supported in this patch.
> > 
> > Take the oppotunity to export some mini helpers for Address Association
> > Registers setup. TDX Connect needs the provided aggregated address
> > ranges but will use specific firmware calls for actual setup instead of
> > pci_ide_stream_setup().
> > 
> > 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>
> > ---
> >  include/linux/pci-ide.h | 11 +++++++
> >  drivers/pci/ide.c       | 64 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 74 insertions(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/pci-ide.h b/include/linux/pci-ide.h
> > index 5adbd8b81f65..ac84fb611963 100644
> > --- a/include/linux/pci-ide.h
> > +++ b/include/linux/pci-ide.h
> > @@ -6,6 +6,15 @@
> >  #ifndef __PCI_IDE_H__
> >  #define __PCI_IDE_H__
> >  
> > +#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))))
> > +
> >  #define PREP_PCI_IDE_SEL_RID_2(base, domain)               \
> >  	(FIELD_PREP(PCI_IDE_SEL_RID_2_VALID, 1) |          \
> >  	 FIELD_PREP(PCI_IDE_SEL_RID_2_BASE, (base)) | \
> > @@ -42,6 +51,8 @@ struct pci_ide_partner {
> >  	unsigned int default_stream:1;
> >  	unsigned int setup:1;
> >  	unsigned int enable:1;
> > +	struct range mem32;
> > +	struct range mem64;
> >  };
> >  
> >  /**
> > diff --git a/drivers/pci/ide.c b/drivers/pci/ide.c
> > index 7633b8e52399..8db1163737e5 100644
> > --- a/drivers/pci/ide.c
> > +++ b/drivers/pci/ide.c
> > @@ -159,7 +159,11 @@ struct pci_ide *pci_ide_stream_alloc(struct pci_dev *pdev)
> >  	struct stream_index __stream[PCI_IDE_HB + 1];
> >  	struct pci_host_bridge *hb;
> >  	struct pci_dev *rp;
> > +	struct pci_dev *br;
> 
> Why not join with the previous line?
> 
> >  	int num_vf, rid_end;
> > +	struct range mem32 = {}, mem64 = {};
> > +	struct pci_bus_region region;
> > +	struct resource *res;
> >  
> >  	if (!pci_is_pcie(pdev))
> >  		return NULL;
> > @@ -206,6 +210,24 @@ struct pci_ide *pci_ide_stream_alloc(struct pci_dev *pdev)
> >  	else
> >  		rid_end = pci_dev_id(pdev);
> >  
> > +	br = pci_upstream_bridge(pdev);
> > +	if (!br)
> > +		return NULL;
> > +
> > +	res = &br->resource[PCI_BRIDGE_MEM_WINDOW];
> 
> pci_resource_n()
> 
> > +	if (res->flags & IORESOURCE_MEM) {
> > +		pcibios_resource_to_bus(br->bus, &region, res);
> > +		mem32.start = region.start;
> > +		mem32.end = region.end;
> > +	}
> > +
> > +	res = &br->resource[PCI_BRIDGE_PREF_MEM_WINDOW];
> 
> Ditto.
> 
> > +	if (res->flags & IORESOURCE_PREFETCH) {
> 
> While I don't know much about what's going on here, is this assuming the 
> bridge window is not disabled solely based on this flag check?

Indeed it does seem to be assumining that the flag is only set when the
resource is valid and active.

> Previously inactive bridge window flags were reset but that's no longer 
> the case after the commit 8278c6914306 ("PCI: Preserve bridge window 
> resource type flags") (currently in pci/resource)?

Thanks for the heads up. It does seem odd that both IORESOURCE_UNSET and
IORESOURCE_DISABLED are both being set and the check allows for either.
Is that assuming that other call paths not touched in that set may only
set one of those flags?

Otherwise, the change to mark the resource as zero-sized feels a better
/ more explicit protocol than checking for flags. IDE setup only cares
that any downstream MMIO get included in the stream.