| Message ID | 20190612170647.43220-1-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | [v2,1/1] PCI/IOV: Fix incorrect cfg_size for VF > 0 | expand |
This looks to basically be a duplicate of what Alex Williamson posted 8 days ago: https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/ Alex says Hao Zheng had a similar patch as well. On Wed, Jun 12, 2019 at 12:08 PM <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > other VFs") calculates and caches the cfg_size for VF0 device before > initializing the pcie_cap of the device which results in using incorrect > cfg_size for all VF devices > 0. So set pcie_cap of the device before > calculating the cfg_size of VF0 device. > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > other VFs") > Cc: Ashok Raj <ashok.raj@intel.com> > Suggested-by: Mike Campin <mike.campin@intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > > Changes since v1: > * Fixed a typo in commit message. > > drivers/pci/iov.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index 3aa115ed3a65..2869011c0e35 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > virtfn->device = iov->vf_device; > virtfn->is_virtfn = 1; > virtfn->physfn = pci_dev_get(dev); > + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); > > if (id == 0) > pci_read_vf_config_common(virtfn); > -- > 2.21.0 >
On Wed, 12 Jun 2019 10:06:47 -0700 sathyanarayanan.kuppuswamy@linux.intel.com wrote: > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > other VFs") calculates and caches the cfg_size for VF0 device before > initializing the pcie_cap of the device which results in using incorrect > cfg_size for all VF devices > 0. So set pcie_cap of the device before > calculating the cfg_size of VF0 device. > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > other VFs") > Cc: Ashok Raj <ashok.raj@intel.com> > Suggested-by: Mike Campin <mike.campin@intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > > Changes since v1: > * Fixed a typo in commit message. > > drivers/pci/iov.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index 3aa115ed3a65..2869011c0e35 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > virtfn->device = iov->vf_device; > virtfn->is_virtfn = 1; > virtfn->physfn = pci_dev_get(dev); > + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); > > if (id == 0) > pci_read_vf_config_common(virtfn); Why not re-order until after we've setup pcie_cap? https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/T/# Thanks, Alex
On 6/12/19 11:19 AM, Alex Williamson wrote: > On Wed, 12 Jun 2019 10:06:47 -0700 > sathyanarayanan.kuppuswamy@linux.intel.com wrote: > >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for >> other VFs") calculates and caches the cfg_size for VF0 device before >> initializing the pcie_cap of the device which results in using incorrect >> cfg_size for all VF devices > 0. So set pcie_cap of the device before >> calculating the cfg_size of VF0 device. >> >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for >> other VFs") >> Cc: Ashok Raj <ashok.raj@intel.com> >> Suggested-by: Mike Campin <mike.campin@intel.com> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> >> Changes since v1: >> * Fixed a typo in commit message. >> >> drivers/pci/iov.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c >> index 3aa115ed3a65..2869011c0e35 100644 >> --- a/drivers/pci/iov.c >> +++ b/drivers/pci/iov.c >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) >> virtfn->device = iov->vf_device; >> virtfn->is_virtfn = 1; >> virtfn->physfn = pci_dev_get(dev); >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); >> >> if (id == 0) >> pci_read_vf_config_common(virtfn); > Why not re-order until after we've setup pcie_cap? > > https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/T/# pci_read_vf_config_common() also caches values for properties like class, hdr_type, susbsystem_vendor/device. These values are read/used in pci_setup_device(). So if we can use cached values in pci_setup_device(), we don't have to read them from registers twice for each device. > > Thanks, > Alex >
On Wed, 12 Jun 2019 11:41:36 -0700 sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > On 6/12/19 11:19 AM, Alex Williamson wrote: > > On Wed, 12 Jun 2019 10:06:47 -0700 > > sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >> > >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > >> other VFs") calculates and caches the cfg_size for VF0 device before > >> initializing the pcie_cap of the device which results in using incorrect > >> cfg_size for all VF devices > 0. So set pcie_cap of the device before > >> calculating the cfg_size of VF0 device. > >> > >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > >> other VFs") > >> Cc: Ashok Raj <ashok.raj@intel.com> > >> Suggested-by: Mike Campin <mike.campin@intel.com> > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > >> --- > >> > >> Changes since v1: > >> * Fixed a typo in commit message. > >> > >> drivers/pci/iov.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > >> index 3aa115ed3a65..2869011c0e35 100644 > >> --- a/drivers/pci/iov.c > >> +++ b/drivers/pci/iov.c > >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > >> virtfn->device = iov->vf_device; > >> virtfn->is_virtfn = 1; > >> virtfn->physfn = pci_dev_get(dev); > >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); > >> > >> if (id == 0) > >> pci_read_vf_config_common(virtfn); > > Why not re-order until after we've setup pcie_cap? > > > > https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/T/# > > pci_read_vf_config_common() also caches values for properties like > class, hdr_type, susbsystem_vendor/device. These values are read/used in > pci_setup_device(). So if we can use cached values in > pci_setup_device(), we don't have to read them from registers twice for > each device. Sorry, I missed that dependency, a bit too subtle. It's still pretty ugly that pci_setup_device()->set_pcie_port_type() is the canonical location for setting pcie_cap and now we need to kludge it earlier. What about the question in the self follow-up to my patch in the link above, can we simply assume 4K config space on a VF? Thanks, Alex
On Wed, Jun 12, 2019 at 12:58:17PM -0600, Alex Williamson wrote: > On Wed, 12 Jun 2019 11:41:36 -0700 > sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> > wrote: > > > On 6/12/19 11:19 AM, Alex Williamson wrote: > > > On Wed, 12 Jun 2019 10:06:47 -0700 > > > sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > > >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > >> > > >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > >> other VFs") calculates and caches the cfg_size for VF0 device before > > >> initializing the pcie_cap of the device which results in using incorrect > > >> cfg_size for all VF devices > 0. So set pcie_cap of the device before > > >> calculating the cfg_size of VF0 device. > > >> > > >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > >> other VFs") > > >> Cc: Ashok Raj <ashok.raj@intel.com> > > >> Suggested-by: Mike Campin <mike.campin@intel.com> > > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > >> --- > > >> > > >> Changes since v1: > > >> * Fixed a typo in commit message. > > >> > > >> drivers/pci/iov.c | 1 + > > >> 1 file changed, 1 insertion(+) > > >> > > >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > >> index 3aa115ed3a65..2869011c0e35 100644 > > >> --- a/drivers/pci/iov.c > > >> +++ b/drivers/pci/iov.c > > >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > > >> virtfn->device = iov->vf_device; > > >> virtfn->is_virtfn = 1; > > >> virtfn->physfn = pci_dev_get(dev); > > >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); > > >> > > >> if (id == 0) > > >> pci_read_vf_config_common(virtfn); > > > Why not re-order until after we've setup pcie_cap? > > > > > > https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/T/# > > > > pci_read_vf_config_common() also caches values for properties like > > class, hdr_type, susbsystem_vendor/device. These values are read/used in > > pci_setup_device(). So if we can use cached values in > > pci_setup_device(), we don't have to read them from registers twice for > > each device. > > Sorry, I missed that dependency, a bit too subtle. It's still pretty > ugly that pci_setup_device()->set_pcie_port_type() is the canonical > location for setting pcie_cap and now we need to kludge it earlier. > What about the question in the self follow-up to my patch in the link > above, can we simply assume 4K config space on a VF? Thanks, There should be no issue simply reading them once? I don't know what that exact optimization saves, unless some broken VFs didn't actually expose all the capabilities in config space and this happens to workaround the problem. + Karim Cheers, Ashok
On Wed, 12 Jun 2019 12:03:03 -0700 "Raj, Ashok" <ashok.raj@intel.com> wrote: > On Wed, Jun 12, 2019 at 12:58:17PM -0600, Alex Williamson wrote: > > On Wed, 12 Jun 2019 11:41:36 -0700 > > sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> > > wrote: > > > > > On 6/12/19 11:19 AM, Alex Williamson wrote: > > > > On Wed, 12 Jun 2019 10:06:47 -0700 > > > > sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > > > > >> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > >> > > > >> Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > > >> other VFs") calculates and caches the cfg_size for VF0 device before > > > >> initializing the pcie_cap of the device which results in using incorrect > > > >> cfg_size for all VF devices > 0. So set pcie_cap of the device before > > > >> calculating the cfg_size of VF0 device. > > > >> > > > >> Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > > >> other VFs") > > > >> Cc: Ashok Raj <ashok.raj@intel.com> > > > >> Suggested-by: Mike Campin <mike.campin@intel.com> > > > >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > >> --- > > > >> > > > >> Changes since v1: > > > >> * Fixed a typo in commit message. > > > >> > > > >> drivers/pci/iov.c | 1 + > > > >> 1 file changed, 1 insertion(+) > > > >> > > > >> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > > >> index 3aa115ed3a65..2869011c0e35 100644 > > > >> --- a/drivers/pci/iov.c > > > >> +++ b/drivers/pci/iov.c > > > >> @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > > > >> virtfn->device = iov->vf_device; > > > >> virtfn->is_virtfn = 1; > > > >> virtfn->physfn = pci_dev_get(dev); > > > >> + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); > > > >> > > > >> if (id == 0) > > > >> pci_read_vf_config_common(virtfn); > > > > Why not re-order until after we've setup pcie_cap? > > > > > > > > https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/T/# > > > > > > pci_read_vf_config_common() also caches values for properties like > > > class, hdr_type, susbsystem_vendor/device. These values are read/used in > > > pci_setup_device(). So if we can use cached values in > > > pci_setup_device(), we don't have to read them from registers twice for > > > each device. > > > > Sorry, I missed that dependency, a bit too subtle. It's still pretty > > ugly that pci_setup_device()->set_pcie_port_type() is the canonical > > location for setting pcie_cap and now we need to kludge it earlier. > > What about the question in the self follow-up to my patch in the link > > above, can we simply assume 4K config space on a VF? Thanks, > > There should be no issue simply reading them once? I don't know > what that exact optimization saves, unless some broken VFs didn't > actually expose all the capabilities in config space and this happens > to workaround the problem. AIUI the original commit only saves us a few config space reads for VFs after the first where we test that extended config space is available and not simply an alias of standard config space. Thanks, Alex
On Wed, 2019-06-12 at 12:03 -0700, Raj, Ashok wrote: > On Wed, Jun 12, 2019 at 12:58:17PM -0600, Alex Williamson wrote: > > > > On Wed, 12 Jun 2019 11:41:36 -0700 > > sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> > > wrote: > > > > > > > > On 6/12/19 11:19 AM, Alex Williamson wrote: > > > > > > > > On Wed, 12 Jun 2019 10:06:47 -0700 > > > > sathyanarayanan.kuppuswamy@linux.intel.com wrote: > > > > > > > > > > > > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > > > > > > Commit 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > > > > other VFs") calculates and caches the cfg_size for VF0 device before > > > > > initializing the pcie_cap of the device which results in using incorrect > > > > > cfg_size for all VF devices > 0. So set pcie_cap of the device before > > > > > calculating the cfg_size of VF0 device. > > > > > > > > > > Fixes: 975bb8b4dc93 ("PCI/IOV: Use VF0 cached config space size for > > > > > other VFs") > > > > > Cc: Ashok Raj <ashok.raj@intel.com> > > > > > Suggested-by: Mike Campin <mike.campin@intel.com> > > > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > > > > > --- > > > > > > > > > > Changes since v1: > > > > > * Fixed a typo in commit message. > > > > > > > > > > drivers/pci/iov.c | 1 + > > > > > 1 file changed, 1 insertion(+) > > > > > > > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > > > > index 3aa115ed3a65..2869011c0e35 100644 > > > > > --- a/drivers/pci/iov.c > > > > > +++ b/drivers/pci/iov.c > > > > > @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) > > > > > virtfn->device = iov->vf_device; > > > > > virtfn->is_virtfn = 1; > > > > > virtfn->physfn = pci_dev_get(dev); > > > > > + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); > > > > > > > > > > if (id == 0) > > > > > pci_read_vf_config_common(virtfn); > > > > Why not re-order until after we've setup pcie_cap? > > > > > > > > https://lore.kernel.org/linux-pci/20190604143617.0a226555@x1.home/T/# > > > > > > pci_read_vf_config_common() also caches values for properties like > > > class, hdr_type, susbsystem_vendor/device. These values are read/used in > > > pci_setup_device(). So if we can use cached values in > > > pci_setup_device(), we don't have to read them from registers twice for > > > each device. > > > > Sorry, I missed that dependency, a bit too subtle. It's still pretty > > ugly that pci_setup_device()->set_pcie_port_type() is the canonical > > location for setting pcie_cap and now we need to kludge it earlier. > > What about the question in the self follow-up to my patch in the link > > above, can we simply assume 4K config space on a VF? Thanks, > > There should be no issue simply reading them once? I don't know > what that exact optimization saves, unless some broken VFs didn't > actually expose all the capabilities in config space and this happens > to workaround the problem. The original patch was to save time when you have hundreds of VFs in the system and doing this for each one of them is just a waste of time. > > + Karim > > Cheers, > Ashok Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index 3aa115ed3a65..2869011c0e35 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -160,6 +160,7 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id) virtfn->device = iov->vf_device; virtfn->is_virtfn = 1; virtfn->physfn = pci_dev_get(dev); + virtfn->pcie_cap = pci_find_capability(virtfn, PCI_CAP_ID_EXP); if (id == 0) pci_read_vf_config_common(virtfn);