Re: [PATCH] PCI: Enable io space 1k granularity for intel cpu root port

Re: [PATCH] PCI: Enable io space 1k granularity for intel cpu root port

[kernel test robot]

Hi Zhou,

kernel test robot noticed the following build errors:

[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus linus/master v6.10-rc5 next-20240625]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zhou-Shengqing/PCI-Enable-io-space-1k-granularity-for-intel-cpu-root-port/20240625-161818
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link:    https://lore.kernel.org/r/20240621020608.28964-1-zhoushengqing%40ttyinfo.com
patch subject: [PATCH] PCI: Enable io space 1k granularity for intel cpu root port
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240626/202406261735.9Fu2z2ic-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240626/202406261735.9Fu2z2ic-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406261735.9Fu2z2ic-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
     484 |                         list_for_each_entry(dev, &bridge->bus->devices, bus_list) {
         |                                             ^
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
>> drivers/pci/probe.c:484:24: error: use of undeclared identifier 'dev'
   drivers/pci/probe.c:485:26: error: use of undeclared identifier 'dev'
     485 |                                 pci_read_config_word(dev, PCI_VENDOR_ID, &ven_id);
         |                                                      ^
   drivers/pci/probe.c:486:26: error: use of undeclared identifier 'dev'
     486 |                                 pci_read_config_word(dev, PCI_DEVICE_ID, &dev_id);
         |                                                      ^
   drivers/pci/probe.c:489:27: error: use of undeclared identifier 'dev'
     489 |                                         pci_read_config_word(dev, 0x1c0, &en1k);
         |                                                              ^
   12 errors generated.


vim +/dev +484 drivers/pci/probe.c

   458	
   459	static void pci_read_bridge_windows(struct pci_dev *bridge)
   460	{
   461		u32 buses;
   462		u16 io;
   463		u32 pmem, tmp;
   464		u16 ven_id, dev_id;
   465		u16 en1k = 0;
   466		struct resource res;
   467	
   468		pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);
   469		res.flags = IORESOURCE_BUS;
   470		res.start = (buses >> 8) & 0xff;
   471		res.end = (buses >> 16) & 0xff;
   472		pci_info(bridge, "PCI bridge to %pR%s\n", &res,
   473			 bridge->transparent ? " (subtractive decode)" : "");
   474	
   475		pci_read_config_word(bridge, PCI_IO_BASE, &io);
   476		if (!io) {
   477			pci_write_config_word(bridge, PCI_IO_BASE, 0xe0f0);
   478			pci_read_config_word(bridge, PCI_IO_BASE, &io);
   479			pci_write_config_word(bridge, PCI_IO_BASE, 0x0);
   480		}
   481		if (io) {
   482			bridge->io_window = 1;
   483			if (pci_is_root_bus(bridge->bus)) {
 > 484				list_for_each_entry(dev, &bridge->bus->devices, bus_list) {
   485					pci_read_config_word(dev, PCI_VENDOR_ID, &ven_id);
   486					pci_read_config_word(dev, PCI_DEVICE_ID, &dev_id);
   487					if (ven_id == PCI_VENDOR_ID_INTEL && dev_id == 0x09a2) {
   488						/*IIO MISC Control offset 0x1c0*/
   489						pci_read_config_word(dev, 0x1c0, &en1k);
   490					}
   491				}
   492			/*
   493			 *Intel ICX SPR EMR GNR
   494			 *IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
   495			 *bit 2:Enable 1K (EN1K)
   496			 *This bit when set, enables 1K granularity for I/O space decode
   497			 *in each of the virtual P2P bridges
   498			 *corresponding to root ports, and DMI ports.
   499			 */
   500			if (en1k & 0x4)
   501				bridge->io_window_1k = 1;
   502			}
   503			pci_read_bridge_io(bridge, &res, true);
   504		}
   505	
   506		pci_read_bridge_mmio(bridge, &res, true);
   507	
   508		/*
   509		 * DECchip 21050 pass 2 errata: the bridge may miss an address
   510		 * disconnect boundary by one PCI data phase.  Workaround: do not
   511		 * use prefetching on this device.
   512		 */
   513		if (bridge->vendor == PCI_VENDOR_ID_DEC && bridge->device == 0x0001)
   514			return;
   515	
   516		pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &pmem);
   517		if (!pmem) {
   518			pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE,
   519						       0xffe0fff0);
   520			pci_read_config_dword(bridge, PCI_PREF_MEMORY_BASE, &pmem);
   521			pci_write_config_dword(bridge, PCI_PREF_MEMORY_BASE, 0x0);
   522		}
   523		if (!pmem)
   524			return;
   525	
   526		bridge->pref_window = 1;
   527	
   528		if ((pmem & PCI_PREF_RANGE_TYPE_MASK) == PCI_PREF_RANGE_TYPE_64) {
   529	
   530			/*
   531			 * Bridge claims to have a 64-bit prefetchable memory
   532			 * window; verify that the upper bits are actually
   533			 * writable.
   534			 */
   535			pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32, &pmem);
   536			pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32,
   537					       0xffffffff);
   538			pci_read_config_dword(bridge, PCI_PREF_BASE_UPPER32, &tmp);
   539			pci_write_config_dword(bridge, PCI_PREF_BASE_UPPER32, pmem);
   540			if (tmp)
   541				bridge->pref_64_window = 1;
   542		}
   543	
   544		pci_read_bridge_mmio_pref(bridge, &res, true);
   545	}
   546	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v2] [PATCH v2] PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

Fix compilation error in drivers/pci/probe.c with commit
1f22711375ebe9fc95ced26c7059621c4d59b437

Changelog:
v2:
 - Fixed compilation error(missed changed).

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202406261735.9Fu2z2ic-lkp@intel.com/

Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
---
 drivers/pci/probe.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3f0c901c6653..909962795311 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -463,6 +463,7 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
 	u32 pmem, tmp;
 	u16 ven_id, dev_id;
 	u16 en1k = 0;
