Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1630507 > unrolled thread

Resizeable PCI BAR support V4

Back to article view | Back to linux.kernel

Contents

  Resizeable PCI BAR support V4 Christian König <deathsimple@vodafone.de> - 2017-04-25 15:30 +0200
    [PATCH 1/4] PCI: add resizeable BAR infrastructure v4 Christian König <deathsimple@vodafone.de> - 2017-04-25 15:30 +0200
      Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4 Alex Deucher <alexdeucher@gmail.com> - 2017-04-25 17:10 +0200
      Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-26 18:50 +0200
        Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4 Christian König <deathsimple@vodafone.de> - 2017-05-02 17:00 +0200
    [PATCH 2/4] PCI: add functionality for resizing resources v3 Christian König <deathsimple@vodafone.de> - 2017-04-25 15:30 +0200
      Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-26 19:10 +0200
        Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 Christian König <deathsimple@vodafone.de> - 2017-05-02 18:00 +0200
          Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-02 22:30 +0200
        Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 Christian König <deathsimple@vodafone.de> - 2017-05-04 11:30 +0200
          Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-04 12:20 +0200
            Re: [PATCH 2/4] PCI: add functionality for resizing resources v3 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-04 18:50 +0200
    [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v2 Christian König <deathsimple@vodafone.de> - 2017-04-25 15:30 +0200
      Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models  30h-3fh) Processors v2 Alex Deucher <alexdeucher@gmail.com> - 2017-04-25 17:10 +0200
      Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models  30h-3fh) Processors v2 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-26 19:20 +0200
    Re: Resizeable PCI BAR support V4 Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-25 16:30 +0200

#1630507 — Resizeable PCI BAR support V4

Hi everyone,

This is the fourth incarnation of this set of patches. It enables device
drivers to resize and most likely also relocate the PCI BAR of devices 
they manage to allow the CPU to access all of the device local memory at once.

This is very useful for GFX device drivers where the default PCI BAR is only
about 256MB in size for compatibility reasons, but the device easily have
multiple gigabyte of local memory.

Some changes since V3:
1. A lot of minor style cleanups.
2. Make internal  functions for changing BARs directly private to the PCI subsystem.
3. Fail if any BAR is still in use when we try to change it.
4. Handle intermediate bridges as well.
5. Print some more messages when changing something.

Please review and/or comment,
Christian.

[toc] | [next] | [standalone]

#1630510 — [PATCH 1/4] PCI: add resizeable BAR infrastructure v4

From: Christian König <christian.koenig@amd.com>

Just the defines and helper functions to read the possible sizes of a BAR and
update it's size.

See https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf
and PCIe r3.1, sec 7.22.

This is useful for hardware with large local storage (mostly GFX) which only
expose 256MB BARs initially to be compatible with 32bit systems.

v2: provide read helper as well
v3: improve function names, use unsigned values, add better comments.
v4: move definition, improve commit message, s/bar/BAR/

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/pci/pci.c             | 115 ++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/pci.h             |   4 ++
 include/uapi/linux/pci_regs.h |  11 +++-
 3 files changed, 128 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ba34907..c2d9f78 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2944,6 +2944,121 @@ bool pci_acs_path_enabled(struct pci_dev *start,
 }
 
 /**
+ * pci_rbar_get_possible_sizes - get possible sizes for BAR
+ * @dev: PCI device
+ * @bar: BAR to query
+ *
+ * Get the possible sizes of a resizeable BAR as bitmask defined in the spec
+ * (bit 0=1MB, bit 19=512GB). Returns 0 if BAR isn't resizeable.
+ */
+u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar)
+{
+	unsigned pos, nbars;
+	u32 ctrl, cap;
+	unsigned i;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
+	if (!pos)
+		return 0;
+
+	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
+
+	for (i = 0; i < nbars; ++i, pos += 8) {
+		int bar_idx;
+
+		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+		bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
+				PCI_REBAR_CTRL_BAR_IDX_SHIFT;
+		if (bar_idx != bar)
+			continue;
+
+		pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
+		return (cap & PCI_REBAR_CTRL_SIZES_MASK) >>
+			PCI_REBAR_CTRL_SIZES_SHIFT;
+	}
+
+	return 0;
+}
+
+/**
+ * pci_rbar_get_current_size - get the current size of a BAR
+ * @dev: PCI device
+ * @bar: BAR to set size to
+ *
+ * Read the size of a BAR from the resizeable BAR config.
+ * Returns size if found or negativ error code.
+ */
+int pci_rbar_get_current_size(struct pci_dev *pdev, int bar)
+{
+	unsigned pos, nbars;
+	u32 ctrl;
+	unsigned i;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
+	if (!pos)
+		return -ENOTSUPP;
+
+	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
+
+	for (i = 0; i < nbars; ++i, pos += 8) {
+		int bar_idx;
+
+		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+		bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
+				PCI_REBAR_CTRL_BAR_IDX_SHIFT;
+		if (bar_idx != bar)
+			continue;
+
+		return (ctrl & PCI_REBAR_CTRL_BAR_SIZE_MASK) >>
+			PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
+	}
+
+	return -ENOENT;
+}
+
+/**
+ * pci_rbar_set_size - set a new size for a BAR
+ * @dev: PCI device
+ * @bar: BAR to set size to
+ * @size: new size as defined in the spec (log2(size in bytes) - 20)
+ *
+ * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
+ * Returns true if resizing was successful, false otherwise.
+ */
+int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)
+{
+	unsigned pos, nbars;
+	u32 ctrl;
+	unsigned i;
+
+	pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
+	if (!pos)
+		return -ENOTSUPP;
+
+	pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+	nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
+
+	for (i = 0; i < nbars; ++i, pos += 8) {
+		int bar_idx;
+
+		pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
+		bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
+				PCI_REBAR_CTRL_BAR_IDX_SHIFT;
+		if (bar_idx != bar)
+			continue;
+
+		ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE_MASK;
+		ctrl |= size << PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
+		pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
+		return 0;
+	}
+
+	return -ENOENT;
+}
+
+/**
  * pci_swizzle_interrupt_pin - swizzle INTx for device behind bridge
  * @dev: the PCI device
  * @pin: the INTx pin (1=INTA, 2=INTB, 3=INTC, 4=INTD)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 4518562..1d27c22 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -356,4 +356,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
 }
 #endif
 
+u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar);
+int pci_rbar_get_current_size(struct pci_dev *pdev, int bar);
+int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size);
+
 #endif /* DRIVERS_PCI_H */
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index e5a2e68..a75429e 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -932,9 +932,16 @@
 #define PCI_SATA_SIZEOF_LONG	16
 
 /* Resizable BARs */
