| Message ID | 20240919223557.1897608-2-michal.winiarski@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: VF resizable BAR | expand |
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-20240919] [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-2-michal.winiarski%40intel.com patch subject: [PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap config: x86_64-kexec (https://download.01.org/0day-ci/archive/20240920/202409201629.QlC0MRbn-lkp@intel.com/config) compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240920/202409201629.QlC0MRbn-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/202409201629.QlC0MRbn-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/pci/pci.c:1920:20: warning: variable 'res' set but not used [-Wunused-but-set-variable] 1920 | struct resource *res; | ^ 1 warning generated. vim +/res +1920 drivers/pci/pci.c 1903 1904 static void pci_restore_vf_rebar_state(struct pci_dev *pdev) 1905 { 1906 unsigned int pos, nbars, i; 1907 u32 ctrl; 1908 1909 if (!pdev->is_physfn) 1910 return; 1911 1912 pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR); 1913 if (!pos) 1914 return; 1915 1916 pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); 1917 nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl); 1918 1919 for (i = 0; i < nbars; i++, pos += 8) { > 1920 struct resource *res; 1921 int bar_idx, size; 1922 1923 pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); 1924 bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX; 1925 res = pdev->resource + bar_idx; 1926 size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]); 1927 ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE; 1928 ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size); 1929 pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl); 1930 } 1931 } 1932
Am 20.09.24 um 00:35 schrieb Michał Winiarski: > Similar to regular resizable BAR, VF BAR can also be resized. > The structures are very similar, which means we can reuse most of the > implementation. See PCIe r4.0, sec 9.3.7.4. > > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> > --- > drivers/pci/iov.c | 28 ++++++++++++++++++++++ > drivers/pci/pci.c | 40 ++++++++++++++++++++++++++++++- > drivers/pci/pci.h | 14 ++++++++++- > drivers/pci/setup-res.c | 44 ++++++++++++++++++++++++++++++----- > include/uapi/linux/pci_regs.h | 1 + > 5 files changed, 119 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c > index aaa33e8dc4c97..e8ccd2ae0f024 100644 > --- a/drivers/pci/iov.c > +++ b/drivers/pci/iov.c > @@ -153,6 +153,34 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) > return dev->sriov->barsz[resno - PCI_IOV_RESOURCES]; > } > > +bool pci_resource_is_iov(struct pci_dev *dev, int resno) > +{ > + if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END) > + return true; > + > + return false; > +} When you want to generalize that check you should probably but it in a header and change the existing checks in pci.h and setup-res.c as well. Otherwise I don't really see the value in having a separate function. Additional to that please code that something like "return resno >=...." the extra if just increases the number of lines without adding any value. > + > +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size) > +{ > + if (!pci_resource_is_iov(dev, resno)) { > + dev_WARN(&dev->dev, "%s is not an IOV resource\n", > + pci_resource_name(dev, resno)); > + return; > + } > + > + dev->sriov->barsz[resno - PCI_IOV_RESOURCES] = size; > +} > + > +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) > +{ > + u16 cmd; > + > + pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd); > + > + return cmd & PCI_SRIOV_CTRL_MSE; > +} > + > static void pci_read_vf_config_common(struct pci_dev *virtfn) > { > struct pci_dev *physfn = virtfn->physfn; > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index ffaaca0978cbc..d4522e365e7ba 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -1901,6 +1901,35 @@ static void pci_restore_rebar_state(struct pci_dev *pdev) > } > } > > +static void pci_restore_vf_rebar_state(struct pci_dev *pdev) > +{ > + unsigned int pos, nbars, i; > + u32 ctrl; > + > + if (!pdev->is_physfn) > + return; > + > + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR); > + if (!pos) > + return; > + > + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); > + nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl); > + > + for (i = 0; i < nbars; i++, pos += 8) { > + struct resource *res; > + int bar_idx, size; > + > + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); > + bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX; > + res = pdev->resource + bar_idx; The variable res seems to be unused. In general I think you should split up the patch into restoring the VF rebar state on resume and implementing the new resize API. > + size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]); > + ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE; > + ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size); > + pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl); > + } > +} > + > /** > * pci_restore_state - Restore the saved state of a PCI device > * @dev: PCI device that we're dealing with > @@ -1916,6 +1945,7 @@ void pci_restore_state(struct pci_dev *dev) > pci_restore_ats_state(dev); > pci_restore_vc_state(dev); > pci_restore_rebar_state(dev); > + pci_restore_vf_rebar_state(dev); > pci_restore_dpc_state(dev); > pci_restore_ptm_state(dev); > > @@ -3703,10 +3733,18 @@ void pci_acs_init(struct pci_dev *dev) > */ > static int pci_rebar_find_pos(struct pci_dev *pdev, int bar) > { > + int cap = PCI_EXT_CAP_ID_REBAR; > unsigned int pos, nbars, i; > u32 ctrl; > > - pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR); > +#ifdef CONFIG_PCI_IOV > + if (pci_resource_is_iov(pdev, bar)) { > + cap = PCI_EXT_CAP_ID_VF_REBAR; > + bar -= PCI_IOV_RESOURCES; > + } > +#endif > + > + pos = pci_find_ext_capability(pdev, cap); > if (!pos) > return -ENOTSUPP; > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 79c8398f39384..e763b3fd4c7a2 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -556,6 +556,9 @@ void pci_restore_iov_state(struct pci_dev *dev); > int pci_iov_bus_range(struct pci_bus *bus); > extern const struct attribute_group sriov_pf_dev_attr_group; > extern const struct attribute_group sriov_vf_dev_attr_group; > +bool pci_resource_is_iov(struct pci_dev *dev, int resno); > +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev); > +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size); > #else > static inline int pci_iov_init(struct pci_dev *dev) > { > @@ -568,7 +571,16 @@ static inline int pci_iov_bus_range(struct pci_bus *bus) > { > return 0; > } > - > +static inline bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) > +{ > + return false; > +} > +static inline bool pci_resource_is_iov(struct pci_dev *dev, int resno) > +{ > + return false; > +} > +static inline void pci_iov_resource_set_size(struct pci_dev *dev, int resno, > + resource_size_t size) { } > #endif /* CONFIG_PCI_IOV */ > > #ifdef CONFIG_PCIE_PTM > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c > index c6d933ddfd464..87a952a114f38 100644 > --- a/drivers/pci/setup-res.c > +++ b/drivers/pci/setup-res.c > @@ -427,13 +427,44 @@ void pci_release_resource(struct pci_dev *dev, int resno) > } > EXPORT_SYMBOL(pci_release_resource); > > +static bool pci_memory_decoding_enabled(struct pci_dev *dev) > +{ I don't really see the value in making it a separate function, just keep the check inside the only caller. > + u16 cmd; > + > + pci_read_config_word(dev, PCI_COMMAND, &cmd); > + > + return cmd & PCI_COMMAND_MEMORY; > +} > + > +static int pci_resize_check_memory_decoding(struct pci_dev *dev, int resno) Also doesn't look like much value in having that a separate function. > +{ > + if (!pci_resource_is_iov(dev, resno) && pci_memory_decoding_enabled(dev)) > + return -EBUSY; > + else if (pci_resource_is_iov(dev, resno) && pci_iov_memory_decoding_enabled(dev)) > + return -EBUSY; Well that is coded as ugly as it could be. I strongly suggest to not call pci_resource_is_iov() twice and to move the -EBUSY return code outside of the function (if you really want a separate function for that). E.g. something like "bool pci_resize_is_decoding_enabled(...)" and then "if (pci_resize_is_decoding_enabled(...)) return -EBUSY;" in the caller. Regards, Christian. > + > + return 0; > +} > + > +static void pci_resize_resource_set_size(struct pci_dev *dev, int resno, int size) > +{ > + resource_size_t res_size = pci_rebar_size_to_bytes(size); > + struct resource *res = dev->resource + resno; > + > + if (!pci_resource_is_iov(dev, resno)) { > + res->end = res->start + res_size - 1; > + } else { > + res->end = res->start + res_size * pci_sriov_get_totalvfs(dev) - 1; > + pci_iov_resource_set_size(dev, resno, res_size); > + } > +} > + > int pci_resize_resource(struct pci_dev *dev, int resno, int size) > { > struct resource *res = dev->resource + resno; > struct pci_host_bridge *host; > int old, ret; > u32 sizes; > - u16 cmd; > > /* Check if we must preserve the firmware's resource assignment */ > host = pci_find_host_bridge(dev->bus); > @@ -444,9 +475,9 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) > if (!(res->flags & IORESOURCE_UNSET)) > return -EBUSY; > > - pci_read_config_word(dev, PCI_COMMAND, &cmd); > - if (cmd & PCI_COMMAND_MEMORY) > - return -EBUSY; > + ret = pci_resize_check_memory_decoding(dev, resno); > + if (ret) > + return ret; > > sizes = pci_rebar_get_possible_sizes(dev, resno); > if (!sizes) > @@ -463,7 +494,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) > if (ret) > return ret; > > - res->end = res->start + pci_rebar_size_to_bytes(size) - 1; > + pci_resize_resource_set_size(dev, resno, size); > > /* Check if the new config works by trying to assign everything. */ > if (dev->bus->self) { > @@ -475,7 +506,8 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) > > error_resize: > pci_rebar_set_size(dev, resno, old); > - res->end = res->start + pci_rebar_size_to_bytes(old) - 1; > + pci_resize_resource_set_size(dev, resno, old); > + > return ret; > } > EXPORT_SYMBOL(pci_resize_resource); > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h > index 94c00996e633e..cb010008c6bb3 100644 > --- a/include/uapi/linux/pci_regs.h > +++ b/include/uapi/linux/pci_regs.h > @@ -738,6 +738,7 @@ > #define PCI_EXT_CAP_ID_L1SS 0x1E /* L1 PM Substates */ > #define PCI_EXT_CAP_ID_PTM 0x1F /* Precision Time Measurement */ > #define PCI_EXT_CAP_ID_DVSEC 0x23 /* Designated Vendor-Specific */ > +#define PCI_EXT_CAP_ID_VF_REBAR 0x24 /* VF Resizable BAR */ > #define PCI_EXT_CAP_ID_DLF 0x25 /* Data Link Feature */ > #define PCI_EXT_CAP_ID_PL_16GT 0x26 /* Physical Layer 16.0 GT/s */ > #define PCI_EXT_CAP_ID_PL_32GT 0x2A /* Physical Layer 32.0 GT/s */
diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index aaa33e8dc4c97..e8ccd2ae0f024 100644 --- a/drivers/pci/iov.c +++ b/drivers/pci/iov.c @@ -153,6 +153,34 @@ resource_size_t pci_iov_resource_size(struct pci_dev *dev, int resno) return dev->sriov->barsz[resno - PCI_IOV_RESOURCES]; } +bool pci_resource_is_iov(struct pci_dev *dev, int resno) +{ + if (resno >= PCI_IOV_RESOURCES && resno <= PCI_IOV_RESOURCE_END) + return true; + + return false; +} + +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size) +{ + if (!pci_resource_is_iov(dev, resno)) { + dev_WARN(&dev->dev, "%s is not an IOV resource\n", + pci_resource_name(dev, resno)); + return; + } + + dev->sriov->barsz[resno - PCI_IOV_RESOURCES] = size; +} + +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) +{ + u16 cmd; + + pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd); + + return cmd & PCI_SRIOV_CTRL_MSE; +} + static void pci_read_vf_config_common(struct pci_dev *virtfn) { struct pci_dev *physfn = virtfn->physfn; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index ffaaca0978cbc..d4522e365e7ba 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -1901,6 +1901,35 @@ static void pci_restore_rebar_state(struct pci_dev *pdev) } } +static void pci_restore_vf_rebar_state(struct pci_dev *pdev) +{ + unsigned int pos, nbars, i; + u32 ctrl; + + if (!pdev->is_physfn) + return; + + pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_VF_REBAR); + if (!pos) + return; + + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); + nbars = FIELD_GET(PCI_REBAR_CTRL_NBAR_MASK, ctrl); + + for (i = 0; i < nbars; i++, pos += 8) { + struct resource *res; + int bar_idx, size; + + pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl); + bar_idx = ctrl & PCI_REBAR_CTRL_BAR_IDX; + res = pdev->resource + bar_idx; + size = pci_rebar_bytes_to_size(pdev->sriov->barsz[bar_idx]); + ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE; + ctrl |= FIELD_PREP(PCI_REBAR_CTRL_BAR_SIZE, size); + pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl); + } +} + /** * pci_restore_state - Restore the saved state of a PCI device * @dev: PCI device that we're dealing with @@ -1916,6 +1945,7 @@ void pci_restore_state(struct pci_dev *dev) pci_restore_ats_state(dev); pci_restore_vc_state(dev); pci_restore_rebar_state(dev); + pci_restore_vf_rebar_state(dev); pci_restore_dpc_state(dev); pci_restore_ptm_state(dev); @@ -3703,10 +3733,18 @@ void pci_acs_init(struct pci_dev *dev) */ static int pci_rebar_find_pos(struct pci_dev *pdev, int bar) { + int cap = PCI_EXT_CAP_ID_REBAR; unsigned int pos, nbars, i; u32 ctrl; - pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR); +#ifdef CONFIG_PCI_IOV + if (pci_resource_is_iov(pdev, bar)) { + cap = PCI_EXT_CAP_ID_VF_REBAR; + bar -= PCI_IOV_RESOURCES; + } +#endif + + pos = pci_find_ext_capability(pdev, cap); if (!pos) return -ENOTSUPP; diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 79c8398f39384..e763b3fd4c7a2 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -556,6 +556,9 @@ void pci_restore_iov_state(struct pci_dev *dev); int pci_iov_bus_range(struct pci_bus *bus); extern const struct attribute_group sriov_pf_dev_attr_group; extern const struct attribute_group sriov_vf_dev_attr_group; +bool pci_resource_is_iov(struct pci_dev *dev, int resno); +bool pci_iov_memory_decoding_enabled(struct pci_dev *dev); +void pci_iov_resource_set_size(struct pci_dev *dev, int resno, resource_size_t size); #else static inline int pci_iov_init(struct pci_dev *dev) { @@ -568,7 +571,16 @@ static inline int pci_iov_bus_range(struct pci_bus *bus) { return 0; } - +static inline bool pci_iov_memory_decoding_enabled(struct pci_dev *dev) +{ + return false; +} +static inline bool pci_resource_is_iov(struct pci_dev *dev, int resno) +{ + return false; +} +static inline void pci_iov_resource_set_size(struct pci_dev *dev, int resno, + resource_size_t size) { } #endif /* CONFIG_PCI_IOV */ #ifdef CONFIG_PCIE_PTM diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index c6d933ddfd464..87a952a114f38 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -427,13 +427,44 @@ void pci_release_resource(struct pci_dev *dev, int resno) } EXPORT_SYMBOL(pci_release_resource); +static bool pci_memory_decoding_enabled(struct pci_dev *dev) +{ + u16 cmd; + + pci_read_config_word(dev, PCI_COMMAND, &cmd); + + return cmd & PCI_COMMAND_MEMORY; +} + +static int pci_resize_check_memory_decoding(struct pci_dev *dev, int resno) +{ + if (!pci_resource_is_iov(dev, resno) && pci_memory_decoding_enabled(dev)) + return -EBUSY; + else if (pci_resource_is_iov(dev, resno) && pci_iov_memory_decoding_enabled(dev)) + return -EBUSY; + + return 0; +} + +static void pci_resize_resource_set_size(struct pci_dev *dev, int resno, int size) +{ + resource_size_t res_size = pci_rebar_size_to_bytes(size); + struct resource *res = dev->resource + resno; + + if (!pci_resource_is_iov(dev, resno)) { + res->end = res->start + res_size - 1; + } else { + res->end = res->start + res_size * pci_sriov_get_totalvfs(dev) - 1; + pci_iov_resource_set_size(dev, resno, res_size); + } +} + int pci_resize_resource(struct pci_dev *dev, int resno, int size) { struct resource *res = dev->resource + resno; struct pci_host_bridge *host; int old, ret; u32 sizes; - u16 cmd; /* Check if we must preserve the firmware's resource assignment */ host = pci_find_host_bridge(dev->bus); @@ -444,9 +475,9 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) if (!(res->flags & IORESOURCE_UNSET)) return -EBUSY; - pci_read_config_word(dev, PCI_COMMAND, &cmd); - if (cmd & PCI_COMMAND_MEMORY) - return -EBUSY; + ret = pci_resize_check_memory_decoding(dev, resno); + if (ret) + return ret; sizes = pci_rebar_get_possible_sizes(dev, resno); if (!sizes) @@ -463,7 +494,7 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) if (ret) return ret; - res->end = res->start + pci_rebar_size_to_bytes(size) - 1; + pci_resize_resource_set_size(dev, resno, size); /* Check if the new config works by trying to assign everything. */ if (dev->bus->self) { @@ -475,7 +506,8 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size) error_resize: pci_rebar_set_size(dev, resno, old); - res->end = res->start + pci_rebar_size_to_bytes(old) - 1; + pci_resize_resource_set_size(dev, resno, old); + return ret; } EXPORT_SYMBOL(pci_resize_resource); diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h index 94c00996e633e..cb010008c6bb3 100644 --- a/include/uapi/linux/pci_regs.h +++ b/include/uapi/linux/pci_regs.h @@ -738,6 +738,7 @@ #define PCI_EXT_CAP_ID_L1SS 0x1E /* L1 PM Substates */ #define PCI_EXT_CAP_ID_PTM 0x1F /* Precision Time Measurement */ #define PCI_EXT_CAP_ID_DVSEC 0x23 /* Designated Vendor-Specific */ +#define PCI_EXT_CAP_ID_VF_REBAR 0x24 /* VF Resizable BAR */ #define PCI_EXT_CAP_ID_DLF 0x25 /* Data Link Feature */ #define PCI_EXT_CAP_ID_PL_16GT 0x26 /* Physical Layer 16.0 GT/s */ #define PCI_EXT_CAP_ID_PL_32GT 0x2A /* Physical Layer 32.0 GT/s */
Similar to regular resizable BAR, VF BAR can also be resized. The structures are very similar, which means we can reuse most of the implementation. See PCIe r4.0, sec 9.3.7.4. Signed-off-by: Michał Winiarski <michal.winiarski@intel.com> --- drivers/pci/iov.c | 28 ++++++++++++++++++++++ drivers/pci/pci.c | 40 ++++++++++++++++++++++++++++++- drivers/pci/pci.h | 14 ++++++++++- drivers/pci/setup-res.c | 44 ++++++++++++++++++++++++++++++----- include/uapi/linux/pci_regs.h | 1 + 5 files changed, 119 insertions(+), 8 deletions(-)