+	struct pci_dev *dev = NULL;
 	struct resource res;
 
 	pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);

base-commit: 1f22711375ebe9fc95ced26c7059621c4d59b437
-- 
2.39.2

Re: [PATCH v2] [PATCH v2] PCI: Enable io space 1k granularity for intel cpu root port

[Bjorn Helgaas]

On Wed, Jun 26, 2024 at 11:19:47AM +0000, Zhou Shengqing wrote:
> Fix compilation error in drivers/pci/probe.c with commit
> 1f22711375ebe9fc95ced26c7059621c4d59b437
> 
> Changelog:
> v2:
>  - Fixed compilation error(missed changed).

Please post a v3 that is the entire patch including the fix.  Nothing
has been merged yet, so there's no point in making it a separate
commit.

> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202406261735.9Fu2z2ic-lkp@intel.com/
> 
> Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
> ---
>  drivers/pci/probe.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3f0c901c6653..909962795311 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -463,6 +463,7 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
>  	u32 pmem, tmp;
>  	u16 ven_id, dev_id;
>  	u16 en1k = 0;
> +	struct pci_dev *dev = NULL;
>  	struct resource res;
>  
>  	pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);
> 
> base-commit: 1f22711375ebe9fc95ced26c7059621c4d59b437
> -- 
> 2.39.2
> 

[PATCH v3] PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

This patch add 1k granularity for intel root port bridge.Intel latest
server CPU support 1K granularity,And there is an BIOS setup item named
"EN1K",but linux doesn't support it. if an IIO has 5 IOU (SPR has 5 IOUs)
all are bifurcated 2x8.In a 2P server system,There are 20 P2P bridges
present.if keep 4K granularity allocation,it need 20*4=80k io space,
exceeding 64k.I test it in a 16*nvidia 4090s system under intel eaglestrem
platform.There are six 4090s that cannot be allocated I/O resources.
So I applied this patch.And I found a similar implementation in quirks.c,
but it only targets the Intel P64H2 platform.

Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
---
 drivers/pci/probe.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 5fbabb4e3425..909962795311 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -461,6 +461,9 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
 	u32 buses;
 	u16 io;
 	u32 pmem, tmp;
+	u16 ven_id, dev_id;
+	u16 en1k = 0;
+	struct pci_dev *dev = NULL;
 	struct resource res;
 
 	pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);
@@ -478,6 +481,26 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
 	}
 	if (io) {
 		bridge->io_window = 1;
+		if (pci_is_root_bus(bridge->bus)) {
+			list_for_each_entry(dev, &bridge->bus->devices, bus_list) {
+				pci_read_config_word(dev, PCI_VENDOR_ID, &ven_id);
+				pci_read_config_word(dev, PCI_DEVICE_ID, &dev_id);
+				if (ven_id == PCI_VENDOR_ID_INTEL && dev_id == 0x09a2) {
+					/*IIO MISC Control offset 0x1c0*/
+					pci_read_config_word(dev, 0x1c0, &en1k);
+				}
+			}
+		/*
+		 *Intel ICX SPR EMR GNR
+		 *IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
+		 *bit 2:Enable 1K (EN1K)
+		 *This bit when set, enables 1K granularity for I/O space decode
+		 *in each of the virtual P2P bridges
+		 *corresponding to root ports, and DMI ports.
+		 */
+		if (en1k & 0x4)
+			bridge->io_window_1k = 1;
+		}
 		pci_read_bridge_io(bridge, &res, true);
 	}
 
-- 
2.39.2

Re: [PATCH v3] PCI: Enable io space 1k granularity for intel cpu root port

[Bjorn Helgaas]

On Thu, Jun 27, 2024 at 12:58:56AM +0000, Zhou Shengqing wrote:
> This patch add 1k granularity for intel root port bridge.Intel latest
> server CPU support 1K granularity,And there is an BIOS setup item named
> "EN1K",but linux doesn't support it. if an IIO has 5 IOU (SPR has 5 IOUs)
> all are bifurcated 2x8.In a 2P server system,There are 20 P2P bridges
> present.if keep 4K granularity allocation,it need 20*4=80k io space,
> exceeding 64k.I test it in a 16*nvidia 4090s system under intel eaglestrem
> platform.There are six 4090s that cannot be allocated I/O resources.
> So I applied this patch.And I found a similar implementation in quirks.c,
> but it only targets the Intel P64H2 platform.
> 
> Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
> ---
>  drivers/pci/probe.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 5fbabb4e3425..909962795311 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -461,6 +461,9 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
>  	u32 buses;
>  	u16 io;
>  	u32 pmem, tmp;
> +	u16 ven_id, dev_id;
> +	u16 en1k = 0;
> +	struct pci_dev *dev = NULL;
>  	struct resource res;
>  
>  	pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);
> @@ -478,6 +481,26 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
>  	}
>  	if (io) {
>  		bridge->io_window = 1;
> +		if (pci_is_root_bus(bridge->bus)) {
> +			list_for_each_entry(dev, &bridge->bus->devices, bus_list) {
> +				pci_read_config_word(dev, PCI_VENDOR_ID, &ven_id);
> +				pci_read_config_word(dev, PCI_DEVICE_ID, &dev_id);
> +				if (ven_id == PCI_VENDOR_ID_INTEL && dev_id == 0x09a2) {
> +					/*IIO MISC Control offset 0x1c0*/
> +					pci_read_config_word(dev, 0x1c0, &en1k);
> +				}
> +			}
> +		/*
> +		 *Intel ICX SPR EMR GNR
> +		 *IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
> +		 *bit 2:Enable 1K (EN1K)
> +		 *This bit when set, enables 1K granularity for I/O space decode
> +		 *in each of the virtual P2P bridges
> +		 *corresponding to root ports, and DMI ports.
> +		 */
> +		if (en1k & 0x4)
> +			bridge->io_window_1k = 1;
> +		}

I still think this is not going to work because I don't want this kind
of device-specific clutter in this generic path. The pcibios_*
interfaces are history that we'd like to get rid of also, but it would
be better than putting it here.

Please follow english conventions as much as you can, e.g., space
after "*" in comments, space after period at end of sentence,
capitalize first word of sentence, blank line between paragraphs.
Most of this you can see by looking at other comments.

>  		pci_read_bridge_io(bridge, &res, true);
>  	}
>  
> -- 
> 2.39.2
> 

