On Fri, Sep 20, 2024 at 11:57:34AM +0200, Christian König wrote:
> 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@xxxxxxxxx>
> > ---
> > 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.
I'll split it into separate patch that's changing the existing checks.
>
> Additional to that please code that something like "return resno >=...." the
> extra if just increases the number of lines without adding any value.
Ok.
>
> > +
> > +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.
I'll split it into separate patch.
>
> > + 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.
Ok.
>
> > + 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.
Ok.
Thanks,
-Michał
>
> 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 */
>
