| Message ID | 20240919223557.1897608-3-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | PCI: VF resizable BAR | expand |
Am 20.09.24 um 00:35 schrieb Michał Winiarski: > VF MMIO resource reservation, either created by system firmware and > inherited by Linux PCI subsystem or created by the subsystem itself, > contains enough space to fit the BAR of all SR-IOV Virtual Functions > that can potentially be created (total VFs supported by the device). > This can be leveraged when the device is exposing lower than optimal BAR > size as a default, allowing access to the entire resource when lower > number of VFs are created. > It is achieved by dynamically resizing the BAR to largest possible value > that allows to fit all newly created VFs within the original resource > boundary. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > drivers/pci/iov.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- > drivers/pci/pci.h | 1 + > include/linux/pci.h | 3 ++ > 3 files changed, 95 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index e8ccd2ae0f024..d88efbfa70e42 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) > return cmd & PCI_SRIOV_CTRL_MSE; > } > > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs) > +{ > + resource_size_t size; > + int ret, old, i; > + u32 sizes; > + > + pci_config_pm_runtime_get(dev); > + > + if (pci_iov_memory_decoding_enabled(dev)) { > + ret = -EBUSY; > + goto err; > + } > + > + sizes = pci_rebar_get_possible_sizes(dev, resno); > + if (!sizes) { > + ret = -ENOTSUPP; > + goto err; > + } > + > + old = pci_rebar_get_current_size(dev, resno); > + if (old < 0) { > + ret = old; > + goto err; > + } > + > + while (sizes > 0) { > + i = __fls(sizes); > + size = pci_rebar_size_to_bytes(i); > + if (size * num_vfs <= pci_resource_len(dev, resno)) { > + if (i != old) { > + ret = pci_rebar_set_size(dev, resno, size); > + if (ret) > + goto err; > + > + pci_iov_resource_set_size(dev, resno, size); > + pci_iov_update_resource(dev, resno); > + } > + break; > + } > + sizes &= ~BIT(i); > + } > + > + pci_config_pm_runtime_put(dev); > + > + return; > + > +err: > + dev_WARN(&dev->dev, "Failed to extend %s: %d\n", > + pci_resource_name(dev, resno), ret); > + > + pci_config_pm_runtime_put(dev); > +} > + > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno) > +{ > + if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES]) > + pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs); > +} > + > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > +{ > + if (!pci_resource_is_iov(dev, resno)) { > + dev_WARN(&dev->dev, "%s is not an IOV resource\n", > + pci_resource_name(dev, resno)); > + > + return -ENODEV; > + } > + > + if (!pci_rebar_get_possible_sizes(dev, resno)) > + return -ENOTSUPP; > + > + if (!enable) > + pci_iov_resource_do_restore(dev, resno); > + > + dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable; > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(pci_iov_resource_extend); > + > static void pci_read_vf_config_common(struct pci_dev *virtfn) > { > struct pci_dev *physfn = virtfn->physfn; > @@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev, > const char *buf, size_t count) > { > struct pci_dev *pdev = to_pci_dev(dev); > - int ret = 0; > + int i, ret = 0; > u16 num_vfs; > > if (kstrtou16(buf, 0, &num_vfs) < 0) > @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev, > goto exit; > } > > + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > + if (pdev->sriov->rebar_extend[i]) > + pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs); > + } > + That sounds like a really bad idea to me. Basically the suggestion is here that the PCI subsystem should silently extend and shrink the VF BARs when the number of VFs change? Bjorn has the last word on that but I would say that instead the driver owning the PCIe device as hypervisor should resize the VF BARs to a desired size and that in turn restricts the number of VFs you can enable. Regards, Christian. > ret = pdev->driver->sriov_configure(pdev, num_vfs); > if (ret < 0) > goto exit; > @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos) > > static void sriov_release(struct pci_dev *dev) > { > + int i; > + > BUG_ON(dev->sriov->num_VFs); > > + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) > + pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES); > + > if (dev != dev->sriov->dev) > pci_dev_put(dev->sriov->dev); > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index e763b3fd4c7a2..47ed2633232aa 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -385,6 +385,7 @@ struct pci_sriov { > u16 subsystem_vendor; /* VF subsystem vendor */ > u16 subsystem_device; /* VF subsystem device */ > resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */ > + bool rebar_extend[PCI_SRIOV_NUM_BARS]; /* Resize VF BAR */ > bool drivers_autoprobe; /* Auto probing of VFs by driver */ > }; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 4cf89a4b4cbcf..c007119da7b3d 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); > int pci_sriov_get_totalvfs(struct pci_dev *dev); > int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn); > resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno); > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable); > void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe); > > /* Arch may override these (weak) */ > @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) > #define pci_sriov_configure_simple NULL > static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) > { return 0; } > +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > +{ return -ENODEV; } > static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } > #endif >
Hi Michał, kernel test robot noticed the following build warnings: [auto build test WARNING on pci/for-linus] [also build test WARNING on drm-xe/drm-xe-next drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11 next-20240920] [cannot apply to pci/next] [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/Micha-Winiarski/PCI-Add-support-for-VF-Resizable-Bar-extended-cap/20240920-064112 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus patch link: https://lore.kernel.org/r/20240919223557.1897608-3-michal.winiarski%40intel.com patch subject: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary config: arc-allnoconfig (https://download.01.org/0day-ci/archive/20240920/202409201854.z0daqyYE-lkp@intel.com/config) compiler: arc-elf-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201854.z0daqyYE-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/202409201854.z0daqyYE-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from drivers/pci/of.c:12: include/linux/pci.h: In function 'pci_iov_resource_extend': >> include/linux/pci.h:2421:10: warning: 'return' with a value, in function returning void [-Wreturn-type] 2421 | { return -ENODEV; } | ^ include/linux/pci.h:2420:20: note: declared here 2420 | static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) | ^~~~~~~~~~~~~~~~~~~~~~~ vim +/return +2421 include/linux/pci.h 2397 2398 static inline int pci_iov_sysfs_link(struct pci_dev *dev, 2399 struct pci_dev *virtfn, int id) 2400 { 2401 return -ENODEV; 2402 } 2403 static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id) 2404 { 2405 return -ENOSYS; 2406 } 2407 static inline void pci_iov_remove_virtfn(struct pci_dev *dev, 2408 int id) { } 2409 static inline void pci_disable_sriov(struct pci_dev *dev) { } 2410 static inline int pci_num_vf(struct pci_dev *dev) { return 0; } 2411 static inline int pci_vfs_assigned(struct pci_dev *dev) 2412 { return 0; } 2413 static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs) 2414 { return 0; } 2415 static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) 2416 { return 0; } 2417 #define pci_sriov_configure_simple NULL 2418 static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) 2419 { return 0; } 2420 static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > 2421 { return -ENODEV; } 2422 static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } 2423 #endif 2424
Hi Michał, kernel test robot noticed the following build warnings: [auto build test WARNING on pci/for-linus] [also build test WARNING on drm-xe/drm-xe-next drm/drm-next drm-exynos/exynos-drm-next drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-misc/drm-misc-next drm-tip/drm-tip linus/master v6.11 next-20240920] [cannot apply to pci/next] [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/Micha-Winiarski/PCI-Add-support-for-VF-Resizable-Bar-extended-cap/20240920-064112 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git for-linus patch link: https://lore.kernel.org/r/20240919223557.1897608-3-michal.winiarski%40intel.com patch subject: [PATCH v2 2/3] PCI: Allow extending VF BAR within original resource boundary config: arm-allnoconfig (https://download.01.org/0day-ci/archive/20240920/202409201934.yM9hVUai-lkp@intel.com/config) compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 8663a75fa2f31299ab8d1d90288d9df92aadee88) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201934.yM9hVUai-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/202409201934.yM9hVUai-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from arch/arm/mm/iomap.c:9: >> include/linux/pci.h:2421:3: warning: void function 'pci_iov_resource_extend' should not return a value [-Wreturn-mismatch] 2421 | { return -ENODEV; } | ^ ~~~~~~~ In file included from arch/arm/mm/iomap.c:9: In file included from include/linux/pci.h:2672: In file included from include/linux/dma-mapping.h:11: In file included from include/linux/scatterlist.h:8: In file included from include/linux/mm.h:2228: include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion] 514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_" | ~~~~~~~~~~~ ^ ~~~ 2 warnings generated. vim +/pci_iov_resource_extend +2421 include/linux/pci.h 2397 2398 static inline int pci_iov_sysfs_link(struct pci_dev *dev, 2399 struct pci_dev *virtfn, int id) 2400 { 2401 return -ENODEV; 2402 } 2403 static inline int pci_iov_add_virtfn(struct pci_dev *dev, int id) 2404 { 2405 return -ENOSYS; 2406 } 2407 static inline void pci_iov_remove_virtfn(struct pci_dev *dev, 2408 int id) { } 2409 static inline void pci_disable_sriov(struct pci_dev *dev) { } 2410 static inline int pci_num_vf(struct pci_dev *dev) { return 0; } 2411 static inline int pci_vfs_assigned(struct pci_dev *dev) 2412 { return 0; } 2413 static inline int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs) 2414 { return 0; } 2415 static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) 2416 { return 0; } 2417 #define pci_sriov_configure_simple NULL 2418 static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) 2419 { return 0; } 2420 static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > 2421 { return -ENODEV; } 2422 static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } 2423 #endif 2424
On Fri, 20 Sep 2024, Michał Winiarski wrote: > VF MMIO resource reservation, either created by system firmware and > inherited by Linux PCI subsystem or created by the subsystem itself, > contains enough space to fit the BAR of all SR-IOV Virtual Functions > that can potentially be created (total VFs supported by the device). > This can be leveraged when the device is exposing lower than optimal BAR > size as a default, allowing access to the entire resource when lower > number of VFs are created. > It is achieved by dynamically resizing the BAR to largest possible value > that allows to fit all newly created VFs within the original resource > boundary. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > drivers/pci/iov.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- > drivers/pci/pci.h | 1 + > include/linux/pci.h | 3 ++ > 3 files changed, 95 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index e8ccd2ae0f024..d88efbfa70e42 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) > return cmd & PCI_SRIOV_CTRL_MSE; > } > > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs) > +{ > + resource_size_t size; > + int ret, old, i; > + u32 sizes; > + > + pci_config_pm_runtime_get(dev); > + > + if (pci_iov_memory_decoding_enabled(dev)) { > + ret = -EBUSY; > + goto err; > + } > + > + sizes = pci_rebar_get_possible_sizes(dev, resno); > + if (!sizes) { > + ret = -ENOTSUPP; > + goto err; > + } > + > + old = pci_rebar_get_current_size(dev, resno); > + if (old < 0) { > + ret = old; > + goto err; > + } > + > + while (sizes > 0) { > + i = __fls(sizes); > + size = pci_rebar_size_to_bytes(i); > + if (size * num_vfs <= pci_resource_len(dev, resno)) { > + if (i != old) { > + ret = pci_rebar_set_size(dev, resno, size); > + if (ret) > + goto err; > + > + pci_iov_resource_set_size(dev, resno, size); > + pci_iov_update_resource(dev, resno); > + } > + break; > + } > + sizes &= ~BIT(i); > + } > + > + pci_config_pm_runtime_put(dev); > + > + return; > + > +err: > + dev_WARN(&dev->dev, "Failed to extend %s: %d\n", > + pci_resource_name(dev, resno), ret); Why do you use dev_WARN()? (analoguous to WARN_ON() / friends). I suppose you'd want to use pci_warn() instead. > + pci_config_pm_runtime_put(dev); > +} > + > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno) > +{ > + if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES]) > + pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs); > +} > + > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > +{ > + if (!pci_resource_is_iov(dev, resno)) { > + dev_WARN(&dev->dev, "%s is not an IOV resource\n", > + pci_resource_name(dev, resno)); pci_warn() ?
On Fri, Sep 20, 2024 at 02:30:00PM +0300, Ilpo Järvinen wrote: > On Fri, 20 Sep 2024, Michał Winiarski wrote: > > > VF MMIO resource reservation, either created by system firmware and > > inherited by Linux PCI subsystem or created by the subsystem itself, > > contains enough space to fit the BAR of all SR-IOV Virtual Functions > > that can potentially be created (total VFs supported by the device). > > This can be leveraged when the device is exposing lower than optimal BAR > > size as a default, allowing access to the entire resource when lower > > number of VFs are created. > > It is achieved by dynamically resizing the BAR to largest possible value > > that allows to fit all newly created VFs within the original resource > > boundary. > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > --- > > drivers/pci/iov.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- > > drivers/pci/pci.h | 1 + > > include/linux/pci.h | 3 ++ > > 3 files changed, 95 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > index e8ccd2ae0f024..d88efbfa70e42 100644 > > --- a/drivers/pci/iov.c > > +++ b/drivers/pci/iov.c > > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) > > return cmd & PCI_SRIOV_CTRL_MSE; > > } > > > > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs) > > +{ > > + resource_size_t size; > > + int ret, old, i; > > + u32 sizes; > > + > > + pci_config_pm_runtime_get(dev); > > + > > + if (pci_iov_memory_decoding_enabled(dev)) { > > + ret = -EBUSY; > > + goto err; > > + } > > + > > + sizes = pci_rebar_get_possible_sizes(dev, resno); > > + if (!sizes) { > > + ret = -ENOTSUPP; > > + goto err; > > + } > > + > > + old = pci_rebar_get_current_size(dev, resno); > > + if (old < 0) { > > + ret = old; > > + goto err; > > + } > > + > > + while (sizes > 0) { > > + i = __fls(sizes); > > + size = pci_rebar_size_to_bytes(i); > > + if (size * num_vfs <= pci_resource_len(dev, resno)) { > > + if (i != old) { > > + ret = pci_rebar_set_size(dev, resno, size); > > + if (ret) > > + goto err; > > + > > + pci_iov_resource_set_size(dev, resno, size); > > + pci_iov_update_resource(dev, resno); > > + } > > + break; > > + } > > + sizes &= ~BIT(i); > > + } > > + > > + pci_config_pm_runtime_put(dev); > > + > > + return; > > + > > +err: > > + dev_WARN(&dev->dev, "Failed to extend %s: %d\n", > > + pci_resource_name(dev, resno), ret); > > Why do you use dev_WARN()? (analoguous to WARN_ON() / friends). > > I suppose you'd want to use pci_warn() instead. This should never happen - but fair point, with something like panic_on_warn it would have more negative consequences. Let's downgrade it to pci_warn(). > > > + pci_config_pm_runtime_put(dev); > > +} > > + > > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno) > > +{ > > + if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES]) > > + pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs); > > +} > > + > > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > > +{ > > + if (!pci_resource_is_iov(dev, resno)) { > > + dev_WARN(&dev->dev, "%s is not an IOV resource\n", > > + pci_resource_name(dev, resno)); > > pci_warn() ? And this one is programmer error. But same as above, let's use pci_warn(). Thanks -Michał
On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote: > Am 20.09.24 um 00:35 schrieb Michał Winiarski: > > VF MMIO resource reservation, either created by system firmware and > > inherited by Linux PCI subsystem or created by the subsystem itself, > > contains enough space to fit the BAR of all SR-IOV Virtual Functions > > that can potentially be created (total VFs supported by the device). > > This can be leveraged when the device is exposing lower than optimal BAR > > size as a default, allowing access to the entire resource when lower > > number of VFs are created. > > It is achieved by dynamically resizing the BAR to largest possible value > > that allows to fit all newly created VFs within the original resource > > boundary. > > > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > > --- > > drivers/pci/iov.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- > > drivers/pci/pci.h | 1 + > > include/linux/pci.h | 3 ++ > > 3 files changed, 95 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > > index e8ccd2ae0f024..d88efbfa70e42 100644 > > --- a/drivers/pci/iov.c > > +++ b/drivers/pci/iov.c > > @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) > > return cmd & PCI_SRIOV_CTRL_MSE; > > } > > +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs) > > +{ > > + resource_size_t size; > > + int ret, old, i; > > + u32 sizes; > > + > > + pci_config_pm_runtime_get(dev); > > + > > + if (pci_iov_memory_decoding_enabled(dev)) { > > + ret = -EBUSY; > > + goto err; > > + } > > + > > + sizes = pci_rebar_get_possible_sizes(dev, resno); > > + if (!sizes) { > > + ret = -ENOTSUPP; > > + goto err; > > + } > > + > > + old = pci_rebar_get_current_size(dev, resno); > > + if (old < 0) { > > + ret = old; > > + goto err; > > + } > > + > > + while (sizes > 0) { > > + i = __fls(sizes); > > + size = pci_rebar_size_to_bytes(i); > > + if (size * num_vfs <= pci_resource_len(dev, resno)) { > > + if (i != old) { > > + ret = pci_rebar_set_size(dev, resno, size); > > + if (ret) > > + goto err; > > + > > + pci_iov_resource_set_size(dev, resno, size); > > + pci_iov_update_resource(dev, resno); > > + } > > + break; > > + } > > + sizes &= ~BIT(i); > > + } > > + > > + pci_config_pm_runtime_put(dev); > > + > > + return; > > + > > +err: > > + dev_WARN(&dev->dev, "Failed to extend %s: %d\n", > > + pci_resource_name(dev, resno), ret); > > + > > + pci_config_pm_runtime_put(dev); > > +} > > + > > +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno) > > +{ > > + if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES]) > > + pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs); > > +} > > + > > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > > +{ > > + if (!pci_resource_is_iov(dev, resno)) { > > + dev_WARN(&dev->dev, "%s is not an IOV resource\n", > > + pci_resource_name(dev, resno)); > > + > > + return -ENODEV; > > + } > > + > > + if (!pci_rebar_get_possible_sizes(dev, resno)) > > + return -ENOTSUPP; > > + > > + if (!enable) > > + pci_iov_resource_do_restore(dev, resno); > > + > > + dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable; > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(pci_iov_resource_extend); > > + > > static void pci_read_vf_config_common(struct pci_dev *virtfn) > > { > > struct pci_dev *physfn = virtfn->physfn; > > @@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev, > > const char *buf, size_t count) > > { > > struct pci_dev *pdev = to_pci_dev(dev); > > - int ret = 0; > > + int i, ret = 0; > > u16 num_vfs; > > if (kstrtou16(buf, 0, &num_vfs) < 0) > > @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev, > > goto exit; > > } > > + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { > > + if (pdev->sriov->rebar_extend[i]) > > + pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs); > > + } > > + > > That sounds like a really bad idea to me. > > Basically the suggestion is here that the PCI subsystem should silently > extend and shrink the VF BARs when the number of VFs change? Why do you think it's a bad idea? Everything is under PCI subsystem control and the driver in charge has to explicitly opt-in to this behavior on a per-BAR basis. > Bjorn has the last word on that but I would say that instead the driver > owning the PCIe device as hypervisor should resize the VF BARs to a desired > size and that in turn restricts the number of VFs you can enable. Then the PCI subsystem would silently change the driver_max_VFs (or new variable, as driver_max_VFs is under PF control, so it's either new var or checking VF BAR size in pci_sriov_set_totalvfs). It also means that we have to do the maths to calculate the new VF limit in both PCI subsystem and the caller. We can go this route as well - I just think it's cleaner to keep this all under PCI subsystem control. I'll keep the current behavior in v3, but I'm open to changing it. Thanks, -Michał > > Regards, > Christian. > > > ret = pdev->driver->sriov_configure(pdev, num_vfs); > > if (ret < 0) > > goto exit; > > @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos) > > static void sriov_release(struct pci_dev *dev) > > { > > + int i; > > + > > BUG_ON(dev->sriov->num_VFs); > > + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) > > + pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES); > > + > > if (dev != dev->sriov->dev) > > pci_dev_put(dev->sriov->dev); > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index e763b3fd4c7a2..47ed2633232aa 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -385,6 +385,7 @@ struct pci_sriov { > > u16 subsystem_vendor; /* VF subsystem vendor */ > > u16 subsystem_device; /* VF subsystem device */ > > resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */ > > + bool rebar_extend[PCI_SRIOV_NUM_BARS]; /* Resize VF BAR */ > > bool drivers_autoprobe; /* Auto probing of VFs by driver */ > > }; > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 4cf89a4b4cbcf..c007119da7b3d 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); > > int pci_sriov_get_totalvfs(struct pci_dev *dev); > > int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn); > > resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno); > > +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable); > > void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe); > > /* Arch may override these (weak) */ > > @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) > > #define pci_sriov_configure_simple NULL > > static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) > > { return 0; } > > +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) > > +{ return -ENODEV; } > > static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } > > #endif >
Am 10.10.24 um 10:59 schrieb Michał Winiarski: > On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote: >> Am 20.09.24 um 00:35 schrieb Michał Winiarski: >>> [SNIP] >>> @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev, >>> goto exit; >>> } >>> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { >>> + if (pdev->sriov->rebar_extend[i]) >>> + pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs); >>> + } >>> + >> That sounds like a really bad idea to me. >> >> Basically the suggestion is here that the PCI subsystem should silently >> extend and shrink the VF BARs when the number of VFs change? > Why do you think it's a bad idea? Everything is under PCI subsystem > control and the driver in charge has to explicitly opt-in to this > behavior on a per-BAR basis. And exactly that's a bad idea. The PCI subsystem shouldn't control this, the driver should. At least for some devices we have tons of interactions with ACPI and EFI. Only the driver does know for example when platform drivers which might be in the way for a resize have been unloaded. From the past experience BAR resize should only be triggered by the driver and never from the PCI subsystem while scanning the bus or probing devices. >> Bjorn has the last word on that but I would say that instead the driver >> owning the PCIe device as hypervisor should resize the VF BARs to a desired >> size and that in turn restricts the number of VFs you can enable. > Then the PCI subsystem would silently change the driver_max_VFs (or new > variable, as driver_max_VFs is under PF control, so it's either new var > or checking VF BAR size in pci_sriov_set_totalvfs). Nope, the PCI subsystem should not magically adjust anything. What should happen instead is that the driver would call pci_enable_sriov() with the number of virtual functions to enable and the PCI subsystem then validates that number and return -EINVAL or -ENOSPC if it won't work. > It also means that we have to do the maths to calculate the new VF limit > in both PCI subsystem and the caller. Well the point is that those calculations are different. What the subsystem does is to validate if with the number of requested virtual functions the necessary resources will fit into the allocate space. What the driver does previously is to either change the allocate space or calculate the other way around and determine the maximum virtual functions from the space available. > We can go this route as well - I just think it's cleaner to keep this > all under PCI subsystem control. I think that would be much cleaner, especially the PCI subsystem shouldn't adjust any values given from the driver or even more general overrule decisions the driver made. Instead proper error codes should be returned if some values don't make sense or the subsystem isn't able to move around BARs currently in use etc... Regards, Christian. > > I'll keep the current behavior in v3, but I'm open to changing it. > > Thanks, > -Michał > >> Regards, >> Christian. >> >>> ret = pdev->driver->sriov_configure(pdev, num_vfs); >>> if (ret < 0) >>> goto exit; >>> @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos) >>> static void sriov_release(struct pci_dev *dev) >>> { >>> + int i; >>> + >>> BUG_ON(dev->sriov->num_VFs); >>> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) >>> + pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES); >>> + >>> if (dev != dev->sriov->dev) >>> pci_dev_put(dev->sriov->dev); >>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >>> index e763b3fd4c7a2..47ed2633232aa 100644 >>> --- a/drivers/pci/pci.h >>> +++ b/drivers/pci/pci.h >>> @@ -385,6 +385,7 @@ struct pci_sriov { >>> u16 subsystem_vendor; /* VF subsystem vendor */ >>> u16 subsystem_device; /* VF subsystem device */ >>> resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */ >>> + bool rebar_extend[PCI_SRIOV_NUM_BARS]; /* Resize VF BAR */ >>> bool drivers_autoprobe; /* Auto probing of VFs by driver */ >>> }; >>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>> index 4cf89a4b4cbcf..c007119da7b3d 100644 >>> --- a/include/linux/pci.h >>> +++ b/include/linux/pci.h >>> @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); >>> int pci_sriov_get_totalvfs(struct pci_dev *dev); >>> int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn); >>> resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno); >>> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable); >>> void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe); >>> /* Arch may override these (weak) */ >>> @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) >>> #define pci_sriov_configure_simple NULL >>> static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) >>> { return 0; } >>> +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) >>> +{ return -ENODEV; } >>> static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } >>> #endif
Re-sending this as text from my private mail account since the AMD servers now seem to convert everything to HTML ^^. Christian. Am 11.10.24 um 10:57 schrieb Christian König: > Am 10.10.24 um 10:59 schrieb Michał Winiarski: >> On Fri, Sep 20, 2024 at 12:07:34PM +0200, Christian König wrote: >>> Am 20.09.24 um 00:35 schrieb Michał Winiarski: >>>> [SNIP] >>>> @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev, >>>> goto exit; >>>> } >>>> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { >>>> + if (pdev->sriov->rebar_extend[i]) >>>> + pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs); >>>> + } >>>> + >>> That sounds like a really bad idea to me. >>> >>> Basically the suggestion is here that the PCI subsystem should silently >>> extend and shrink the VF BARs when the number of VFs change? >> Why do you think it's a bad idea? Everything is under PCI subsystem >> control and the driver in charge has to explicitly opt-in to this >> behavior on a per-BAR basis. > > And exactly that's a bad idea. The PCI subsystem shouldn't control > this, the driver should. > > At least for some devices we have tons of interactions with ACPI and > EFI. Only the driver does know for example when platform drivers which > might be in the way for a resize have been unloaded. > > From the past experience BAR resize should only be triggered by the > driver and never from the PCI subsystem while scanning the bus or > probing devices. > >>> Bjorn has the last word on that but I would say that instead the driver >>> owning the PCIe device as hypervisor should resize the VF BARs to a desired >>> size and that in turn restricts the number of VFs you can enable. >> Then the PCI subsystem would silently change the driver_max_VFs (or new >> variable, as driver_max_VFs is under PF control, so it's either new var >> or checking VF BAR size in pci_sriov_set_totalvfs). > > Nope, the PCI subsystem should not magically adjust anything. > > What should happen instead is that the driver would call > pci_enable_sriov() with the number of virtual functions to enable and > the PCI subsystem then validates that number and return -EINVAL or > -ENOSPC if it won't work. > >> It also means that we have to do the maths to calculate the new VF limit >> in both PCI subsystem and the caller. > > Well the point is that those calculations are different. > > What the subsystem does is to validate if with the number of requested > virtual functions the necessary resources will fit into the allocate > space. > > What the driver does previously is to either change the allocate space > or calculate the other way around and determine the maximum virtual > functions from the space available. > >> We can go this route as well - I just think it's cleaner to keep this >> all under PCI subsystem control. > > I think that would be much cleaner, especially the PCI subsystem > shouldn't adjust any values given from the driver or even more general > overrule decisions the driver made. > > Instead proper error codes should be returned if some values don't > make sense or the subsystem isn't able to move around BARs currently > in use etc... > > Regards, > Christian. > >> I'll keep the current behavior in v3, but I'm open to changing it. >> >> Thanks, >> -Michał >> >>> Regards, >>> Christian. >>> >>>> ret = pdev->driver->sriov_configure(pdev, num_vfs); >>>> if (ret < 0) >>>> goto exit; >>>> @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos) >>>> static void sriov_release(struct pci_dev *dev) >>>> { >>>> + int i; >>>> + >>>> BUG_ON(dev->sriov->num_VFs); >>>> + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) >>>> + pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES); >>>> + >>>> if (dev != dev->sriov->dev) >>>> pci_dev_put(dev->sriov->dev); >>>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >>>> index e763b3fd4c7a2..47ed2633232aa 100644 >>>> --- a/drivers/pci/pci.h >>>> +++ b/drivers/pci/pci.h >>>> @@ -385,6 +385,7 @@ struct pci_sriov { >>>> u16 subsystem_vendor; /* VF subsystem vendor */ >>>> u16 subsystem_device; /* VF subsystem device */ >>>> resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */ >>>> + bool rebar_extend[PCI_SRIOV_NUM_BARS]; /* Resize VF BAR */ >>>> bool drivers_autoprobe; /* Auto probing of VFs by driver */ >>>> }; >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>> index 4cf89a4b4cbcf..c007119da7b3d 100644 >>>> --- a/include/linux/pci.h >>>> +++ b/include/linux/pci.h >>>> @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); >>>> int pci_sriov_get_totalvfs(struct pci_dev *dev); >>>> int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn); >>>> resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno); >>>> +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable); >>>> void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe); >>>> /* Arch may override these (weak) */ >>>> @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) >>>> #define pci_sriov_configure_simple NULL >>>> static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) >>>> { return 0; } >>>> +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) >>>> +{ return -ENODEV; } >>>> static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } >>>> #endif >
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index e8ccd2ae0f024..d88efbfa70e42 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -181,6 +181,86 @@ bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) return cmd & PCI_SRIOV_CTRL_MSE; } +static void pci_iov_resource_do_extend(struct pci_dev *dev, int resno, u16 num_vfs) +{ + resource_size_t size; + int ret, old, i; + u32 sizes; + + pci_config_pm_runtime_get(dev); + + if (pci_iov_memory_decoding_enabled(dev)) { + ret = -EBUSY; + goto err; + } + + sizes = pci_rebar_get_possible_sizes(dev, resno); + if (!sizes) { + ret = -ENOTSUPP; + goto err; + } + + old = pci_rebar_get_current_size(dev, resno); + if (old < 0) { + ret = old; + goto err; + } + + while (sizes > 0) { + i = __fls(sizes); + size = pci_rebar_size_to_bytes(i); + if (size * num_vfs <= pci_resource_len(dev, resno)) { + if (i != old) { + ret = pci_rebar_set_size(dev, resno, size); + if (ret) + goto err; + + pci_iov_resource_set_size(dev, resno, size); + pci_iov_update_resource(dev, resno); + } + break; + } + sizes &= ~BIT(i); + } + + pci_config_pm_runtime_put(dev); + + return; + +err: + dev_WARN(&dev->dev, "Failed to extend %s: %d\n", + pci_resource_name(dev, resno), ret); + + pci_config_pm_runtime_put(dev); +} + +static void pci_iov_resource_do_restore(struct pci_dev *dev, int resno) +{ + if (dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES]) + pci_iov_resource_do_extend(dev, resno, dev->sriov->total_VFs); +} + +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) +{ + if (!pci_resource_is_iov(dev, resno)) { + dev_WARN(&dev->dev, "%s is not an IOV resource\n", + pci_resource_name(dev, resno)); + + return -ENODEV; + } + + if (!pci_rebar_get_possible_sizes(dev, resno)) + return -ENOTSUPP; + + if (!enable) + pci_iov_resource_do_restore(dev, resno); + + dev->sriov->rebar_extend[resno - PCI_IOV_RESOURCES] = enable; + + return 0; +} +EXPORT_SYMBOL_GPL(pci_iov_resource_extend); + static void pci_read_vf_config_common(struct pci_dev *virtfn) { struct pci_dev *physfn = virtfn->physfn; @@ -445,7 +525,7 @@ static ssize_t sriov_numvfs_store(struct device *dev, const char *buf, size_t count) { struct pci_dev *pdev = to_pci_dev(dev); - int ret = 0; + int i, ret = 0; u16 num_vfs; if (kstrtou16(buf, 0, &num_vfs) < 0) @@ -487,6 +567,11 @@ static ssize_t sriov_numvfs_store(struct device *dev, goto exit; } + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) { + if (pdev->sriov->rebar_extend[i]) + pci_iov_resource_do_extend(pdev, i + PCI_IOV_RESOURCES, num_vfs); + } + ret = pdev->driver->sriov_configure(pdev, num_vfs); if (ret < 0) goto exit; @@ -881,8 +966,13 @@ static int sriov_init(struct pci_dev *dev, int pos) static void sriov_release(struct pci_dev *dev) { + int i; + BUG_ON(dev->sriov->num_VFs); + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) + pci_iov_resource_do_restore(dev, i + PCI_IOV_RESOURCES); + if (dev != dev->sriov->dev) pci_dev_put(dev->sriov->dev); diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index e763b3fd4c7a2..47ed2633232aa 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -385,6 +385,7 @@ struct pci_sriov { u16 subsystem_vendor; /* VF subsystem vendor */ u16 subsystem_device; /* VF subsystem device */ resource_size_t barsz[PCI_SRIOV_NUM_BARS]; /* VF BAR size */ + bool rebar_extend[PCI_SRIOV_NUM_BARS]; /* Resize VF BAR */ bool drivers_autoprobe; /* Auto probing of VFs by driver */ }; diff --git a/include/linux/pci.h b/include/linux/pci.h index 4cf89a4b4cbcf..c007119da7b3d 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -2364,6 +2364,7 @@ int pci_sriov_set_totalvfs(struct pci_dev *dev, u16 numvfs); int pci_sriov_get_totalvfs(struct pci_dev *dev); int pci_sriov_configure_simple(struct pci_dev *dev, int nr_virtfn); resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno); +int pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable); void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe); /* Arch may override these (weak) */ @@ -2416,6 +2417,8 @@ static inline int pci_sriov_get_totalvfs(struct pci_dev *dev) #define pci_sriov_configure_simple NULL static inline resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) { return 0; } +static inline void pci_iov_resource_extend(struct pci_dev *dev, int resno, bool enable) +{ return -ENODEV; } static inline void pci_vf_drivers_autoprobe(struct pci_dev *dev, bool probe) { } #endif
VF MMIO resource reservation, either created by system firmware and inherited by Linux PCI subsystem or created by the subsystem itself, contains enough space to fit the BAR of all SR-IOV Virtual Functions that can potentially be created (total VFs supported by the device). This can be leveraged when the device is exposing lower than optimal BAR size as a default, allowing access to the entire resource when lower number of VFs are created. It is achieved by dynamically resizing the BAR to largest possible value that allows to fit all newly created VFs within the original resource boundary. Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> --- drivers/pci/iov.c | 92 ++++++++++++++++++++++++++++++++++++++++++++- drivers/pci/pci.h | 1 + include/linux/pci.h | 3 ++ 3 files changed, 95 insertions(+), 1 deletion(-)