Re: Re: [PATCH] PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

> On Thu, Jun 27, 2024 at 12:58:56AM +0000, Zhou Shengqing wrote:
> > This patch add 1k granularity for intel root port bridge.Intel latest
> > server CPU support 1K granularity,And there is an BIOS setup item named
> > "EN1K",but linux doesn't support it. if an IIO has 5 IOU (SPR has 5 IOUs)
> > all are bifurcated 2x8.In a 2P server system,There are 20 P2P bridges
> > present.if keep 4K granularity allocation,it need 20*4=80k io space,
> > exceeding 64k.I test it in a 16*nvidia 4090s system under intel eaglestrem
> > platform.There are six 4090s that cannot be allocated I/O resources.
> > So I applied this patch.And I found a similar implementation in quirks.c,
> > but it only targets the Intel P64H2 platform.
> > 
> > Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
> > ---
> >  drivers/pci/probe.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 5fbabb4e3425..909962795311 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -461,6 +461,9 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
> >  	u32 buses;
> >  	u16 io;
> >  	u32 pmem, tmp;
> > +	u16 ven_id, dev_id;
> > +	u16 en1k = 0;
> > +	struct pci_dev *dev = NULL;
> >  	struct resource res;
> >  
> >  	pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);
> > @@ -478,6 +481,26 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
> >  	}
> >  	if (io) {
> >  		bridge->io_window = 1;
> > +		if (pci_is_root_bus(bridge->bus)) {
> > +			list_for_each_entry(dev, &bridge->bus->devices, bus_list) {
> > +				pci_read_config_word(dev, PCI_VENDOR_ID, &ven_id);
> > +				pci_read_config_word(dev, PCI_DEVICE_ID, &dev_id);
> > +				if (ven_id == PCI_VENDOR_ID_INTEL && dev_id == 0x09a2) {
> > +					/*IIO MISC Control offset 0x1c0*/
> > +					pci_read_config_word(dev, 0x1c0, &en1k);
> > +				}
> > +			}
> > +		/*
> > +		 *Intel ICX SPR EMR GNR
> > +		 *IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
> > +		 *bit 2:Enable 1K (EN1K)
> > +		 *This bit when set, enables 1K granularity for I/O space decode
> > +		 *in each of the virtual P2P bridges
> > +		 *corresponding to root ports, and DMI ports.
> > +		 */
> > +		if (en1k & 0x4)
> > +			bridge->io_window_1k = 1;
> > +		}
> 
> I still think this is not going to work because I don't want this kind
> of device-specific clutter in this generic path. The pcibios_*
> interfaces are history that we'd like to get rid of also, but it would
> be better than putting it here.

Do you think it should be putted to the pci_bios* interface?

And if there is no suitable place to apply this patch, 
then let's just ignore this issue.

> 
> Please follow english conventions as much as you can, e.g., space
> after "*" in comments, space after period at end of sentence,
> capitalize first word of sentence, blank line between paragraphs.
> Most of this you can see by looking at other comments.
> 

Thank you sincerely for your help with my first patch commitment.

> >  		pci_read_bridge_io(bridge, &res, true);
> >  	}
> >  
> > -- 
> > 2.39.2
> > 

Re: Re: [PATCH] PCI: Enable io space 1k granularity for intel cpu root port

[Bjorn Helgaas]