+#define PCI_REBAR_CAP		4	/* capability register */
+#define  PCI_REBAR_CTRL_SIZES_MASK	(0xFFFFF << 4)	/* mask for sizes */
+#define  PCI_REBAR_CTRL_SIZES_SHIFT	4	/* shift for sizes */
 #define PCI_REBAR_CTRL		8	/* control register */
-#define  PCI_REBAR_CTRL_NBAR_MASK	(7 << 5)	/* mask for # bars */
-#define  PCI_REBAR_CTRL_NBAR_SHIFT	5	/* shift for # bars */
+#define  PCI_REBAR_CTRL_BAR_IDX_MASK	(7 << 0)	/* mask for BAR index */
+#define  PCI_REBAR_CTRL_BAR_IDX_SHIFT	0	/* shift for BAR index */
+#define  PCI_REBAR_CTRL_NBAR_MASK	(7 << 5)	/* mask for # BARs */
+#define  PCI_REBAR_CTRL_NBAR_SHIFT	5	/* shift for # BARs */
+#define  PCI_REBAR_CTRL_BAR_SIZE_MASK	(0x1F << 8)	/* mask for BAR size */
+#define  PCI_REBAR_CTRL_BAR_SIZE_SHIFT	8	/* shift for BAR size */
 
 /* Dynamic Power Allocation */
 #define PCI_DPA_CAP		4	/* capability register */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1630586 — Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4

On Tue, Apr 25, 2017 at 9:19 AM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Just the defines and helper functions to read the possible sizes of a BAR and
> update it's size.
>
> See https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf
> and PCIe r3.1, sec 7.22.
>
> This is useful for hardware with large local storage (mostly GFX) which only
> expose 256MB BARs initially to be compatible with 32bit systems.
>
> v2: provide read helper as well
> v3: improve function names, use unsigned values, add better comments.
> v4: move definition, improve commit message, s/bar/BAR/
>
> Signed-off-by: Christian König <christian.koenig@amd.com>


Patches 1, 2:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/pci/pci.c             | 115 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/pci.h             |   4 ++
>  include/uapi/linux/pci_regs.h |  11 +++-
>  3 files changed, 128 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ba34907..c2d9f78 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2944,6 +2944,121 @@ bool pci_acs_path_enabled(struct pci_dev *start,
>  }
>
>  /**
> + * pci_rbar_get_possible_sizes - get possible sizes for BAR
> + * @dev: PCI device
> + * @bar: BAR to query
> + *
> + * Get the possible sizes of a resizeable BAR as bitmask defined in the spec
> + * (bit 0=1MB, bit 19=512GB). Returns 0 if BAR isn't resizeable.
> + */
> +u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar)
> +{
> +       unsigned pos, nbars;
> +       u32 ctrl, cap;
> +       unsigned i;
> +
> +       pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> +       if (!pos)
> +               return 0;
> +
> +       pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +       nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +       for (i = 0; i < nbars; ++i, pos += 8) {
> +               int bar_idx;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +               bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> +                               PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> +               if (bar_idx != bar)
> +                       continue;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CAP, &cap);
> +               return (cap & PCI_REBAR_CTRL_SIZES_MASK) >>
> +                       PCI_REBAR_CTRL_SIZES_SHIFT;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
> + * pci_rbar_get_current_size - get the current size of a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to
> + *
> + * Read the size of a BAR from the resizeable BAR config.
> + * Returns size if found or negativ error code.
> + */
> +int pci_rbar_get_current_size(struct pci_dev *pdev, int bar)
> +{
> +       unsigned pos, nbars;
> +       u32 ctrl;
> +       unsigned i;
> +
> +       pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> +       if (!pos)
> +               return -ENOTSUPP;
> +
> +       pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +       nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +       for (i = 0; i < nbars; ++i, pos += 8) {
> +               int bar_idx;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +               bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> +                               PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> +               if (bar_idx != bar)
> +                       continue;
> +
> +               return (ctrl & PCI_REBAR_CTRL_BAR_SIZE_MASK) >>
> +                       PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
> +       }
> +
> +       return -ENOENT;
> +}
> +
> +/**
> + * pci_rbar_set_size - set a new size for a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to
> + * @size: new size as defined in the spec (log2(size in bytes) - 20)
> + *
> + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
> + * Returns true if resizing was successful, false otherwise.
> + */
> +int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)
> +{
> +       unsigned pos, nbars;
> +       u32 ctrl;
> +       unsigned i;
> +
> +       pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> +       if (!pos)
> +               return -ENOTSUPP;
> +
> +       pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +       nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +       for (i = 0; i < nbars; ++i, pos += 8) {
> +               int bar_idx;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +               bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> +                               PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> +               if (bar_idx != bar)
> +                       continue;
> +
> +               ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE_MASK;
> +               ctrl |= size << PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
> +               pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> +               return 0;
> +       }
> +
> +       return -ENOENT;
> +}
> +
> +/**
>   * pci_swizzle_interrupt_pin - swizzle INTx for device behind bridge
>   * @dev: the PCI device
>   * @pin: the INTx pin (1=INTA, 2=INTB, 3=INTC, 4=INTD)
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 4518562..1d27c22 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -356,4 +356,8 @@ static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe)
>  }
>  #endif
>
> +u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar);
> +int pci_rbar_get_current_size(struct pci_dev *pdev, int bar);
> +int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size);
> +
>  #endif /* DRIVERS_PCI_H */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index e5a2e68..a75429e 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -932,9 +932,16 @@
>  #define PCI_SATA_SIZEOF_LONG   16
>
>  /* Resizable BARs */
> +#define PCI_REBAR_CAP          4       /* capability register */
> +#define  PCI_REBAR_CTRL_SIZES_MASK     (0xFFFFF << 4)  /* mask for sizes */
> +#define  PCI_REBAR_CTRL_SIZES_SHIFT    4       /* shift for sizes */
>  #define PCI_REBAR_CTRL         8       /* control register */
> -#define  PCI_REBAR_CTRL_NBAR_MASK      (7 << 5)        /* mask for # bars */
> -#define  PCI_REBAR_CTRL_NBAR_SHIFT     5       /* shift for # bars */
> +#define  PCI_REBAR_CTRL_BAR_IDX_MASK   (7 << 0)        /* mask for BAR index */
> +#define  PCI_REBAR_CTRL_BAR_IDX_SHIFT  0       /* shift for BAR index */
> +#define  PCI_REBAR_CTRL_NBAR_MASK      (7 << 5)        /* mask for # BARs */
> +#define  PCI_REBAR_CTRL_NBAR_SHIFT     5       /* shift for # BARs */
> +#define  PCI_REBAR_CTRL_BAR_SIZE_MASK  (0x1F << 8)     /* mask for BAR size */
> +#define  PCI_REBAR_CTRL_BAR_SIZE_SHIFT 8       /* shift for BAR size */
>
>  /* Dynamic Power Allocation */
>  #define PCI_DPA_CAP            4       /* capability register */
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1631600 — Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4

