| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <15a36075-2800-4274-a404-402ceafde5a0@amd.com>
Date: Fri, 20 Sep 2024 11:57:34 +0200
From: Christian König <christian.koenig@....com>
To: Michał Winiarski <michal.winiarski@...el.com>,
linux-pci@...r.kernel.org, intel-xe@...ts.freedesktop.org,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
Bjorn Helgaas <bhelgaas@...gle.com>, Krzysztof Wilczyński
<kw@...ux.com>
Cc: Rodrigo Vivi <rodrigo.vivi@...el.com>,
Michal Wajdeczko <michal.wajdeczko@...el.com>,
Lucas De Marchi <lucas.demarchi@...el.com>,
Thomas Hellström <thomas.hellstrom@...ux.intel.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
Matt Roper <matthew.d.roper@...el.com>
Subject: Re: [PATCH v2 1/3] PCI: Add support for VF Resizable Bar extended cap
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@...el.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 */
Powered by blists - more mailing lists