On Sun, Jun 30, 2024 at 02:52:25AM +0000, Zhou Shengqing wrote:
> > On Thu, Jun 27, 2024 at 12:58:56AM +0000, Zhou Shengqing wrote:
> > > This patch add 1k granularity for intel root port bridge.Intel latest
> > > server CPU support 1K granularity,And there is an BIOS setup item named
> > > "EN1K",but linux doesn't support it. if an IIO has 5 IOU (SPR has 5 IOUs)
> > > all are bifurcated 2x8.In a 2P server system,There are 20 P2P bridges
> > > present.if keep 4K granularity allocation,it need 20*4=80k io space,
> > > exceeding 64k.I test it in a 16*nvidia 4090s system under intel eaglestrem
> > > platform.There are six 4090s that cannot be allocated I/O resources.
> > > So I applied this patch.And I found a similar implementation in quirks.c,
> > > but it only targets the Intel P64H2 platform.
> > > 
> > > Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
> > > ---
> > >  drivers/pci/probe.c | 23 +++++++++++++++++++++++
> > >  1 file changed, 23 insertions(+)
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 5fbabb4e3425..909962795311 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -461,6 +461,9 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
> > >  	u32 buses;
> > >  	u16 io;
> > >  	u32 pmem, tmp;
> > > +	u16 ven_id, dev_id;
> > > +	u16 en1k = 0;
> > > +	struct pci_dev *dev = NULL;
> > >  	struct resource res;
> > >  
> > >  	pci_read_config_dword(bridge, PCI_PRIMARY_BUS, &buses);
> > > @@ -478,6 +481,26 @@ static void pci_read_bridge_windows(struct pci_dev *bridge)
> > >  	}
> > >  	if (io) {
> > >  		bridge->io_window = 1;
> > > +		if (pci_is_root_bus(bridge->bus)) {
> > > +			list_for_each_entry(dev, &bridge->bus->devices, bus_list) {
> > > +				pci_read_config_word(dev, PCI_VENDOR_ID, &ven_id);
> > > +				pci_read_config_word(dev, PCI_DEVICE_ID, &dev_id);
> > > +				if (ven_id == PCI_VENDOR_ID_INTEL && dev_id == 0x09a2) {
> > > +					/*IIO MISC Control offset 0x1c0*/
> > > +					pci_read_config_word(dev, 0x1c0, &en1k);
> > > +				}
> > > +			}
> > > +		/*
> > > +		 *Intel ICX SPR EMR GNR
> > > +		 *IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
> > > +		 *bit 2:Enable 1K (EN1K)
> > > +		 *This bit when set, enables 1K granularity for I/O space decode
> > > +		 *in each of the virtual P2P bridges
> > > +		 *corresponding to root ports, and DMI ports.
> > > +		 */
> > > +		if (en1k & 0x4)
> > > +			bridge->io_window_1k = 1;
> > > +		}
> > 
> > I still think this is not going to work because I don't want this kind
> > of device-specific clutter in this generic path. The pcibios_*
> > interfaces are history that we'd like to get rid of also, but it would
> > be better than putting it here.
> 
> Do you think it should be putted to the pci_bios* interface?
> 
> And if there is no suitable place to apply this patch, 
> then let's just ignore this issue.

What are the device/function addresses of the IIO and the Root Ports?

They must all be on the root bus, and if the IIO is enumerated first
(i.e., if it has a smaller device/function number), you may be able to 
make a quirk that applies to the Root Port vendor/device ID but reads
the config bit from the IIO.

quirk_passive_release(), quirk_vialatency(), quirk_amd_780_apc_msi(),
etc. do similar things.

Bjorn

[PATCH v4] Subject: PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

This patch add 1k granularity for intel root port bridge. Intel latest
server CPU support 1K granularity, And there is an BIOS setup item named
"EN1K", but linux doesn't support it. if an IIO has 5 IOU (SPR has 5 IOUs)
all are bifurcated 2x8.In a 2P server system,There are 20 P2P bridges
present. if keep 4K granularity allocation,it need 20*4=80k io space,
exceeding 64k. I test it in a 16*nvidia 4090s system under intel eaglestrem
platform. There are six 4090s that cannot be allocated I/O resources.
So I applied this patch. And I found a similar implementation in quirks.c,
but it only targets the Intel P64H2 platform.

Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
---
 drivers/pci/quirks.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 568410e64ce6..f30083d51e15 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2562,6 +2562,36 @@ static void quirk_p64h2_1k_io(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1460, quirk_p64h2_1k_io);
 
+/* Enable 1k I/O space granularity on the intel root port */
+static void quirk_intel_rootport_1k_io(struct pci_dev *dev)
+{
+	struct pci_dev *d = NULL;
+	u16 en1k = 0;
+	struct pci_dev *root_port = pcie_find_root_port(dev);
+
+	if (!root_port)
+		return;
+
+	/*
+	 * Per intel sever CPU EDS vol2(register) spec,
+	 * Intel Memory Map/Intel VT-d configuration space,
+	 * IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
+	 * bit 2.
+	 * Enable 1K (EN1K):
+	 * This bit when set, enables 1K granularity for I/O space decode
+	 * in each of the virtual P2P bridges
+	 * corresponding to root ports, and DMI ports.
+	 */
+	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
+		pci_read_config_word(d, 0x1c0, &en1k);
+		if (en1k & 0x4) {
+			pci_info(d, "INTEL: System should support 1k io window\n");
+			dev->io_window_1k = 1;
+		}
+	}
+}
+DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_ANY_ID,	quirk_intel_rootport_1k_io);
+
 /*
  * Under some circumstances, AER is not linked with extended capabilities.
  * Force it to be linked by setting the corresponding control bit in the
-- 
2.39.2

Re: [PATCH v4] Subject: PCI: Enable io space 1k granularity for intel cpu root port

[Bjorn Helgaas]

On Tue, Jul 02, 2024 at 03:56:49AM +0000, Zhou Shengqing wrote:
> This patch add 1k granularity for intel root port bridge. Intel latest
> server CPU support 1K granularity, And there is an BIOS setup item named
> "EN1K", but linux doesn't support it. if an IIO has 5 IOU (SPR has 5 IOUs)
> all are bifurcated 2x8.In a 2P server system,There are 20 P2P bridges
> present. if keep 4K granularity allocation,it need 20*4=80k io space,
> exceeding 64k. I test it in a 16*nvidia 4090s system under intel eaglestrem
> platform. There are six 4090s that cannot be allocated I/O resources.
> So I applied this patch. And I found a similar implementation in quirks.c,
> but it only targets the Intel P64H2 platform.

I think this has potential.  Can you include a more complete citation
for the Intel spec?  Complete name, document number if available,
revision, section?  Hopefully it's publically available?

> Signed-off-by: Zhou Shengqing <zhoushengqing@ttyinfo.com>
> ---
>  drivers/pci/quirks.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 568410e64ce6..f30083d51e15 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2562,6 +2562,36 @@ static void quirk_p64h2_1k_io(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, 0x1460, quirk_p64h2_1k_io);
>  
> +/* Enable 1k I/O space granularity on the intel root port */
> +static void quirk_intel_rootport_1k_io(struct pci_dev *dev)
> +{
> +	struct pci_dev *d = NULL;
> +	u16 en1k = 0;
> +	struct pci_dev *root_port = pcie_find_root_port(dev);
> +
> +	if (!root_port)
> +		return;

This doesn't seem quite right to me.  The point is to set
dev->io_window_1k when "dev" is a Root Port itself and when the EN1K
bit is set in a [8086:09a2] device.

So I don't think we need to *look* for the Root Port, we just need to
check that "dev" itself *is* a Root Port, e.g.,

  if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
    return;

> +	/*
> +	 * Per intel sever CPU EDS vol2(register) spec,
> +	 * Intel Memory Map/Intel VT-d configuration space,
> +	 * IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
> +	 * bit 2.
> +	 * Enable 1K (EN1K):
> +	 * This bit when set, enables 1K granularity for I/O space decode
> +	 * in each of the virtual P2P bridges
> +	 * corresponding to root ports, and DMI ports.
> +	 */
> +	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {

To be safe, "d" (the [8086:09a2] device) should be on the same bus as
"dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
which would be a different bus, and they presumably are not influenced
by the EN1K bit.

> +		pci_read_config_word(d, 0x1c0, &en1k);
> +		if (en1k & 0x4) {
> +			pci_info(d, "INTEL: System should support 1k io window\n");

If we log this, I think it should be with "dev", not "d", since we
likely will have several Root Ports, and this would lead to several
identical messages.  Maybe something like this:

  pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));

> +			dev->io_window_1k = 1;
> +		}
> +	}
> +}
> +DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_ANY_ID,	quirk_intel_rootport_1k_io);
> +
>  /*
>   * Under some circumstances, AER is not linked with extended capabilities.
>   * Force it to be linked by setting the corresponding control bit in the
> -- 
> 2.39.2
> 

Re: [PATCH v4] Subject: PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

> I think this has potential.  Can you include a more complete citation
> for the Intel spec?  Complete name, document number if available,
> revision, section?  Hopefully it's publically available?

Most of intel CPU EDS specs are under NDA. But you can refer to
https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-v2-datasheet-vol-2.pdf
keyword:"EN1K".

> + /*
> > + * Per intel sever CPU EDS vol2(register) spec,
> > + * Intel Memory Map/Intel VT-d configuration space,
> > + * IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
> > + * bit 2.
> > + * Enable 1K (EN1K):
> > + * This bit when set, enables 1K granularity for I/O space decode
> > + * in each of the virtual P2P bridges
> > + * corresponding to root ports, and DMI ports.
> > + */
> > + while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> 
> To be safe, "d" (the [8086:09a2] device) should be on the same bus as
> "dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
> which would be a different bus, and they presumably are not influenced
> by the EN1K bit.

