xen/drivers/vpci/Makefile | 2 +- xen/drivers/vpci/rebar.c | 135 +++++++++++++++++++++++++++++++++++++ xen/drivers/vpci/vpci.c | 6 ++ xen/include/xen/pci_regs.h | 14 ++++ xen/include/xen/vpci.h | 3 + 5 files changed, 159 insertions(+), 1 deletion(-) create mode 100644 xen/drivers/vpci/rebar.c
Some devices, like discrete GPU of amd, support resizable bar
capability, but vpci of Xen doesn't support this feature, so
they fail to resize bars and then cause probing failure.
According to PCIe spec, each bar that supports resizing has
two registers, PCI_REBAR_CAP and PCI_REBAR_CTRL. So, add
handlers for them to support resizing the size of BARs.
Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
---
Hi all,
v4->v5 changes:
* Called pci_size_mem_bar in rebar_ctrl_write to get addr and size of BAR instead of setting
their values directly after writing new size to hardware.
* Changed from "return" to "continue" when index/type of BAR are not correct during initializing
BAR.
* Corrected the value of PCI_REBAR_CTRL_BAR_SIZE from "0x00001F00" to "0x00003F00".
* Renamed PCI_REBAR_SIZE_BIAS to PCI_REBAR_CTRL_SIZE_BIAS.
* Re-defined "PCI_REBAR_CAP_SHIFT 4" to "PCI_REBAR_CAP_SIZES_MASK 0xFFFFFFF0U".
Best regards,
Jiqian Chen.
v3->v4 changes:
* Removed PCI_REBAR_CAP_SIZES since it was not needed, and added
PCI_REBAR_CAP_SHIFT and PCI_REBAR_CTRL_SIZES.
* Added parameter resizable_sizes to struct vpci_bar to cache the support resizable sizes and
added the logic in init_rebar().
* Changed PCI_REBAR_CAP to PCI_REBAR_CAP(n) (4+8*(n)), changed PCI_REBAR_CTRL to
PCI_REBAR_CTRL(n) (8+8*(n)).
* Added domain info of pci_dev to printings of init_rebar().
v2->v3 changes:
* Used "bar->enabled" to replace "pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY",
and added comments why it needs this check.
* Added "!is_hardware_domain(pdev->domain)" check in init_rebar() to return EOPNOTSUPP for domUs.
* Moved BAR type and index check into init_rebar(), then only need to check once.
* Added 'U' suffix for macro PCI_REBAR_CAP_SIZES.
* Added macro PCI_REBAR_SIZE_BIAS to represent 20.
TODO: need to hide ReBar capability from hardware domain when init_rebar() fails.
v1->v2 changes:
* In rebar_ctrl_write, to check if memory decoding is enabled, and added
some checks for the type of Bar.
* Added vpci_hw_write32 to handle PCI_REBAR_CAP's write, since there is
no write limitation of dom0.
* And has many other minor modifications as well.
---
xen/drivers/vpci/Makefile | 2 +-
xen/drivers/vpci/rebar.c | 135 +++++++++++++++++++++++++++++++++++++
xen/drivers/vpci/vpci.c | 6 ++
xen/include/xen/pci_regs.h | 14 ++++
xen/include/xen/vpci.h | 3 +
5 files changed, 159 insertions(+), 1 deletion(-)
create mode 100644 xen/drivers/vpci/rebar.c
diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
index 1a1413b93e76..a7c8a30a8956 100644
--- a/xen/drivers/vpci/Makefile
+++ b/xen/drivers/vpci/Makefile
@@ -1,2 +1,2 @@
-obj-y += vpci.o header.o
+obj-y += vpci.o header.o rebar.o
obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
new file mode 100644
index 000000000000..ed22a75e16fd
--- /dev/null
+++ b/xen/drivers/vpci/rebar.c
@@ -0,0 +1,135 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
+ *
+ * Author: Jiqian Chen <Jiqian.Chen@amd.com>
+ */
+
+#include <xen/sched.h>
+#include <xen/vpci.h>
+
+static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
+ unsigned int reg,
+ uint32_t val,
+ void *data)
+{
+ unsigned int index;
+ struct vpci_bar *bar = data;
+ uint64_t size = PCI_REBAR_CTRL_SIZE(val);
+
+ if ( bar->enabled )
+ {
+ /*
+ * Refuse to resize a BAR while memory decoding is enabled, as
+ * otherwise the size of the mapped region in the p2m would become
+ * stale with the newly set BAR size, and the position of the BAR
+ * would be reset to undefined. Note the PCIe specification also
+ * forbids resizing a BAR with memory decoding enabled.
+ */
+ if ( size != bar->size )
+ gprintk(XENLOG_ERR,
+ "%pp: refuse to resize BAR with memory decoding enabled\n",
+ &pdev->sbdf);
+ return;
+ }
+
+ if ( !((size >> PCI_REBAR_CTRL_SIZE_BIAS) & bar->resizable_sizes) )
+ gprintk(XENLOG_WARNING,
+ "%pp: new size %#lx is not supported by hardware\n",
+ &pdev->sbdf, size);
+
+ pci_conf_write32(pdev->sbdf, reg, val);
+
+ index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
+ pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + index * 4, &bar->addr,
+ &bar->size, ((index == PCI_HEADER_NORMAL_NR_BARS - 1) ?
+ PCI_BAR_LAST : 0));
+ bar->guest_addr = bar->addr;
+}
+
+static int cf_check init_rebar(struct pci_dev *pdev)
+{
+ uint32_t ctrl;
+ unsigned int nbars;
+ unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
+ PCI_EXT_CAP_ID_REBAR);
+
+ if ( !rebar_offset )
+ return 0;
+
+ if ( !is_hardware_domain(pdev->domain) )
+ {
+ printk(XENLOG_ERR "%pp: resizable BARs unsupported for unpriv %pd\n",
+ &pdev->sbdf, pdev->domain);
+ return -EOPNOTSUPP;
+ }
+
+ ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
+ nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
+ for ( unsigned int i = 0; i < nbars; i++ )
+ {
+ int rc;
+ struct vpci_bar *bar;
+ unsigned int index;
+
+ ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
+ index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
+ if ( index >= PCI_HEADER_NORMAL_NR_BARS )
+ {
+ printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
+ pdev->domain, &pdev->sbdf, index);
+ continue;
+ }
+
+ bar = &pdev->vpci->header.bars[index];
+ if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
+ {
+ printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
+ pdev->domain, &pdev->sbdf, index);
+ continue;
+ }
+
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
+ rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
+ if ( rc )
+ {
+ /*
+ * TODO: for failed pathes, need to hide ReBar capability
+ * from hardware domain instead of returning an error.
+ */
+ printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP rc=%d\n",
+ pdev->domain, &pdev->sbdf, rc);
+ return rc;
+ }
+
+ rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
+ rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
+ if ( rc )
+ {
+ printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL rc=%d\n",
+ pdev->domain, &pdev->sbdf, rc);
+ return rc;
+ }
+
+ bar->resizable_sizes =
+ MASK_EXTR(pci_conf_read32(pdev->sbdf,
+ rebar_offset + PCI_REBAR_CAP(i)),
+ PCI_REBAR_CAP_SIZES_MASK);
+ bar->resizable_sizes |=
+ (((uint64_t)MASK_EXTR(ctrl, PCI_REBAR_CTRL_SIZES_MASK) << 32) /
+ ISOLATE_LSB(PCI_REBAR_CAP_SIZES_MASK));
+ }
+
+ return 0;
+}
+REGISTER_VPCI_INIT(init_rebar, VPCI_PRIORITY_LOW);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
index 1e6aa5d799b9..3349b98389b8 100644
--- a/xen/drivers/vpci/vpci.c
+++ b/xen/drivers/vpci/vpci.c
@@ -232,6 +232,12 @@ void cf_check vpci_hw_write16(
pci_conf_write16(pdev->sbdf, reg, val);
}
+void cf_check vpci_hw_write32(
+ const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data)
+{
+ pci_conf_write32(pdev->sbdf, reg, val);
+}
+
int vpci_add_register_mask(struct vpci *vpci, vpci_read_t *read_handler,
vpci_write_t *write_handler, unsigned int offset,
unsigned int size, void *data, uint32_t ro_mask,
diff --git a/xen/include/xen/pci_regs.h b/xen/include/xen/pci_regs.h
index 250ba106dbd3..65d77624fc73 100644
--- a/xen/include/xen/pci_regs.h
+++ b/xen/include/xen/pci_regs.h
@@ -459,6 +459,7 @@
#define PCI_EXT_CAP_ID_ARI 14
#define PCI_EXT_CAP_ID_ATS 15
#define PCI_EXT_CAP_ID_SRIOV 16
+#define PCI_EXT_CAP_ID_REBAR 21 /* Resizable BAR */
/* Advanced Error Reporting */
#define PCI_ERR_UNCOR_STATUS 4 /* Uncorrectable Error Status */
@@ -541,6 +542,19 @@
#define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf)
#define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff)
+/* Resizable BARs */
+#define PCI_REBAR_CAP(n) (4 + 8 * (n)) /* capability register */
+#define PCI_REBAR_CAP_SIZES_MASK 0xFFFFFFF0U /* supported BAR sizes in CAP */
+#define PCI_REBAR_CTRL(n) (8 + 8 * (n)) /* control register */
+#define PCI_REBAR_CTRL_BAR_IDX 0x00000007 /* BAR index */
+#define PCI_REBAR_CTRL_NBAR_MASK 0x000000E0 /* # of resizable BARs */
+#define PCI_REBAR_CTRL_BAR_SIZE 0x00003F00 /* BAR size */
+#define PCI_REBAR_CTRL_SIZES_MASK 0xFFFF0000U /* supported BAR sizes in CTRL */
+#define PCI_REBAR_CTRL_SIZE_BIAS 20
+#define PCI_REBAR_CTRL_SIZE(v) \
+ (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) \
+ + PCI_REBAR_CTRL_SIZE_BIAS))
+
/*
* Hypertransport sub capability types
*
diff --git a/xen/include/xen/vpci.h b/xen/include/xen/vpci.h
index 41e7c3bc2791..9d47b8c1a50e 100644
--- a/xen/include/xen/vpci.h
+++ b/xen/include/xen/vpci.h
@@ -78,6 +78,8 @@ uint32_t cf_check vpci_hw_read32(
const struct pci_dev *pdev, unsigned int reg, void *data);
void cf_check vpci_hw_write16(
const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
+void cf_check vpci_hw_write32(
+ const struct pci_dev *pdev, unsigned int reg, uint32_t val, void *data);
/*
* Check for pending vPCI operations on this vcpu. Returns true if the vcpu
@@ -100,6 +102,7 @@ struct vpci {
/* Guest address. */
uint64_t guest_addr;
uint64_t size;
+ uint64_t resizable_sizes;
struct rangeset *mem;
enum {
VPCI_BAR_EMPTY,
--
2.34.1On Tue, Jan 14, 2025 at 11:26:36AM +0800, Jiqian Chen wrote:
> Some devices, like discrete GPU of amd, support resizable bar
> capability, but vpci of Xen doesn't support this feature, so
> they fail to resize bars and then cause probing failure.
>
> According to PCIe spec, each bar that supports resizing has
> two registers, PCI_REBAR_CAP and PCI_REBAR_CTRL. So, add
> handlers for them to support resizing the size of BARs.
>
> Signed-off-by: Jiqian Chen <Jiqian.Chen@amd.com>
> ---
> Hi all,
> v4->v5 changes:
> * Called pci_size_mem_bar in rebar_ctrl_write to get addr and size of BAR instead of setting
> their values directly after writing new size to hardware.
> * Changed from "return" to "continue" when index/type of BAR are not correct during initializing
> BAR.
> * Corrected the value of PCI_REBAR_CTRL_BAR_SIZE from "0x00001F00" to "0x00003F00".
> * Renamed PCI_REBAR_SIZE_BIAS to PCI_REBAR_CTRL_SIZE_BIAS.
> * Re-defined "PCI_REBAR_CAP_SHIFT 4" to "PCI_REBAR_CAP_SIZES_MASK 0xFFFFFFF0U".
>
> Best regards,
> Jiqian Chen.
>
> v3->v4 changes:
> * Removed PCI_REBAR_CAP_SIZES since it was not needed, and added
> PCI_REBAR_CAP_SHIFT and PCI_REBAR_CTRL_SIZES.
> * Added parameter resizable_sizes to struct vpci_bar to cache the support resizable sizes and
> added the logic in init_rebar().
> * Changed PCI_REBAR_CAP to PCI_REBAR_CAP(n) (4+8*(n)), changed PCI_REBAR_CTRL to
> PCI_REBAR_CTRL(n) (8+8*(n)).
> * Added domain info of pci_dev to printings of init_rebar().
>
> v2->v3 changes:
> * Used "bar->enabled" to replace "pci_conf_read16(pdev->sbdf, PCI_COMMAND) & PCI_COMMAND_MEMORY",
> and added comments why it needs this check.
> * Added "!is_hardware_domain(pdev->domain)" check in init_rebar() to return EOPNOTSUPP for domUs.
> * Moved BAR type and index check into init_rebar(), then only need to check once.
> * Added 'U' suffix for macro PCI_REBAR_CAP_SIZES.
> * Added macro PCI_REBAR_SIZE_BIAS to represent 20.
> TODO: need to hide ReBar capability from hardware domain when init_rebar() fails.
>
> v1->v2 changes:
> * In rebar_ctrl_write, to check if memory decoding is enabled, and added
> some checks for the type of Bar.
> * Added vpci_hw_write32 to handle PCI_REBAR_CAP's write, since there is
> no write limitation of dom0.
> * And has many other minor modifications as well.
> ---
> xen/drivers/vpci/Makefile | 2 +-
> xen/drivers/vpci/rebar.c | 135 +++++++++++++++++++++++++++++++++++++
> xen/drivers/vpci/vpci.c | 6 ++
> xen/include/xen/pci_regs.h | 14 ++++
> xen/include/xen/vpci.h | 3 +
> 5 files changed, 159 insertions(+), 1 deletion(-)
> create mode 100644 xen/drivers/vpci/rebar.c
>
> diff --git a/xen/drivers/vpci/Makefile b/xen/drivers/vpci/Makefile
> index 1a1413b93e76..a7c8a30a8956 100644
> --- a/xen/drivers/vpci/Makefile
> +++ b/xen/drivers/vpci/Makefile
> @@ -1,2 +1,2 @@
> -obj-y += vpci.o header.o
> +obj-y += vpci.o header.o rebar.o
> obj-$(CONFIG_HAS_PCI_MSI) += msi.o msix.o
> diff --git a/xen/drivers/vpci/rebar.c b/xen/drivers/vpci/rebar.c
> new file mode 100644
> index 000000000000..ed22a75e16fd
> --- /dev/null
> +++ b/xen/drivers/vpci/rebar.c
> @@ -0,0 +1,135 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
> + *
> + * Author: Jiqian Chen <Jiqian.Chen@amd.com>
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/vpci.h>
> +
> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> + unsigned int reg,
> + uint32_t val,
> + void *data)
> +{
> + unsigned int index;
> + struct vpci_bar *bar = data;
> + uint64_t size = PCI_REBAR_CTRL_SIZE(val);
> +
> + if ( bar->enabled )
> + {
> + /*
> + * Refuse to resize a BAR while memory decoding is enabled, as
> + * otherwise the size of the mapped region in the p2m would become
> + * stale with the newly set BAR size, and the position of the BAR
> + * would be reset to undefined. Note the PCIe specification also
> + * forbids resizing a BAR with memory decoding enabled.
> + */
> + if ( size != bar->size )
> + gprintk(XENLOG_ERR,
> + "%pp: refuse to resize BAR with memory decoding enabled\n",
> + &pdev->sbdf);
> + return;
> + }
> +
> + if ( !((size >> PCI_REBAR_CTRL_SIZE_BIAS) & bar->resizable_sizes) )
> + gprintk(XENLOG_WARNING,
> + "%pp: new size %#lx is not supported by hardware\n",
> + &pdev->sbdf, size);
> +
> + pci_conf_write32(pdev->sbdf, reg, val);
> +
> + index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
No need for the PCI config space access, you can get the index using:
bar - pdev->vpci->header.bars
In fact you could define the local variables as:
struct vpci_bar *bar = data;
const unsigned int index = bar - pdev->vpci->header.bars;
uint64_t size = PCI_REBAR_CTRL_SIZE(val);
And use index in the error messages also:
"%pp: refuse to resize BAR#u with memory decoding enabled\n"
"%pp: new BAR#%u size %#lx is not supported by hardware\n",
> + pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + index * 4, &bar->addr,
> + &bar->size, ((index == PCI_HEADER_NORMAL_NR_BARS - 1) ?
> + PCI_BAR_LAST : 0));
Seeing as Jan already asked you to fix indentation here, I think you
can also drop the outermost parentheses from the last argument.
> + bar->guest_addr = bar->addr;
> +}
> +
> +static int cf_check init_rebar(struct pci_dev *pdev)
> +{
> + uint32_t ctrl;
> + unsigned int nbars;
> + unsigned int rebar_offset = pci_find_ext_capability(pdev->sbdf,
> + PCI_EXT_CAP_ID_REBAR);
> +
> + if ( !rebar_offset )
> + return 0;
> +
> + if ( !is_hardware_domain(pdev->domain) )
> + {
> + printk(XENLOG_ERR "%pp: resizable BARs unsupported for unpriv %pd\n",
> + &pdev->sbdf, pdev->domain);
> + return -EOPNOTSUPP;
> + }
> +
> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
> + for ( unsigned int i = 0; i < nbars; i++ )
> + {
> + int rc;
> + struct vpci_bar *bar;
> + unsigned int index;
> +
> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
> + index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> + if ( index >= PCI_HEADER_NORMAL_NR_BARS )
> + {
> + printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
> + pdev->domain, &pdev->sbdf, index);
> + continue;
> + }
> +
> + bar = &pdev->vpci->header.bars[index];
> + if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
> + {
> + printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
> + pdev->domain, &pdev->sbdf, index);
> + continue;
> + }
> +
> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
> + rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
> + if ( rc )
> + {
> + /*
> + * TODO: for failed pathes, need to hide ReBar capability
> + * from hardware domain instead of returning an error.
> + */
> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP rc=%d\n",
> + pdev->domain, &pdev->sbdf, rc);
> + return rc;
> + }
> +
> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
> + rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
> + if ( rc )
> + {
> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL rc=%d\n",
> + pdev->domain, &pdev->sbdf, rc);
> + return rc;
I think we said we wanted to attempt to continue here, rather than
returning an error and thus removing all vPCI handlers from the
device?
Error messages should also contain the BAR index IMO:
"%pd %pp: BAR%u fail to add reg of REBAR_{CAP,CTRL} rc=%d\n"
Thanks, Roger.
On 2025/1/21 16:46, Roger Pau Monné wrote:
> On Tue, Jan 14, 2025 at 11:26:36AM +0800, Jiqian Chen wrote:
>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>> + for ( unsigned int i = 0; i < nbars; i++ )
>> + {
>> + int rc;
>> + struct vpci_bar *bar;
>> + unsigned int index;
>> +
>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
>> + index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
>> + if ( index >= PCI_HEADER_NORMAL_NR_BARS )
>> + {
>> + printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
>> + pdev->domain, &pdev->sbdf, index);
>> + continue;
>> + }
>> +
>> + bar = &pdev->vpci->header.bars[index];
>> + if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
>> + {
>> + printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
>> + pdev->domain, &pdev->sbdf, index);
>> + continue;
>> + }
>> +
>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
>> + rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
>> + if ( rc )
>> + {
>> + /*
>> + * TODO: for failed pathes, need to hide ReBar capability
>> + * from hardware domain instead of returning an error.
>> + */
>> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP rc=%d\n",
>> + pdev->domain, &pdev->sbdf, rc);
>> + return rc;
>> + }
>> +
>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
>> + rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
>> + if ( rc )
>> + {
>> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL rc=%d\n",
>> + pdev->domain, &pdev->sbdf, rc);
>> + return rc;
>
> I think we said we wanted to attempt to continue here, rather than
> returning an error and thus removing all vPCI handlers from the
> device?
I thought the result of your discussion with Jan was that I only needed to change the above two error paths to be "continue".
If these two also need to be changed, I will modify them in the next version.
--
Best regards,
Jiqian Chen.
On Tue, Jan 21, 2025 at 09:10:26AM +0000, Chen, Jiqian wrote:
> On 2025/1/21 16:46, Roger Pau Monné wrote:
> > On Tue, Jan 14, 2025 at 11:26:36AM +0800, Jiqian Chen wrote:
> >> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
> >> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
> >> + for ( unsigned int i = 0; i < nbars; i++ )
> >> + {
> >> + int rc;
> >> + struct vpci_bar *bar;
> >> + unsigned int index;
> >> +
> >> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
> >> + index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
> >> + if ( index >= PCI_HEADER_NORMAL_NR_BARS )
> >> + {
> >> + printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
> >> + pdev->domain, &pdev->sbdf, index);
> >> + continue;
> >> + }
> >> +
> >> + bar = &pdev->vpci->header.bars[index];
> >> + if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
> >> + {
> >> + printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
> >> + pdev->domain, &pdev->sbdf, index);
> >> + continue;
> >> + }
> >> +
> >> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
> >> + rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
> >> + if ( rc )
> >> + {
> >> + /*
> >> + * TODO: for failed pathes, need to hide ReBar capability
> >> + * from hardware domain instead of returning an error.
> >> + */
> >> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP rc=%d\n",
> >> + pdev->domain, &pdev->sbdf, rc);
> >> + return rc;
> >> + }
> >> +
> >> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
> >> + rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
> >> + if ( rc )
> >> + {
> >> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL rc=%d\n",
> >> + pdev->domain, &pdev->sbdf, rc);
> >> + return rc;
> >
> > I think we said we wanted to attempt to continue here, rather than
> > returning an error and thus removing all vPCI handlers from the
> > device?
> I thought the result of your discussion with Jan was that I only needed to change the above two error paths to be "continue".
> If these two also need to be changed, I will modify them in the next version.
Hm, let's wait for Jan to confirm, but even if handler cannot be setup
for some of the registers, it's better than just allowing dom0
unmediated access to the capability.
None of this is ideal, but it seems to be the option that gives dom0
most options to successfully boot.
Thanks, Roger.
On 21.01.2025 10:29, Roger Pau Monné wrote:
> On Tue, Jan 21, 2025 at 09:10:26AM +0000, Chen, Jiqian wrote:
>> On 2025/1/21 16:46, Roger Pau Monné wrote:
>>> On Tue, Jan 14, 2025 at 11:26:36AM +0800, Jiqian Chen wrote:
>>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>>>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>>>> + for ( unsigned int i = 0; i < nbars; i++ )
>>>> + {
>>>> + int rc;
>>>> + struct vpci_bar *bar;
>>>> + unsigned int index;
>>>> +
>>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
>>>> + index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
>>>> + if ( index >= PCI_HEADER_NORMAL_NR_BARS )
>>>> + {
>>>> + printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
>>>> + pdev->domain, &pdev->sbdf, index);
>>>> + continue;
>>>> + }
>>>> +
>>>> + bar = &pdev->vpci->header.bars[index];
>>>> + if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
>>>> + {
>>>> + printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
>>>> + pdev->domain, &pdev->sbdf, index);
>>>> + continue;
>>>> + }
>>>> +
>>>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
>>>> + rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
>>>> + if ( rc )
>>>> + {
>>>> + /*
>>>> + * TODO: for failed pathes, need to hide ReBar capability
>>>> + * from hardware domain instead of returning an error.
>>>> + */
>>>> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP rc=%d\n",
>>>> + pdev->domain, &pdev->sbdf, rc);
>>>> + return rc;
>>>> + }
>>>> +
>>>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
>>>> + rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
>>>> + if ( rc )
>>>> + {
>>>> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL rc=%d\n",
>>>> + pdev->domain, &pdev->sbdf, rc);
>>>> + return rc;
>>>
>>> I think we said we wanted to attempt to continue here, rather than
>>> returning an error and thus removing all vPCI handlers from the
>>> device?
>> I thought the result of your discussion with Jan was that I only needed to change the above two error paths to be "continue".
>> If these two also need to be changed, I will modify them in the next version.
>
> Hm, let's wait for Jan to confirm, but even if handler cannot be setup
> for some of the registers, it's better than just allowing dom0
> unmediated access to the capability.
I remained silent on this because I accepted this middle ground as ...
> None of this is ideal, but it seems to be the option that gives dom0
> most options to successfully boot.
... perhaps the most reasonable compromise.
Jan
On 2025/1/21 18:29, Jan Beulich wrote:
> On 21.01.2025 10:29, Roger Pau Monné wrote:
>> On Tue, Jan 21, 2025 at 09:10:26AM +0000, Chen, Jiqian wrote:
>>> On 2025/1/21 16:46, Roger Pau Monné wrote:
>>>> On Tue, Jan 14, 2025 at 11:26:36AM +0800, Jiqian Chen wrote:
>>>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(0));
>>>>> + nbars = MASK_EXTR(ctrl, PCI_REBAR_CTRL_NBAR_MASK);
>>>>> + for ( unsigned int i = 0; i < nbars; i++ )
>>>>> + {
>>>>> + int rc;
>>>>> + struct vpci_bar *bar;
>>>>> + unsigned int index;
>>>>> +
>>>>> + ctrl = pci_conf_read32(pdev->sbdf, rebar_offset + PCI_REBAR_CTRL(i));
>>>>> + index = ctrl & PCI_REBAR_CTRL_BAR_IDX;
>>>>> + if ( index >= PCI_HEADER_NORMAL_NR_BARS )
>>>>> + {
>>>>> + printk(XENLOG_ERR "%pd %pp: too big BAR number %u in REBAR_CTRL\n",
>>>>> + pdev->domain, &pdev->sbdf, index);
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + bar = &pdev->vpci->header.bars[index];
>>>>> + if ( bar->type != VPCI_BAR_MEM64_LO && bar->type != VPCI_BAR_MEM32 )
>>>>> + {
>>>>> + printk(XENLOG_ERR "%pd %pp: BAR%u is not in memory space\n",
>>>>> + pdev->domain, &pdev->sbdf, index);
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, vpci_hw_write32,
>>>>> + rebar_offset + PCI_REBAR_CAP(i), 4, NULL);
>>>>> + if ( rc )
>>>>> + {
>>>>> + /*
>>>>> + * TODO: for failed pathes, need to hide ReBar capability
>>>>> + * from hardware domain instead of returning an error.
>>>>> + */
>>>>> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CAP rc=%d\n",
>>>>> + pdev->domain, &pdev->sbdf, rc);
>>>>> + return rc;
>>>>> + }
>>>>> +
>>>>> + rc = vpci_add_register(pdev->vpci, vpci_hw_read32, rebar_ctrl_write,
>>>>> + rebar_offset + PCI_REBAR_CTRL(i), 4, bar);
>>>>> + if ( rc )
>>>>> + {
>>>>> + printk(XENLOG_ERR "%pd %pp: fail to add reg of REBAR_CTRL rc=%d\n",
>>>>> + pdev->domain, &pdev->sbdf, rc);
>>>>> + return rc;
>>>>
>>>> I think we said we wanted to attempt to continue here, rather than
>>>> returning an error and thus removing all vPCI handlers from the
>>>> device?
>>> I thought the result of your discussion with Jan was that I only needed to change the above two error paths to be "continue".
>>> If these two also need to be changed, I will modify them in the next version.
>>
>> Hm, let's wait for Jan to confirm, but even if handler cannot be setup
>> for some of the registers, it's better than just allowing dom0
>> unmediated access to the capability.
>
> I remained silent on this because I accepted this middle ground as ...
>
>> None of this is ideal, but it seems to be the option that gives dom0
>> most options to successfully boot.
>
> ... perhaps the most reasonable compromise.
OK, I see.
I will change to "continue" in next version and send v6 soon.
Thank you.
>
> Jan
--
Best regards,
Jiqian Chen.
On 14.01.2025 04:26, Jiqian Chen wrote:
> --- /dev/null
> +++ b/xen/drivers/vpci/rebar.c
> @@ -0,0 +1,135 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
Nit: This has now gone stale.
> + * Author: Jiqian Chen <Jiqian.Chen@amd.com>
> + */
> +
> +#include <xen/sched.h>
> +#include <xen/vpci.h>
> +
> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
> + unsigned int reg,
> + uint32_t val,
> + void *data)
> +{
> + unsigned int index;
> + struct vpci_bar *bar = data;
> + uint64_t size = PCI_REBAR_CTRL_SIZE(val);
> +
> + if ( bar->enabled )
> + {
> + /*
> + * Refuse to resize a BAR while memory decoding is enabled, as
> + * otherwise the size of the mapped region in the p2m would become
> + * stale with the newly set BAR size, and the position of the BAR
> + * would be reset to undefined. Note the PCIe specification also
> + * forbids resizing a BAR with memory decoding enabled.
> + */
> + if ( size != bar->size )
> + gprintk(XENLOG_ERR,
> + "%pp: refuse to resize BAR with memory decoding enabled\n",
> + &pdev->sbdf);
> + return;
> + }
> +
> + if ( !((size >> PCI_REBAR_CTRL_SIZE_BIAS) & bar->resizable_sizes) )
> + gprintk(XENLOG_WARNING,
> + "%pp: new size %#lx is not supported by hardware\n",
> + &pdev->sbdf, size);
> +
> + pci_conf_write32(pdev->sbdf, reg, val);
> +
> + index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
> + pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + index * 4, &bar->addr,
> + &bar->size, ((index == PCI_HEADER_NORMAL_NR_BARS - 1) ?
> + PCI_BAR_LAST : 0));
Nit: Imo it's unhelpful to the reader if you put multiple arguments on a single
line, when the final one then needs wrapping across lines. (Putting multiple
arguments on a single line is fine of course when they fully fit.)
> --- a/xen/include/xen/pci_regs.h
> +++ b/xen/include/xen/pci_regs.h
> @@ -459,6 +459,7 @@
> #define PCI_EXT_CAP_ID_ARI 14
> #define PCI_EXT_CAP_ID_ATS 15
> #define PCI_EXT_CAP_ID_SRIOV 16
> +#define PCI_EXT_CAP_ID_REBAR 21 /* Resizable BAR */
>
> /* Advanced Error Reporting */
> #define PCI_ERR_UNCOR_STATUS 4 /* Uncorrectable Error Status */
> @@ -541,6 +542,19 @@
> #define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf)
> #define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff)
>
> +/* Resizable BARs */
> +#define PCI_REBAR_CAP(n) (4 + 8 * (n)) /* capability register */
> +#define PCI_REBAR_CAP_SIZES_MASK 0xFFFFFFF0U /* supported BAR sizes in CAP */
> +#define PCI_REBAR_CTRL(n) (8 + 8 * (n)) /* control register */
> +#define PCI_REBAR_CTRL_BAR_IDX 0x00000007 /* BAR index */
> +#define PCI_REBAR_CTRL_NBAR_MASK 0x000000E0 /* # of resizable BARs */
> +#define PCI_REBAR_CTRL_BAR_SIZE 0x00003F00 /* BAR size */
> +#define PCI_REBAR_CTRL_SIZES_MASK 0xFFFF0000U /* supported BAR sizes in CTRL */
> +#define PCI_REBAR_CTRL_SIZE_BIAS 20
> +#define PCI_REBAR_CTRL_SIZE(v) \
> + (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) \
> + + PCI_REBAR_CTRL_SIZE_BIAS))
On x86 (being 64-bit only) and Arm64 1UL may be good enough here, but
I expect we'll need 1ULL for any 32-bit architecture.
Plus, as indicated before, these two auxiliary #define-s would imo
better be separated from those directly pertaining to the control
register fields (and then also not be padded like those).
Jan
On 2025/1/20 23:35, Jan Beulich wrote:
> On 14.01.2025 04:26, Jiqian Chen wrote:
>> --- /dev/null
>> +++ b/xen/drivers/vpci/rebar.c
>> @@ -0,0 +1,135 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (C) 2024 Advanced Micro Devices, Inc. All Rights Reserved.
>
> Nit: This has now gone stale.
Thanks, will change 2024 to 2025.
>
>> + * Author: Jiqian Chen <Jiqian.Chen@amd.com>
>> + */
>> +
>> +#include <xen/sched.h>
>> +#include <xen/vpci.h>
>> +
>> +static void cf_check rebar_ctrl_write(const struct pci_dev *pdev,
>> + unsigned int reg,
>> + uint32_t val,
>> + void *data)
>> +{
>> + unsigned int index;
>> + struct vpci_bar *bar = data;
>> + uint64_t size = PCI_REBAR_CTRL_SIZE(val);
>> +
>> + if ( bar->enabled )
>> + {
>> + /*
>> + * Refuse to resize a BAR while memory decoding is enabled, as
>> + * otherwise the size of the mapped region in the p2m would become
>> + * stale with the newly set BAR size, and the position of the BAR
>> + * would be reset to undefined. Note the PCIe specification also
>> + * forbids resizing a BAR with memory decoding enabled.
>> + */
>> + if ( size != bar->size )
>> + gprintk(XENLOG_ERR,
>> + "%pp: refuse to resize BAR with memory decoding enabled\n",
>> + &pdev->sbdf);
>> + return;
>> + }
>> +
>> + if ( !((size >> PCI_REBAR_CTRL_SIZE_BIAS) & bar->resizable_sizes) )
>> + gprintk(XENLOG_WARNING,
>> + "%pp: new size %#lx is not supported by hardware\n",
>> + &pdev->sbdf, size);
>> +
>> + pci_conf_write32(pdev->sbdf, reg, val);
>> +
>> + index = pci_conf_read32(pdev->sbdf, reg) & PCI_REBAR_CTRL_BAR_IDX;
>> + pci_size_mem_bar(pdev->sbdf, PCI_BASE_ADDRESS_0 + index * 4, &bar->addr,
>> + &bar->size, ((index == PCI_HEADER_NORMAL_NR_BARS - 1) ?
>> + PCI_BAR_LAST : 0));
>
> Nit: Imo it's unhelpful to the reader if you put multiple arguments on a single
> line, when the final one then needs wrapping across lines. (Putting multiple
> arguments on a single line is fine of course when they fully fit.)
Will change to put each argument in a single line in next version.
>
>> --- a/xen/include/xen/pci_regs.h
>> +++ b/xen/include/xen/pci_regs.h
>> @@ -459,6 +459,7 @@
>> #define PCI_EXT_CAP_ID_ARI 14
>> #define PCI_EXT_CAP_ID_ATS 15
>> #define PCI_EXT_CAP_ID_SRIOV 16
>> +#define PCI_EXT_CAP_ID_REBAR 21 /* Resizable BAR */
>>
>> /* Advanced Error Reporting */
>> #define PCI_ERR_UNCOR_STATUS 4 /* Uncorrectable Error Status */
>> @@ -541,6 +542,19 @@
>> #define PCI_VNDR_HEADER_REV(x) (((x) >> 16) & 0xf)
>> #define PCI_VNDR_HEADER_LEN(x) (((x) >> 20) & 0xfff)
>>
>> +/* Resizable BARs */
>> +#define PCI_REBAR_CAP(n) (4 + 8 * (n)) /* capability register */
>> +#define PCI_REBAR_CAP_SIZES_MASK 0xFFFFFFF0U /* supported BAR sizes in CAP */
>> +#define PCI_REBAR_CTRL(n) (8 + 8 * (n)) /* control register */
>> +#define PCI_REBAR_CTRL_BAR_IDX 0x00000007 /* BAR index */
>> +#define PCI_REBAR_CTRL_NBAR_MASK 0x000000E0 /* # of resizable BARs */
>> +#define PCI_REBAR_CTRL_BAR_SIZE 0x00003F00 /* BAR size */
>> +#define PCI_REBAR_CTRL_SIZES_MASK 0xFFFF0000U /* supported BAR sizes in CTRL */
>> +#define PCI_REBAR_CTRL_SIZE_BIAS 20
>> +#define PCI_REBAR_CTRL_SIZE(v) \
>> + (1UL << (MASK_EXTR(v, PCI_REBAR_CTRL_BAR_SIZE) \
>> + + PCI_REBAR_CTRL_SIZE_BIAS))
>
> On x86 (being 64-bit only) and Arm64 1UL may be good enough here, but
> I expect we'll need 1ULL for any 32-bit architecture.
>
> Plus, as indicated before, these two auxiliary #define-s would imo
> better be separated from those directly pertaining to the control
> register fields (and then also not be padded like those).
Thank you!
Will use a space line to separate these two #define-s from the others and change 1UL to 1ULL.
>
> Jan
--
Best regards,
Jiqian Chen.
© 2016 - 2025 Red Hat, Inc.