[PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap

Michał Winiarski michal.winiarski at intel.com
Thu Oct 10 08:46:39 UTC 2024


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 at 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.

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 */
> 


More information about the dri-devel mailing list