I modified the code as follows, can you help me review it? 

/* Enable 1k I/O space granularity on the intel root port */
static void quirk_intel_rootport_1k_io(struct pci_dev *dev)
{
	struct pci_dev *d = NULL;
	u16 en1k = 0;

	if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
		return;

	/*
	 * Per intel sever CPU (ICX SPR GNR)EDS vol2(register) spec,
	 * Intel Memory Map/Intel VT-d configuration space,
	 * IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
	 * bit 2.
	 * Enable 1K (EN1K):
	 * This bit when set, enables 1K granularity for I/O space decode 
	 * in each of the virtual P2P bridges
	 * corresponding to root ports, and DMI ports.
	 */
	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
		if (pci_domain_nr(d->bus) == pci_domain_nr(dev->bus)) {
			pci_read_config_word(d, 0x1c0, &en1k);
			if (en1k & 0x4) {
				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
				dev->io_window_1k = 1;
			}
		}
	}
}
DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_ANY_ID,	quirk_intel_rootport_1k_io);

If you have a better method, please let me know. If there are no issues, 
I can submit a new patch.

Re: [PATCH v4] Subject: PCI: Enable io space 1k granularity for intel cpu root port

[Ethan Zhao]

On 7/23/2024 4:04 PM, Zhou Shengqing wrote:
>> I think this has potential.  Can you include a more complete citation
>> for the Intel spec?  Complete name, document number if available,
>> revision, section?  Hopefully it's publically available?
> Most of intel CPU EDS specs are under NDA. But you can refer to
> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-v2-datasheet-vol-2.pdf
> keyword:"EN1K".
>
>> + /*
>>> + * Per intel sever CPU EDS vol2(register) spec,
>>> + * Intel Memory Map/Intel VT-d configuration space,
>>> + * IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
>>> + * bit 2.
>>> + * Enable 1K (EN1K):
>>> + * This bit when set, enables 1K granularity for I/O space decode
>>> + * in each of the virtual P2P bridges
>>> + * corresponding to root ports, and DMI ports.
>>> + */
>>> + while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
>> To be safe, "d" (the [8086:09a2] device) should be on the same bus as
>> "dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
>> which would be a different bus, and they presumably are not influenced
>> by the EN1K bit.
> I modified the code as follows, can you help me review it?
>
> /* Enable 1k I/O space granularity on the intel root port */
> static void quirk_intel_rootport_1k_io(struct pci_dev *dev)
> {
> 	struct pci_dev *d = NULL;
> 	u16 en1k = 0;
>
> 	if (!pci_is_pcie(dev) || pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> 		return;
>
> 	/*
> 	 * Per intel sever CPU (ICX SPR GNR)EDS vol2(register) spec,
> 	 * Intel Memory Map/Intel VT-d configuration space,
> 	 * IIO MISC Control (IIOMISCCTRL_1_5_0_CFG) — Offset 1C0h
> 	 * bit 2.
> 	 * Enable 1K (EN1K):
> 	 * This bit when set, enables 1K granularity for I/O space decode
> 	 * in each of the virtual P2P bridges
> 	 * corresponding to root ports, and DMI ports.
> 	 */
> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> 		if (pci_domain_nr(d->bus) == pci_domain_nr(dev->bus)) {

Perhaps it is enough to check if the 0x09a2 VT-d and the rootport are on the smae bus
e.g. On my SPR, domain 0000

00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])

  
15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])