On Tue, Apr 25, 2017 at 4:19 PM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Just the defines and helper functions to read the possible sizes of a BAR and
> update it's size.
>
> See https://pcisig.com/sites/default/files/specification_documents/ECN_Resizable-BAR_24Apr2008.pdf
> and PCIe r3.1, sec 7.22.
>
> This is useful for hardware with large local storage (mostly GFX) which only
> expose 256MB BARs initially to be compatible with 32bit systems.

> +u32 pci_rbar_get_possible_sizes(struct pci_dev *pdev, int bar)
> +{

> +       unsigned pos, nbars;
> +       u32 ctrl, cap;
> +       unsigned i;

Are we supposed to use plain 'unsigned' nowadays? I would go with
'unsigned int'.

> +}

> + * Returns size if found or negativ error code.

Typo: negative.

> +int pci_rbar_get_current_size(struct pci_dev *pdev, int bar)
> +{
> +       unsigned pos, nbars;

> +       u32 ctrl;
> +       unsigned i;

Reversed tree order?

> +       for (i = 0; i < nbars; ++i, pos += 8) {
> +               int bar_idx;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +               bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> +                               PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> +               if (bar_idx != bar)
> +                       continue;
> +
> +               return (ctrl & PCI_REBAR_CTRL_BAR_SIZE_MASK) >>
> +                       PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
> +       }

This one the same as previous function, the difference only in what is
returned. CAre to split static helper function for both?

> +       return -ENOENT;
> +}

> +/**
> + * pci_rbar_set_size - set a new size for a BAR
> + * @dev: PCI device
> + * @bar: BAR to set size to

> + * @size: new size as defined in the spec (log2(size in bytes) - 20)

Not clear is it rounded up / down. I would go with "...in the spec
(0=1MB, 19=512GB)".

> + *
> + * Set the new size of a BAR as defined in the spec (0=1MB, 19=512GB).
> + * Returns true if resizing was successful, false otherwise.
> + */

> +int pci_rbar_set_size(struct pci_dev *pdev, int bar, int size)
> +{
> +       unsigned pos, nbars;
> +       u32 ctrl;
> +       unsigned i;


> +
> +       pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_REBAR);
> +       if (!pos)
> +               return -ENOTSUPP;
> +
> +       pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +       nbars = (ctrl & PCI_REBAR_CTRL_NBAR_MASK) >> PCI_REBAR_CTRL_NBAR_SHIFT;
> +
> +       for (i = 0; i < nbars; ++i, pos += 8) {
> +               int bar_idx;
> +
> +               pci_read_config_dword(pdev, pos + PCI_REBAR_CTRL, &ctrl);
> +               bar_idx = (ctrl & PCI_REBAR_CTRL_BAR_IDX_MASK) >>
> +                               PCI_REBAR_CTRL_BAR_IDX_SHIFT;
> +               if (bar_idx != bar)
> +                       continue;

Above is duplicating previous.

So,
static int ..._find_rbar(..., u32 *ctrl)
{
}

Returns: (i.e.) 0 - found, 1 - not found, -ERRNO.

ret = _find_rbar();
if (ret < 0)
 return ret;
if (ret)
 return -ENOENT;
...
return 0;

So, please refactor.

> +
> +               ctrl &= ~PCI_REBAR_CTRL_BAR_SIZE_MASK;
> +               ctrl |= size << PCI_REBAR_CTRL_BAR_SIZE_SHIFT;
> +               pci_write_config_dword(pdev, pos + PCI_REBAR_CTRL, ctrl);
> +               return 0;
> +       }
> +
> +       return -ENOENT;
> +}

