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

["Christian König" <christian.koenig@amd.com>]

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