and if you check domain number only, they might sit on different bus, perhaps that
would make thing complex, could you make sure the VT-d is on the upstream bus of the
bridge ?

Thanks,
Ethan
  

> 			pci_read_config_word(d, 0x1c0, &en1k);
> 			if (en1k & 0x4) {
> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
> 				dev->io_window_1k = 1;
> 			}
> 		}
> 	}
> }
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL,	PCI_ANY_ID,	quirk_intel_rootport_1k_io);
>
> If you have a better method, please let me know. If there are no issues,
> I can submit a new patch.
>

Re: [PATCH v4] Subject: PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

> On 7/23/2024 4:04 PM, Zhou Shengqing wrote:
> >> I think this has potential.  Can you include a more complete citation
> >> for the Intel spec?  Complete name, document number if available,
> >> revision, section?  Hopefully it's publically available?
> > Most of intel CPU EDS specs are under NDA. But you can refer to
> > https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-v2-datasheet-vol-2.pdf
> > keyword:"EN1K".
> > ...
> > 
> > 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> > 		if (pci_domain_nr(d->bus) == pci_domain_nr(dev->bus)) {
> 
> Perhaps it is enough to check if the 0x09a2 VT-d and the rootport are on the smae bus
> e.g. On my SPR, domain 0000

Thank you for your comment.

Do you mean it shoud be like this?

	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
		if (d->bus->number == dev->bus->number) {
			pci_read_config_word(d, 0x1c0, &en1k);
			if (en1k & 0x4) {
				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
				dev->io_window_1k = 1;
			}
		}
	}

> 
> 00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
> 00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])
> 
>   
> 15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
> 15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])
> 
> and if you check domain number only, they might sit on different bus, perhaps that
> would make thing complex, could you make sure the VT-d is on the upstream bus of the
> bridge ?

I checked it on ICX SPR EMR GNR, VT-d is always on the same bus with root port,
and VT-d device and function number is always 0. 

Please let me know if further modifications are needed.

> 
> Thanks,
> Ethan

Re: [PATCH v4] Subject: PCI: Enable io space 1k granularity for intel cpu root port

[Ethan Zhao]

On 7/24/2024 11:38 AM, Zhou Shengqing wrote:
>> On 7/23/2024 4:04 PM, Zhou Shengqing wrote:
>>>> I think this has potential.  Can you include a more complete citation
>>>> for the Intel spec?  Complete name, document number if available,
>>>> revision, section?  Hopefully it's publically available?
>>> Most of intel CPU EDS specs are under NDA. But you can refer to
>>> https://www.intel.com/content/dam/www/public/us/en/documents/datasheets/xeon-e5-v2-datasheet-vol-2.pdf
>>> keyword:"EN1K".
>>> ...
>>>
>>> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
>>> 		if (pci_domain_nr(d->bus) == pci_domain_nr(dev->bus)) {
>> Perhaps it is enough to check if the 0x09a2 VT-d and the rootport are on the smae bus
>> e.g. On my SPR, domain 0000
> Thank you for your comment.
>
> Do you mean it shoud be like this?
>
> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> 		if (d->bus->number == dev->bus->number) {
> 			pci_read_config_word(d, 0x1c0, &en1k);
> 			if (en1k & 0x4) {
> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
> 				dev->io_window_1k = 1;
> 			}
> 		}
> 	}
>
>> 00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
>> 00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])
>>
>>    
>> 15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
>> 15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])
>>
>> and if you check domain number only, they might sit on different bus, perhaps that
>> would make thing complex, could you make sure the VT-d is on the upstream bus of the
>> bridge ?
> I checked it on ICX SPR EMR GNR, VT-d is always on the same bus with root port,
> and VT-d device and function number is always 0.

Yes, every VT-d instance in the root complex and the root port integrated are
on the same bus. and VT-d is the first device of that bus.

The EDS doesn't say if there is exception one of the VT-d instances in an
system its EN1K wasn't set while others were set, vice vesa. so I suggest
just check the VT-d and then set the root port's io_windows_1k of the same bus.

Hope that works for your case.


Thanks,
Ethan

>
> Please let me know if further modifications are needed.
>
>> Thanks,
>> Ethan

Re: [PATCH v4] PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

> > Do you mean it shoud be like this?
> >
> > 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> > 		if (d->bus->number == dev->bus->number) {
> > 			pci_read_config_word(d, 0x1c0, &en1k);
> > 			if (en1k & 0x4) {
> > 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
> > 				dev->io_window_1k = 1;
> > 			}
> > 		}
> > 	}
> >
> >> 00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
> >> 00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])
> >>
> >>    
> >> 15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
> >> 15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])
> >>
> >> and if you check domain number only, they might sit on different bus, perhaps that
> >> would make thing complex, could you make sure the VT-d is on the upstream bus of the
> >> bridge ?
> > I checked it on ICX SPR EMR GNR, VT-d is always on the same bus with root port,
> > and VT-d device and function number is always 0.
> 
> Yes, every VT-d instance in the root complex and the root port integrated are
> on the same bus. and VT-d is the first device of that bus.
> 
> The EDS doesn't say if there is exception one of the VT-d instances in an
> system its EN1K wasn't set while others were set, vice vesa. so I suggest
> just check the VT-d and then set the root port's io_windows_1k of the same bus.

But as Bjorn mentioned at July 12, 2024, 6:48 p.m.,

"To be safe, "d" (the [8086:09a2] device) should be on the same bus as
"dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
which would be a different bus, and they presumably are not influenced
by the EN1K bit."

When VMD enabled, just check bus number identical may lead to enable
1k io windows for VMD domain root port. E.g. 0000:80:00.0 is a 
VT-d(09a2). If VMD enabled, there might be a root port 10000:80:01.0 present.
this code may lead to enable 10000:80:01.0 io_window_1k = 1. 
This is probably not expected.