> -#define  PCI_REBAR_CTRL_NBAR_MASK      (7 << 5)        /* mask for # bars */
> -#define  PCI_REBAR_CTRL_NBAR_SHIFT     5       /* shift for # bars */

> +#define  PCI_REBAR_CTRL_NBAR_MASK      (7 << 5)        /* mask for # BARs */
> +#define  PCI_REBAR_CTRL_NBAR_SHIFT     5       /* shift for # BARs */

I understand why, but I dunno it worth to do.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1634469 — Re: [PATCH 1/4] PCI: add resizeable BAR infrastructure v4

Am 26.04.2017 um 18:45 schrieb Andy Shevchenko:
> [SNIP]
>> -#define  PCI_REBAR_CTRL_NBAR_MASK      (7 << 5)        /* mask for # bars */
>> -#define  PCI_REBAR_CTRL_NBAR_SHIFT     5       /* shift for # bars */
>> +#define  PCI_REBAR_CTRL_NBAR_MASK      (7 << 5)        /* mask for # BARs */
>> +#define  PCI_REBAR_CTRL_NBAR_SHIFT     5       /* shift for # BARs */
> I understand why, but I dunno it worth to do.
Bjorn suggested that. Either way is fine with me, but he needs to stick 
his signed-of-by on it when pushing it upstream. So his opinion usually 
wins.

All other comments will be integrated in the next version of the patch.

Thanks for the review,
Christian.

[toc] | [prev] | [next] | [standalone]

#1630513 — [PATCH 2/4] PCI: add functionality for resizing resources v3

From: Christian König <christian.koenig@amd.com>

This allows device drivers to request resizing their BARs.

The function only tries to reprogram the windows of the bridge directly above
the requesting device and only the BAR of the same type (usually mem, 64bit,
prefetchable). This is done to make sure not to disturb other drivers by
changing the BARs of their devices.

If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
returned to the calling device driver.

v2: rebase on changes in rbar support
v3: style cleanups, fail if memory decoding is enabled or resources
    still allocated, resize all unused bridge BARs,
    drop calling pci_reenable_device

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/pci/setup-bus.c | 102 ++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/setup-res.c |  60 ++++++++++++++++++++++++++++
 include/linux/pci.h     |   3 ++
 3 files changed, 165 insertions(+)

diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index f30ca75..39351cf 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -1923,6 +1923,108 @@ void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge)
 }
 EXPORT_SYMBOL_GPL(pci_assign_unassigned_bridge_resources);
 
+int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
+{
+	const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
+		IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
+
+	struct pci_dev_resource *dev_res;
+	LIST_HEAD(saved);
+	LIST_HEAD(added);
+	LIST_HEAD(failed);
+	unsigned i;
+	int ret = 0;
+
+	/* Walk to the root BUS, releasing bridge BARs when possible */
+	while (1) {
+		for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END;
+		     i++) {
+			struct resource *res = &bridge->resource[i];
+
+			if ((res->flags & type_mask) != (type & type_mask))
+				continue;
+
+			/* Ignore BARs which are still in use */
+			if (res->child)
+				continue;
+
+			ret = add_to_list(&saved, bridge, res, 0, 0);
+			if (ret)
+				goto cleanup;
+
+			if (res->parent)
+				release_resource(res);
+			res->start = 0;
+			res->end = 0;
+			dev_info(&bridge->dev, "BAR %d: released %pR\n",
+				 i, res);
+			break;
+		}
+		if (i == PCI_BRIDGE_RESOURCE_END)
+			break;
+
+		if (!bridge->bus || !bridge->bus->self)
+			break;
+
+		bridge = bridge->bus->self;
+	}
+
+	if (list_empty(&saved))
+		return -ENOENT;
+
+	__pci_bus_size_bridges(bridge->subordinate, &added);
+	__pci_bridge_assign_resources(bridge, &added, &failed);
+	BUG_ON(!list_empty(&added));
+
+	if (!list_empty(&failed)) {
+		ret = -ENOSPC;
+		goto cleanup;
+	}
+
+
+	list_for_each_entry(dev_res, &saved, list) {
+		/* Skip the bridge we just assigned resources for. */
+		if (bridge == dev_res->dev)
+			continue;
+
+		bridge = dev_res->dev;
+		pci_setup_bridge(bridge->subordinate);
+	}
+
+	free_list(&saved);
+	free_list(&failed);
+	return ret;
+
+cleanup:
+	/* restore size and flags */
+	list_for_each_entry(dev_res, &failed, list) {
+		struct resource *res = dev_res->res;
+
+		res->start = dev_res->start;
+		res->end = dev_res->end;
+		res->flags = dev_res->flags;
+	}
+	free_list(&failed);
+
+	/* Revert to the old configuration */
+	list_for_each_entry(dev_res, &saved, list) {
+		struct resource *res = dev_res->res;
+
+		bridge = dev_res->dev;
+		i = res - bridge->resource;
+
+		res->start = dev_res->start;
+		res->end = dev_res->end;
+		res->flags = dev_res->flags;
+
+		pci_claim_resource(bridge, i);
+		pci_setup_bridge(bridge->subordinate);
+	}
+	free_list(&saved);
+
+	return ret;
+}
+
 void pci_assign_unassigned_bus_resources(struct pci_bus *bus)
 {
 	struct pci_dev *dev;
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 9526e34..0d40adb 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -363,6 +363,66 @@ int pci_reassign_resource(struct pci_dev *dev, int resno, resource_size_t addsiz
 	return 0;
 }
 
+void pci_release_resource(struct pci_dev *dev, int resno)
+{
+	struct resource *res = dev->resource + resno;
+
+	release_resource(res);
+	res->end = resource_size(res) - 1;
+	res->start = 0;
+	res->flags |= IORESOURCE_UNSET;
+	dev_info(&dev->dev, "BAR %d: released %pR\n", resno, res);
+}
+EXPORT_SYMBOL(pci_release_resource);
+
+int pci_resize_resource(struct pci_dev *dev, int resno, int size)
+{
+	struct resource *res = dev->resource + resno;
+	u64 bytes = 1ULL << (size + 20);
+	u16 cmd;
+	u32 sizes;
+	int old, ret = 0;
+
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+	if (cmd & PCI_COMMAND_MEMORY)
+		return -EBUSY;
+
+	sizes = pci_rbar_get_possible_sizes(dev, resno);
+	if (!sizes)
+		return -ENOTSUPP;
+
+	if (!(sizes & BIT(size)))
+		return -EINVAL;
+
+	old = pci_rbar_get_current_size(dev, resno);
+	if (old < 0)
+		return old;
+
+	/* Make sure the resource isn't assigned before making it larger. */
+	pci_release_resource(dev, resno);
+
+	ret = pci_rbar_set_size(dev, resno, size);
+	if (ret)
+		goto error_reassign;
+
+	res->end = res->start + bytes - 1;
+
+	ret = pci_reassign_bridge_resources(dev->bus->self, res->flags);
+	if (ret)
+		goto error_resize;
+
+	return 0;
+
+error_resize:
+	pci_rbar_set_size(dev, resno, old);
+	res->end = res->start + (1ULL << (old + 20)) - 1;
+
+error_reassign:
+	pci_assign_unassigned_bus_resources(dev->bus);
+	return ret;
+}
+EXPORT_SYMBOL(pci_resize_resource);
+
 int pci_enable_resources(struct pci_dev *dev, int mask)
 {
 	u16 cmd, old_cmd;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a38772a..f0a630a 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1055,6 +1055,8 @@ void pci_reset_bridge_secondary_bus(struct pci_dev *dev);
 void pci_update_resource(struct pci_dev *dev, int resno);
 int __must_check pci_assign_resource(struct pci_dev *dev, int i);
 int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
+void pci_release_resource(struct pci_dev *dev, int resno);
+int __must_check pci_resize_resource(struct pci_dev *dev, int i, int size);
 int pci_select_bars(struct pci_dev *dev, unsigned long flags);
 bool pci_device_is_present(struct pci_dev *pdev);
 void pci_ignore_hotplug(struct pci_dev *dev);
@@ -1135,6 +1137,7 @@ void pci_assign_unassigned_resources(void);
 void pci_assign_unassigned_bridge_resources(struct pci_dev *bridge);
 void pci_assign_unassigned_bus_resources(struct pci_bus *bus);
 void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus);
+int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type);
 void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1631611 — Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