If I modify it like this,

	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
	---if (d->bus->number == dev->bus->number) {
	+++if (d->bus == dev->bus) {
			pci_read_config_word(d, 0x1c0, &en1k);
			if (en1k & 0x4) {
				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
				dev->io_window_1k = 1;
			}
		}
	}
    
Can the situation mentioned above be avoided?

Hope for your suggestion.

> 
> Hope that works for your case.
> 

Re: [PATCH v4] PCI: Enable io space 1k granularity for intel cpu root port

[Ethan Zhao]

On 7/24/2024 2:35 PM, Zhou Shengqing wrote:
>>> Do you mean it shoud be like this?
>>>
>>> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
>>> 		if (d->bus->number == dev->bus->number) {
>>> 			pci_read_config_word(d, 0x1c0, &en1k);
>>> 			if (en1k & 0x4) {
>>> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
>>> 				dev->io_window_1k = 1;
>>> 			}
>>> 		}
>>> 	}
>>>
>>>> 00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
>>>> 00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])
>>>>
>>>>     
>>>> 15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
>>>> 15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])
>>>>
>>>> and if you check domain number only, they might sit on different bus, perhaps that
>>>> would make thing complex, could you make sure the VT-d is on the upstream bus of the
>>>> bridge ?
>>> I checked it on ICX SPR EMR GNR, VT-d is always on the same bus with root port,
>>> and VT-d device and function number is always 0.
>> Yes, every VT-d instance in the root complex and the root port integrated are
>> on the same bus. and VT-d is the first device of that bus.
>>
>> The EDS doesn't say if there is exception one of the VT-d instances in an
>> system its EN1K wasn't set while others were set, vice vesa. so I suggest
>> just check the VT-d and then set the root port's io_windows_1k of the same bus.
> But as Bjorn mentioned at July 12, 2024, 6:48 p.m.,
>
> "To be safe, "d" (the [8086:09a2] device) should be on the same bus as
> "dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
> which would be a different bus, and they presumably are not influenced
> by the EN1K bit."
>
> When VMD enabled, just check bus number identical may lead to enable
> 1k io windows for VMD domain root port. E.g. 0000:80:00.0 is a
> VT-d(09a2). If VMD enabled, there might be a root port 10000:80:01.0 present.
> this code may lead to enable 10000:80:01.0 io_window_1k = 1.
> This is probably not expected.
>
> If I modify it like this,
>
> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {

BTW, don't save letters to use single letter variable 'd', please use 'vtd_dev' or
something else to express the VT-d device.

> 	---if (d->bus->number == dev->bus->number) {
> 	+++if (d->bus == dev->bus) {

What if their 'bus' are NULL, though it is almost impossible. :)

> 			pci_read_config_word(d, 0x1c0, &en1k);
> 			if (en1k & 0x4) {
> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
> 				dev->io_window_1k = 1;
> 			}
> 		}
> 	}
>      
> Can the situation mentioned above be avoided?

Yes, my understanding, as Bjorn pointed out root port extended from VMD
bridge not on the same bus as VT-d.



Thanks,
Ethan

>
> Hope for your suggestion.
>
>> Hope that works for your case.
>>

Re: [PATCH v4] PCI: Enable io space 1k granularity for intel cpu root port

[Zhou Shengqing]

> On 7/24/2024 2:35 PM, Zhou Shengqing wrote:
> >>> Do you mean it shoud be like this?
> >>>
> >>> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> >>> 		if (d->bus->number == dev->bus->number) {
> >>> 			pci_read_config_word(d, 0x1c0, &en1k);
> >>> 			if (en1k & 0x4) {
> >>> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
> >>> 				dev->io_window_1k = 1;
> >>> 			}
> >>> 		}
> >>> 	}
> >>>
> >>>> 00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
> >>>> 00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])
> >>>>
> >>>>     
> >>>> 15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
> >>>> 15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])
> >>>>
> >>>> and if you check domain number only, they might sit on different bus, perhaps that
> >>>> would make thing complex, could you make sure the VT-d is on the upstream bus of the
> >>>> bridge ?
> >>> I checked it on ICX SPR EMR GNR, VT-d is always on the same bus with root port,
> >>> and VT-d device and function number is always 0.
> >> Yes, every VT-d instance in the root complex and the root port integrated are
> >> on the same bus. and VT-d is the first device of that bus.
> >>
> >> The EDS doesn't say if there is exception one of the VT-d instances in an
> >> system its EN1K wasn't set while others were set, vice vesa. so I suggest
> >> just check the VT-d and then set the root port's io_windows_1k of the same bus.
> > But as Bjorn mentioned at July 12, 2024, 6:48 p.m.,
> >
> > "To be safe, "d" (the [8086:09a2] device) should be on the same bus as
> > "dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
> > which would be a different bus, and they presumably are not influenced
> > by the EN1K bit."
> >
> > When VMD enabled, just check bus number identical may lead to enable
> > 1k io windows for VMD domain root port. E.g. 0000:80:00.0 is a
> > VT-d(09a2). If VMD enabled, there might be a root port 10000:80:01.0 present.
> > this code may lead to enable 10000:80:01.0 io_window_1k = 1.
> > This is probably not expected.
> >
> > If I modify it like this,
> >
> > 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
> 
> BTW, don't save letters to use single letter variable 'd', please use 'vtd_dev' or
> something else to express the VT-d device.

Got it!

> 
> > 	---if (d->bus->number == dev->bus->number) {
> > 	+++if (d->bus == dev->bus) {
> 
> What if their 'bus' are NULL, though it is almost impossible. :)
> 
> > 			pci_read_config_word(d, 0x1c0, &en1k);
> > 			if (en1k & 0x4) {
> > 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
> > 				dev->io_window_1k = 1;
> > 			}
> > 		}
> > 	}
> >      
> > Can the situation mentioned above be avoided?
> 
> Yes, my understanding, as Bjorn pointed out root port extended from VMD
> bridge not on the same bus as VT-d.

For the root port extended from VMD, should the 1k window be set
when BIOS setup EN1K knob enabled? 
In my case, I think  EN1K should not apply to the VMD root port.

But what I'm confused about is, how can I reasonably exclude the VMD root port
in the code?

> 
> 

Re: [PATCH v4] PCI: Enable io space 1k granularity for intel cpu root port

[Ethan Zhao]

On 7/25/2024 3:44 PM, Zhou Shengqing wrote:
>> On 7/24/2024 2:35 PM, Zhou Shengqing wrote:
>>>>> Do you mean it shoud be like this?
>>>>>
>>>>> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
>>>>> 		if (d->bus->number == dev->bus->number) {
>>>>> 			pci_read_config_word(d, 0x1c0, &en1k);
>>>>> 			if (en1k & 0x4) {
>>>>> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
>>>>> 				dev->io_window_1k = 1;
>>>>> 			}
>>>>> 		}
>>>>> 	}
>>>>>
>>>>>> 00:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
>>>>>> 00:0f.0 PCI bridge: Intel Corporation Device 1bbf (rev 10) (prog-if 00 [Normal decode])
>>>>>>
>>>>>>      
>>>>>> 15:00.0 System peripheral: Intel Corporation Device 09a2 (rev 20)
>>>>>> 15:01.0 PCI bridge: Intel Corporation Device 352a (rev 04) (prog-if 00 [Normal decode])
>>>>>>
>>>>>> and if you check domain number only, they might sit on different bus, perhaps that
>>>>>> would make thing complex, could you make sure the VT-d is on the upstream bus of the
>>>>>> bridge ?
>>>>> I checked it on ICX SPR EMR GNR, VT-d is always on the same bus with root port,
>>>>> and VT-d device and function number is always 0.
>>>> Yes, every VT-d instance in the root complex and the root port integrated are
>>>> on the same bus. and VT-d is the first device of that bus.
>>>>
>>>> The EDS doesn't say if there is exception one of the VT-d instances in an
>>>> system its EN1K wasn't set while others were set, vice vesa. so I suggest
>>>> just check the VT-d and then set the root port's io_windows_1k of the same bus.
>>> But as Bjorn mentioned at July 12, 2024, 6:48 p.m.,
>>>
>>> "To be safe, "d" (the [8086:09a2] device) should be on the same bus as
>>> "dev" (with VMD, I think we get Root Ports *below* the VMD bridge,
>>> which would be a different bus, and they presumably are not influenced
>>> by the EN1K bit."
>>>
>>> When VMD enabled, just check bus number identical may lead to enable
>>> 1k io windows for VMD domain root port. E.g. 0000:80:00.0 is a
>>> VT-d(09a2). If VMD enabled, there might be a root port 10000:80:01.0 present.
>>> this code may lead to enable 10000:80:01.0 io_window_1k = 1.
>>> This is probably not expected.
>>>
>>> If I modify it like this,
>>>
>>> 	while ((d = pci_get_device(PCI_VENDOR_ID_INTEL, 0x09a2, d))) {
>> BTW, don't save letters to use single letter variable 'd', please use 'vtd_dev' or
>> something else to express the VT-d device.
> Got it!
>
>>> 	---if (d->bus->number == dev->bus->number) {
>>> 	+++if (d->bus == dev->bus) {
>> What if their 'bus' are NULL, though it is almost impossible. :)
>>
>>> 			pci_read_config_word(d, 0x1c0, &en1k);
>>> 			if (en1k & 0x4) {
>>> 				pci_info(dev, "1K I/O windows enabled per %s EN1K setting\n", pci_name(d));
>>> 				dev->io_window_1k = 1;
>>> 			}
>>> 		}
>>> 	}
>>>       
>>> Can the situation mentioned above be avoided?
>> Yes, my understanding, as Bjorn pointed out root port extended from VMD
>> bridge not on the same bus as VT-d.
> For the root port extended from VMD, should the 1k window be set
> when BIOS setup EN1K knob enabled?
> In my case, I think  EN1K should not apply to the VMD root port.
>
> But what I'm confused about is, how can I reasonably exclude the VMD root port
> in the code?

VMD, if enabled, is EP, not RP. and its RPs are mapped into its own space, and
sit at different buses as VT-d, no need to care about them if am correct.

Thanks,
Ethan

>>
>

Re: Re: Re: [PATCH] PCI: Enable io space 1k granularity for

[Zhou Shengqing]

> > I still think this is not going to work because I don't want this kind
> > of device-specific clutter in this generic path. The pcibios_*
> > interfaces are history that we'd like to get rid of also, but it would
> > be better than putting it here.
> 
> Do you think it should be putted to the pci_bios* interface?
> 
> And if there is no suitable place to apply this patch, 
> then let's just ignore this issue.

> What are the device/function addresses of the IIO and the Root Ports?

IIO device = 0,function = 0.
Root Ports device = 1/2/3/4/5/6/7/8,function = 0, And they are on the same bus.

> 
> They must all be on the root bus, and if the IIO is enumerated first
> (i.e., if it has a smaller device/function number), you may be able to 
> make a quirk that applies to the Root Port vendor/device ID but reads
> the config bit from the IIO.
> 
> quirk_passive_release(), quirk_vialatency(), quirk_amd_780_apc_msi(),
> etc. do similar things.

I have commited a new patch referred to quirk_passive_release(). 
https://lore.kernel.org/all/20240702035649.26039-1-zhoushengqing@ttyinfo.com/

And I validate it on my system which previously some devices couldn't have IO space.
After applying this patch, all devices were able to have IO space.

Thank you again for your assistance.

> 
> Bjorn