On Tue, Apr 25, 2017 at 4:19 PM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> This allows device drivers to request resizing their BARs.
>
> The function only tries to reprogram the windows of the bridge directly above
> the requesting device and only the BAR of the same type (usually mem, 64bit,
> prefetchable). This is done to make sure not to disturb other drivers by
> changing the BARs of their devices.
>
> If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
> returned to the calling device driver.

> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
> +{
> +       const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
> +               IORESOURCE_PREFETCH | IORESOURCE_MEM_64;

> +

Redundant.

> +       struct pci_dev_resource *dev_res;
> +       LIST_HEAD(saved);
> +       LIST_HEAD(added);
> +       LIST_HEAD(failed);

> +       unsigned i;

unsigned int i;

> +       int ret = 0;
> +
> +       /* Walk to the root BUS, releasing bridge BARs when possible */

> +       while (1) {

This raises red flag. Care to refactor?
Also do {} while () syntax allows faster to get that the loop body
goes at least once.

> +               for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END;
> +                    i++) {

> +                       struct resource *res = &bridge->resource[i];
> +

> +                       if ((res->flags & type_mask) != (type & type_mask))

I would rather go with

if ((res->flags ^ type) & type_mask)

> +                               continue;
> +
> +                       /* Ignore BARs which are still in use */
> +                       if (res->child)
> +                               continue;
> +
> +                       ret = add_to_list(&saved, bridge, res, 0, 0);
> +                       if (ret)
> +                               goto cleanup;
> +
> +                       if (res->parent)
> +                               release_resource(res);

> +                       res->start = 0;
> +                       res->end = 0;

> +                       dev_info(&bridge->dev, "BAR %d: released %pR\n",
> +                                i, res);

I doesn't make much sense to me after zeroing a range.

> +                       break;
> +               }
> +               if (i == PCI_BRIDGE_RESOURCE_END)
> +                       break;
> +
> +               if (!bridge->bus || !bridge->bus->self)
> +                       break;
> +
> +               bridge = bridge->bus->self;
> +       }
> +
> +       if (list_empty(&saved))
> +               return -ENOENT;
> +
> +       __pci_bus_size_bridges(bridge->subordinate, &added);
> +       __pci_bridge_assign_resources(bridge, &added, &failed);
> +       BUG_ON(!list_empty(&added));
> +
> +       if (!list_empty(&failed)) {
> +               ret = -ENOSPC;
> +               goto cleanup;
> +       }

> +
> +

Remove extra empty line.

> +       list_for_each_entry(dev_res, &saved, list) {
> +               /* Skip the bridge we just assigned resources for. */
> +               if (bridge == dev_res->dev)
> +                       continue;
> +
> +               bridge = dev_res->dev;
> +               pci_setup_bridge(bridge->subordinate);
> +       }
> +

> +       free_list(&saved);
> +       free_list(&failed);
> +       return ret;

You might re-use two lines with below, but perhaps better to show
which case returns 0 explicitly and drop assignment ret = 0 above.

> +
> +cleanup:
> +       /* restore size and flags */
> +       list_for_each_entry(dev_res, &failed, list) {
> +               struct resource *res = dev_res->res;
> +
> +               res->start = dev_res->start;
> +               res->end = dev_res->end;
> +               res->flags = dev_res->flags;
> +       }
> +       free_list(&failed);
> +
> +       /* Revert to the old configuration */
> +       list_for_each_entry(dev_res, &saved, list) {
> +               struct resource *res = dev_res->res;
> +
> +               bridge = dev_res->dev;
> +               i = res - bridge->resource;
> +
> +               res->start = dev_res->start;
> +               res->end = dev_res->end;
> +               res->flags = dev_res->flags;
> +
> +               pci_claim_resource(bridge, i);
> +               pci_setup_bridge(bridge->subordinate);
> +       }

> +       free_list(&saved);
> +
> +       return ret;

> +}

> +void pci_release_resource(struct pci_dev *dev, int resno)
> +{
> +       struct resource *res = dev->resource + resno;
> +
> +       release_resource(res);

> +       res->end = resource_size(res) - 1;
> +       res->start = 0;
> +       res->flags |= IORESOURCE_UNSET;

> +       dev_info(&dev->dev, "BAR %d: released %pR\n", resno, res);

Makes little sense to me after you cleared information out.

> +}
> +EXPORT_SYMBOL(pci_release_resource);
> +
> +int pci_resize_resource(struct pci_dev *dev, int resno, int size)
> +{
> +       struct resource *res = dev->resource + resno;

> +       u64 bytes = 1ULL << (size + 20);

> +       res->end = res->start + (1ULL << (old + 20)) - 1;

Perhaps macro or helper?

static inline u64 rbar_size_to_bytes(size)
{
return 1ULL << (size + 20);
}

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1634489 — Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
> On Tue, Apr 25, 2017 at 4:19 PM, Christian König
> <deathsimple@vodafone.de> wrote:
>> From: Christian König <christian.koenig@amd.com>
>>
>> This allows device drivers to request resizing their BARs.
>>
>> The function only tries to reprogram the windows of the bridge directly above
>> the requesting device and only the BAR of the same type (usually mem, 64bit,
>> prefetchable). This is done to make sure not to disturb other drivers by
>> changing the BARs of their devices.
>>
>> If reprogramming the bridge BAR fails the old status is restored and -ENOSPC
>> returned to the calling device driver.
>> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long type)
>> +{
>> +       const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>> +               IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
>> +
> Redundant.

Redundant, but also a reminder to myself that I wanted to ask something 
about that.

This type_mask is used already three times in this file, shouldn't we 
add a define for that?

> [SNIP]
>> +       list_for_each_entry(dev_res, &saved, list) {
>> +               /* Skip the bridge we just assigned resources for. */
>> +               if (bridge == dev_res->dev)
>> +                       continue;
>> +
>> +               bridge = dev_res->dev;
>> +               pci_setup_bridge(bridge->subordinate);
>> +       }
>> +
>> +       free_list(&saved);
>> +       free_list(&failed);
>> +       return ret;
> You might re-use two lines with below, but perhaps better to show
> which case returns 0 explicitly and drop assignment ret = 0 above.

Good point, but actually the free_list(&failed) is superfluous here 
since when the failed list isn't empty we end up in the cleanup path.

Going to fix all other comments in the next version.

Regards,
Christian.

[toc] | [prev] | [next] | [standalone]

#1634613 — Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

On Tue, May 2, 2017 at 6:51 PM, Christian König <deathsimple@vodafone.de> wrote:
> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
>> On Tue, Apr 25, 2017 at 4:19 PM, Christian König
>> <deathsimple@vodafone.de> wrote:

>>> +int pci_reassign_bridge_resources(struct pci_dev *bridge, unsigned long
>>> type)
>>> +{
>>> +       const unsigned long type_mask = IORESOURCE_IO | IORESOURCE_MEM |
>>> +               IORESOURCE_PREFETCH | IORESOURCE_MEM_64;
>>> +
>>
>> Redundant.
>
>
> Redundant, but also a reminder to myself that I wanted to ask something
> about that.

Missed context I suppose. Usually I comment in one word something
obvious, i.e. redundant empty line.
Sorry for missing my point.

> This type_mask is used already three times in this file, shouldn't we add a
> define for that?

Yes, that's wxactly what I commented somewhere (in one of the rest cases).

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1635576 — Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:
> [SNIP]
>> +       while (1) {
> This raises red flag. Care to refactor?
> Also do {} while () syntax allows faster to get that the loop body
> goes at least once.

I've tried to refactor this, but couldn't come up with something which 
works and is readable at the same time.

Any suggestion how this should look like?

Christian.

[toc] | [prev] | [next] | [standalone]

#1635619 — Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

On Thu, May 4, 2017 at 12:23 PM, Christian König
<deathsimple@vodafone.de> wrote:
> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:

>>> +       while (1) {
>>
>> This raises red flag. Care to refactor?
>> Also do {} while () syntax allows faster to get that the loop body
>> goes at least once.
>
>
> I've tried to refactor this, but couldn't come up with something which works
> and is readable at the same time.
>
> Any suggestion how this should look like?

This is original code.

--- 8< --- 8< ---

    while (1) {
            for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; i++) {
                    struct resource *res = &bridge->resource[i];

                    if ((res->flags & type_mask) != (type & type_mask))
                            continue;

                    /* Ignore BARs which are still in use */
                    if (res->child)
                            continue;

                    ret = add_to_list(&saved, bridge, res, 0, 0);
                    if (ret)
                            goto cleanup;

                    if (res->parent)
                            release_resource(res);
                    res->start = 0;
                    res->end = 0;
                    dev_info(&bridge->dev, "BAR %d: released %pR\n", i, res);
                    break;
            }
            if (i == PCI_BRIDGE_RESOURCE_END)
                    break;

            if (!bridge->bus || !bridge->bus->self)
                    break;

            bridge = bridge->bus->self;
    }

--- 8< --- 8< ---

I would think about something like below

static int ...xxx...(...)
{
    unsigned int i;
    int ret;

    for (i = PCI_BRIDGE_RESOURCES; i < PCI_BRIDGE_RESOURCE_END; i++) {
            struct resource *res = &bridge->resource[i];

            /* Ignore BARs which are still in use */
            if (((res->flags ^ type) & type_mask) == 0 && !res->child)
                    break;
    }
    if (i == PCI_BRIDGE_RESOURCE_END)
        return -EBUSY;

    ret = add_to_list(&saved, bridge, res, 0, 0);
    if (ret)
            return ret;

    if (res->parent)
            release_resource(res);

    res->start = 0;
    res->end = 0;
    dev_info(&bridge->dev, "BAR %d: released %pR\n", i, res);
    return i;
}


struct pci_dev *next = bridge;
...
    do {
        bridge = next;

        ret = ...xxx...(...);
        if (ret)
            goto cleanup;

        next = bridge->bus ? bridge->bus->self : NULL;
    } while (next);
...

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1635888 — Re: [PATCH 2/4] PCI: add functionality for resizing resources v3

On Thu, May 4, 2017 at 1:15 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, May 4, 2017 at 12:23 PM, Christian König
> <deathsimple@vodafone.de> wrote:
>> Am 26.04.2017 um 19:00 schrieb Andy Shevchenko:

> static int ...xxx...(...)
> {
>     unsigned int i;
>     int ret;
>

>     if (res->parent)
>             release_resource(res);
>

dev_info() should be here. I commented on this earlier, just forgot.

>     res->start = 0;
>     res->end = 0;
>     dev_info(&bridge->dev, "BAR %d: released %pR\n", i, res);
>     return i;
> }
>
>
> struct pci_dev *next = bridge;
> ...
>     do {
>         bridge = next;
>
>         ret = ...xxx...(...);

>         if (ret)

Here, of course,

if (ret < 0)

>             goto cleanup;
>
>         next = bridge->bus ? bridge->bus->self : NULL;
>     } while (next);


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1630518 — [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v2

From: Christian König <christian.koenig@amd.com>

Most BIOS don't enable this because of compatibility reasons.

Manually enable a 64bit BAR of 64GB size so that we have
enough room for PCI devices.

v2: style cleanups, increase size, add resource name, set correct flags,
    print message that windows was added

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 arch/x86/pci/fixup.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index 6d52b94..8d949c4 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -571,3 +571,56 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
 DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
+
+static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
+{
+	uint32_t base, limit, high;
+	struct resource *res, *conflict;
+	unsigned i;
+
+	for (i = 0; i < 8; ++i) {
+		pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
+		pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
+
+		/* Is this slot free? */
+		if ((base & 0x3) == 0x0)
+			break;
+
+		base >>= 8;
+		base |= high << 24;
+
+		/* Abort if a slot already configures a 64bit BAR. */
+		if (base > 0x10000)
+			return;
+	}
+	if (i == 8)
+		return;
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return;
+
+	res->name = "PCI Bus 0000:00";
+	res->flags = IORESOURCE_PREFETCH | IORESOURCE_MEM |
+		IORESOURCE_MEM_64 | IORESOURCE_WINDOW;
+	res->start = 0x100000000;
+	res->end = 0xfd00000000 - 1;
+
+	/* Just grab the free area behind system memory for this */
+	while ((conflict = request_resource_conflict(&iomem_resource, res)))
+		res->start = conflict->end + 1;
+
+	dev_info(&dev->dev, "adding root bus resource %pR\n", res);
+
+	base = ((res->start >> 8) & 0xffffff00) | 0x3;
+	limit = ((res->end + 1) >> 8) & 0xffffff00;
+	high = ((res->start >> 40) & 0xff) |
+		((((res->end + 1) >> 40) & 0xff) << 16);
+
+	pci_write_config_dword(dev, 0x180 + i * 0x4, high);
+	pci_write_config_dword(dev, 0x84 + i * 0x8, limit);
+	pci_write_config_dword(dev, 0x80 + i * 0x8, base);
+
+	pci_bus_add_resource(dev->bus, res, 0);
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x141b, pci_amd_enable_64bit_bar);
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1630594 — Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v2

On Tue, Apr 25, 2017 at 9:19 AM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Most BIOS don't enable this because of compatibility reasons.
>
> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.
>
> v2: style cleanups, increase size, add resource name, set correct flags,
>     print message that windows was added
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  arch/x86/pci/fixup.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>
> diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
> index 6d52b94..8d949c4 100644
> --- a/arch/x86/pci/fixup.c
> +++ b/arch/x86/pci/fixup.c
> @@ -571,3 +571,56 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x2fc0, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6f60, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fa0, pci_invalid_bar);
>  DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, 0x6fc0, pci_invalid_bar);
> +
> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{
> +       uint32_t base, limit, high;
> +       struct resource *res, *conflict;
> +       unsigned i;
> +
> +       for (i = 0; i < 8; ++i) {
> +               pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
> +               pci_read_config_dword(dev, 0x180 + i * 0x4, &high);

Might be nice to define names for the register offsets.


> +
> +               /* Is this slot free? */
> +               if ((base & 0x3) == 0x0)
> +                       break;
> +
> +               base >>= 8;
> +               base |= high << 24;
> +
> +               /* Abort if a slot already configures a 64bit BAR. */
> +               if (base > 0x10000)
> +                       return;
> +       }
> +       if (i == 8)
> +               return;
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       if (!res)
> +               return;
> +
> +       res->name = "PCI Bus 0000:00";
> +       res->flags = IORESOURCE_PREFETCH | IORESOURCE_MEM |
> +               IORESOURCE_MEM_64 | IORESOURCE_WINDOW;
> +       res->start = 0x100000000;
> +       res->end = 0xfd00000000 - 1;
> +
> +       /* Just grab the free area behind system memory for this */
> +       while ((conflict = request_resource_conflict(&iomem_resource, res)))
> +               res->start = conflict->end + 1;
> +
> +       dev_info(&dev->dev, "adding root bus resource %pR\n", res);
> +
> +       base = ((res->start >> 8) & 0xffffff00) | 0x3;
> +       limit = ((res->end + 1) >> 8) & 0xffffff00;
> +       high = ((res->start >> 40) & 0xff) |
> +               ((((res->end + 1) >> 40) & 0xff) << 16);
> +
> +       pci_write_config_dword(dev, 0x180 + i * 0x4, high);
> +       pci_write_config_dword(dev, 0x84 + i * 0x8, limit);
> +       pci_write_config_dword(dev, 0x80 + i * 0x8, base);

Same here.  Either way:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> +
> +       pci_bus_add_resource(dev->bus, res, 0);
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_AMD, 0x141b, pci_amd_enable_64bit_bar);
> --
> 2.7.4
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[toc] | [prev] | [next] | [standalone]

#1631615 — Re: [PATCH 3/4] x86/PCI: Enable a 64bit BAR on AMD Family 15h (Models 30h-3fh) Processors v2

On Tue, Apr 25, 2017 at 4:19 PM, Christian König
<deathsimple@vodafone.de> wrote:
> From: Christian König <christian.koenig@amd.com>
>
> Most BIOS don't enable this because of compatibility reasons.
>
> Manually enable a 64bit BAR of 64GB size so that we have
> enough room for PCI devices.

> +static void pci_amd_enable_64bit_bar(struct pci_dev *dev)
> +{

> +       uint32_t base, limit, high;

Is u32 or uint32_t used actually in this file?

> +       struct resource *res, *conflict;

> +       unsigned i;

unsigned int i;

Reversed tree order.

> +       for (i = 0; i < 8; ++i) {
> +               pci_read_config_dword(dev, 0x80 + i * 0x8, &base);
> +               pci_read_config_dword(dev, 0x180 + i * 0x4, &high);
> +
> +               /* Is this slot free? */
> +               if ((base & 0x3) == 0x0)
> +                       break;
> +
> +               base >>= 8;
> +               base |= high << 24;
> +
> +               /* Abort if a slot already configures a 64bit BAR. */
> +               if (base > 0x10000)
> +                       return;
> +       }
> +       if (i == 8)
> +               return;
> +
> +       res = kzalloc(sizeof(*res), GFP_KERNEL);
> +       if (!res)
> +               return;
> +
> +       res->name = "PCI Bus 0000:00";

> +       res->flags = IORESOURCE_PREFETCH | IORESOURCE_MEM |
> +               IORESOURCE_MEM_64 | IORESOURCE_WINDOW;

You are using this constant twice AFAICS, perhaps define it in generic
pci.h and re-use?

> +       res->start = 0x100000000;
> +       res->end = 0xfd00000000 - 1;

Perhaps you forgot ul / ull in many places in your code. Care to
compile for 32-bit and fix the warnings?

> +       dev_info(&dev->dev, "adding root bus resource %pR\n", res);

Some of your messages I think are rather dev_dbg() than dev_info().

> +       base = ((res->start >> 8) & 0xffffff00) | 0x3;
> +       limit = ((res->end + 1) >> 8) & 0xffffff00;
> +       high = ((res->start >> 40) & 0xff) |
> +               ((((res->end + 1) >> 40) & 0xff) << 16);

It would be nice to have the magics defined (just on top of this function) like

#define PCI_AMD_BAR_XXX_MASK GENMASK(...)
#define PCI_AMD_BAR_XXX_SHIFT nn

> +       pci_write_config_dword(dev, 0x180 + i * 0x4, high);
> +       pci_write_config_dword(dev, 0x84 + i * 0x8, limit);
> +       pci_write_config_dword(dev, 0x80 + i * 0x8, base);

Ditto for pos:s.

> +
> +       pci_bus_add_resource(dev->bus, res, 0);
> +}

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1630555

On Tue, Apr 25, 2017 at 4:19 PM, Christian König
<deathsimple@vodafone.de> wrote:
> Hi everyone,
>
> This is the fourth incarnation of this set of patches.

It would be nice if you put (next time) version of the series in each
patch. Usually it's done quite easy by passing -vX (where X is version
number) to git format-patch.

> It enables device
> drivers to resize and most likely also relocate the PCI BAR of devices
> they manage to allow the CPU to access all of the device local memory at once.
>
> This is very useful for GFX device drivers where the default PCI BAR is only
> about 256MB in size for compatibility reasons, but the device easily have
> multiple gigabyte of local memory.
>
> Some changes since V3:
> 1. A lot of minor style cleanups.
> 2. Make internal  functions for changing BARs directly private to the PCI subsystem.
> 3. Fail if any BAR is still in use when we try to change it.
> 4. Handle intermediate bridges as well.
> 5. Print some more messages when changing something.
>
> Please review and/or comment,

Would look later (perhaps this week),

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web