[PATCH] misc: pci_endpoint_test: Add consecutive BAR test

[PATCH] misc: pci_endpoint_test: Add consecutive BAR test

[Niklas Cassel]

Add a more advanced BAR test that writes all BARs in one go, and then reads
them back and verifies that the value matches the BAR number bitwise OR:ed
with offset, this allows us to verify:
-The BAR number was what we intended to read.
-The offset was what we intended to read.

This allows us to detect potential address translation issues on the EP.

Reading back the BAR directly after writing will not allow us to detect the
case where inbound address translation on the endpoint incorrectly causes
multiple BARs to be redirected to the same memory region (within the EP).

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 88 ++++++++++++++++++++++++++++++++
 include/uapi/linux/pcitest.h     |  1 +
 tools/pci/pcitest.c              | 16 +++++-
 tools/pci/pcitest.sh             |  1 +
 4 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee2..34cb54aef3d8b 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -322,6 +322,91 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
 	return true;
 }
 
+static u32 bar_test_pattern_with_offset(enum pci_barno barno, int offset)
+{
+	u32 val;
+
+	/* Keep the BAR pattern in the top byte. */
+	val = bar_test_pattern[barno] & 0xff000000;
+	/* Store the (partial) offset in the remaining bytes. */
+	val |= offset & 0x00ffffff;
+
+	return val;
+}
+
+static bool pci_endpoint_test_bars_write_bar(struct pci_endpoint_test *test,
+					     enum pci_barno barno)
+{
+	struct pci_dev *pdev = test->pdev;
+	int j, size;
+
+	size = pci_resource_len(pdev, barno);
+
+	if (barno == test->test_reg_bar)
+		size = 0x4;
+
+	for (j = 0; j < size; j += 4)
+		writel_relaxed(bar_test_pattern_with_offset(barno, j),
+			       test->bar[barno] + j);
+
+	return true;
+}
+
+static bool pci_endpoint_test_bars_read_bar(struct pci_endpoint_test *test,
+					    enum pci_barno barno)
+{
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	int j, size;
+	u32 val;
+
+	size = pci_resource_len(pdev, barno);
+
+	if (barno == test->test_reg_bar)
+		size = 0x4;
+
+	for (j = 0; j < size; j += 4) {
+		u32 expected = bar_test_pattern_with_offset(barno, j);
+
+		val = readl_relaxed(test->bar[barno] + j);
+		if (val != expected) {
+			dev_err(dev,
+				"BAR%d incorrect data at offset: %#x, got: %#x expected: %#x\n",
+				barno, j, val, expected);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+static bool pci_endpoint_test_bars(struct pci_endpoint_test *test)
+{
+	enum pci_barno bar;
+	bool ret;
+
+	/* Write all BARs in order (without reading). */
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
+		if (test->bar[bar])
+			pci_endpoint_test_bars_write_bar(test, bar);
+
+	/*
+	 * Read all BARs in order (without writing).
+	 * If there is an address translation issue on the EP, writing one BAR
+	 * might have overwritten another BAR. Ensure that this is not the case.
+	 * (Reading back the BAR directly after writing can not detect this.)
+	 */
+	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
+		if (test->bar[bar]) {
+			ret = pci_endpoint_test_bars_read_bar(test, bar);
+			if (!ret)
+				return ret;
+		}
+	}
+
+	return true;
+}
+
 static bool pci_endpoint_test_intx_irq(struct pci_endpoint_test *test)
 {
 	u32 val;
@@ -768,6 +853,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
 			goto ret;
 		ret = pci_endpoint_test_bar(test, bar);
 		break;
+	case PCITEST_BARS:
+		ret = pci_endpoint_test_bars(test);
+		break;
 	case PCITEST_INTX_IRQ:
 		ret = pci_endpoint_test_intx_irq(test);
 		break;
diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
index 94b46b043b536..acd261f498666 100644
--- a/include/uapi/linux/pcitest.h
+++ b/include/uapi/linux/pcitest.h
@@ -20,6 +20,7 @@
 #define PCITEST_MSIX		_IOW('P', 0x7, int)
 #define PCITEST_SET_IRQTYPE	_IOW('P', 0x8, int)
 #define PCITEST_GET_IRQTYPE	_IO('P', 0x9)
+#define PCITEST_BARS		_IO('P', 0xa)
 #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
 
 #define PCITEST_FLAGS_USE_DMA	0x00000001
diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
index 470258009ddc2..2ce56ea56202c 100644
--- a/tools/pci/pcitest.c
+++ b/tools/pci/pcitest.c
@@ -22,6 +22,7 @@ static char *irq[] = { "LEGACY", "MSI", "MSI-X" };
 struct pci_test {
 	char		*device;
 	char		barnum;
+	bool		consecutive_bar_test;
 	bool		legacyirq;
 	unsigned int	msinum;
 	unsigned int	msixnum;
@@ -57,6 +58,15 @@ static int run_test(struct pci_test *test)
 			fprintf(stdout, "%s\n", result[ret]);
 	}
 
+	if (test->consecutive_bar_test) {
+		ret = ioctl(fd, PCITEST_BARS);
+		fprintf(stdout, "Consecutive BAR test:\t\t");
+		if (ret < 0)
+			fprintf(stdout, "TEST FAILED\n");
+		else
+			fprintf(stdout, "%s\n", result[ret]);
+	}
+
 	if (test->set_irqtype) {
 		ret = ioctl(fd, PCITEST_SET_IRQTYPE, test->irqtype);
 		fprintf(stdout, "SET IRQ TYPE TO %s:\t\t", irq[test->irqtype]);
@@ -172,7 +182,7 @@ int main(int argc, char **argv)
 	/* set default endpoint device */
 	test->device = "/dev/pci-endpoint-test.0";
 
-	while ((c = getopt(argc, argv, "D:b:m:x:i:deIlhrwcs:")) != EOF)
+	while ((c = getopt(argc, argv, "D:b:Cm:x:i:deIlhrwcs:")) != EOF)
 	switch (c) {
 	case 'D':
 		test->device = optarg;
@@ -182,6 +192,9 @@ int main(int argc, char **argv)
 		if (test->barnum < 0 || test->barnum > 5)
 			goto usage;
 		continue;
+	case 'C':
+		test->consecutive_bar_test = true;
+		continue;
 	case 'l':
 		test->legacyirq = true;
 		continue;
@@ -230,6 +243,7 @@ int main(int argc, char **argv)
 			"Options:\n"
 			"\t-D <dev>		PCI endpoint test device {default: /dev/pci-endpoint-test.0}\n"
 			"\t-b <bar num>		BAR test (bar number between 0..5)\n"
+			"\t-C			Consecutive BAR test\n"
 			"\t-m <msi num>		MSI test (msi number between 1..32)\n"
 			"\t-x <msix num>	\tMSI-X test (msix number between 1..2048)\n"
 			"\t-i <irq type>	\tSet IRQ type (0 - Legacy, 1 - MSI, 2 - MSI-X)\n"
diff --git a/tools/pci/pcitest.sh b/tools/pci/pcitest.sh
index 75ed48ff29900..770f4d6df34b6 100644
--- a/tools/pci/pcitest.sh
+++ b/tools/pci/pcitest.sh
@@ -11,6 +11,7 @@ do
 	pcitest -b $bar
 	bar=`expr $bar + 1`
 done
+pcitest -C
 echo
 
 echo "Interrupt tests"
-- 
2.47.0

Re: [PATCH] misc: pci_endpoint_test: Add consecutive BAR test

[Manivannan Sadhasivam]

On Sat, Nov 16, 2024 at 04:20:45AM +0100, Niklas Cassel wrote:
> Add a more advanced BAR test that writes all BARs in one go, and then reads
> them back and verifies that the value matches the BAR number bitwise OR:ed
> with offset, this allows us to verify:
> -The BAR number was what we intended to read.
> -The offset was what we intended to read.
> 
> This allows us to detect potential address translation issues on the EP.
> 
> Reading back the BAR directly after writing will not allow us to detect the
> case where inbound address translation on the endpoint incorrectly causes
> multiple BARs to be redirected to the same memory region (within the EP).
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

- Mani

> ---
>  drivers/misc/pci_endpoint_test.c | 88 ++++++++++++++++++++++++++++++++
>  include/uapi/linux/pcitest.h     |  1 +
>  tools/pci/pcitest.c              | 16 +++++-
>  tools/pci/pcitest.sh             |  1 +
>  4 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee2..34cb54aef3d8b 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -322,6 +322,91 @@ static bool pci_endpoint_test_bar(struct pci_endpoint_test *test,
>  	return true;
>  }
>  
> +static u32 bar_test_pattern_with_offset(enum pci_barno barno, int offset)
> +{
> +	u32 val;
> +
> +	/* Keep the BAR pattern in the top byte. */
> +	val = bar_test_pattern[barno] & 0xff000000;
> +	/* Store the (partial) offset in the remaining bytes. */
> +	val |= offset & 0x00ffffff;
> +
> +	return val;
> +}
> +
> +static bool pci_endpoint_test_bars_write_bar(struct pci_endpoint_test *test,
> +					     enum pci_barno barno)
> +{
> +	struct pci_dev *pdev = test->pdev;
> +	int j, size;
> +
> +	size = pci_resource_len(pdev, barno);
> +
> +	if (barno == test->test_reg_bar)
> +		size = 0x4;
> +
> +	for (j = 0; j < size; j += 4)
> +		writel_relaxed(bar_test_pattern_with_offset(barno, j),
> +			       test->bar[barno] + j);
> +
> +	return true;
> +}
> +
> +static bool pci_endpoint_test_bars_read_bar(struct pci_endpoint_test *test,
> +					    enum pci_barno barno)
> +{
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +	int j, size;
> +	u32 val;
> +
> +	size = pci_resource_len(pdev, barno);
> +
> +	if (barno == test->test_reg_bar)
> +		size = 0x4;
> +
> +	for (j = 0; j < size; j += 4) {
> +		u32 expected = bar_test_pattern_with_offset(barno, j);
> +
> +		val = readl_relaxed(test->bar[barno] + j);
> +		if (val != expected) {
> +			dev_err(dev,
> +				"BAR%d incorrect data at offset: %#x, got: %#x expected: %#x\n",
> +				barno, j, val, expected);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +static bool pci_endpoint_test_bars(struct pci_endpoint_test *test)
> +{
> +	enum pci_barno bar;
> +	bool ret;
> +
> +	/* Write all BARs in order (without reading). */
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++)
> +		if (test->bar[bar])
> +			pci_endpoint_test_bars_write_bar(test, bar);
> +
> +	/*
> +	 * Read all BARs in order (without writing).
> +	 * If there is an address translation issue on the EP, writing one BAR
> +	 * might have overwritten another BAR. Ensure that this is not the case.
> +	 * (Reading back the BAR directly after writing can not detect this.)
> +	 */
> +	for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		if (test->bar[bar]) {
> +			ret = pci_endpoint_test_bars_read_bar(test, bar);
> +			if (!ret)
> +				return ret;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  static bool pci_endpoint_test_intx_irq(struct pci_endpoint_test *test)
>  {
>  	u32 val;
> @@ -768,6 +853,9 @@ static long pci_endpoint_test_ioctl(struct file *file, unsigned int cmd,
>  			goto ret;
>  		ret = pci_endpoint_test_bar(test, bar);
>  		break;
> +	case PCITEST_BARS:
> +		ret = pci_endpoint_test_bars(test);
> +		break;
>  	case PCITEST_INTX_IRQ:
>  		ret = pci_endpoint_test_intx_irq(test);
>  		break;
> diff --git a/include/uapi/linux/pcitest.h b/include/uapi/linux/pcitest.h
> index 94b46b043b536..acd261f498666 100644
> --- a/include/uapi/linux/pcitest.h
> +++ b/include/uapi/linux/pcitest.h
> @@ -20,6 +20,7 @@
>  #define PCITEST_MSIX		_IOW('P', 0x7, int)
>  #define PCITEST_SET_IRQTYPE	_IOW('P', 0x8, int)
>  #define PCITEST_GET_IRQTYPE	_IO('P', 0x9)
> +#define PCITEST_BARS		_IO('P', 0xa)
>  #define PCITEST_CLEAR_IRQ	_IO('P', 0x10)
>  
>  #define PCITEST_FLAGS_USE_DMA	0x00000001
> diff --git a/tools/pci/pcitest.c b/tools/pci/pcitest.c
> index 470258009ddc2..2ce56ea56202c 100644
> --- a/tools/pci/pcitest.c
> +++ b/tools/pci/pcitest.c
> @@ -22,6 +22,7 @@ static char *irq[] = { "LEGACY", "MSI", "MSI-X" };
>  struct pci_test {
>  	char		*device;
>  	char		barnum;
> +	bool		consecutive_bar_test;
>  	bool		legacyirq;
>  	unsigned int	msinum;
>  	unsigned int	msixnum;
> @@ -57,6 +58,15 @@ static int run_test(struct pci_test *test)
>  			fprintf(stdout, "%s\n", result[ret]);
>  	}
>  
> +	if (test->consecutive_bar_test) {
> +		ret = ioctl(fd, PCITEST_BARS);
> +		fprintf(stdout, "Consecutive BAR test:\t\t");
> +		if (ret < 0)
> +			fprintf(stdout, "TEST FAILED\n");
> +		else
> +			fprintf(stdout, "%s\n", result[ret]);
> +	}
> +
>  	if (test->set_irqtype) {
>  		ret = ioctl(fd, PCITEST_SET_IRQTYPE, test->irqtype);
>  		fprintf(stdout, "SET IRQ TYPE TO %s:\t\t", irq[test->irqtype]);
> @@ -172,7 +182,7 @@ int main(int argc, char **argv)
>  	/* set default endpoint device */
>  	test->device = "/dev/pci-endpoint-test.0";
>  
> -	while ((c = getopt(argc, argv, "D:b:m:x:i:deIlhrwcs:")) != EOF)
> +	while ((c = getopt(argc, argv, "D:b:Cm:x:i:deIlhrwcs:")) != EOF)
>  	switch (c) {
>  	case 'D':
>  		test->device = optarg;
> @@ -182,6 +192,9 @@ int main(int argc, char **argv)
>  		if (test->barnum < 0 || test->barnum > 5)
>  			goto usage;
>  		continue;
> +	case 'C':
> +		test->consecutive_bar_test = true;
> +		continue;
>  	case 'l':
>  		test->legacyirq = true;
>  		continue;
> @@ -230,6 +243,7 @@ int main(int argc, char **argv)
>  			"Options:\n"
>  			"\t-D <dev>		PCI endpoint test device {default: /dev/pci-endpoint-test.0}\n"
>  			"\t-b <bar num>		BAR test (bar number between 0..5)\n"
> +			"\t-C			Consecutive BAR test\n"
>  			"\t-m <msi num>		MSI test (msi number between 1..32)\n"
>  			"\t-x <msix num>	\tMSI-X test (msix number between 1..2048)\n"
>  			"\t-i <irq type>	\tSet IRQ type (0 - Legacy, 1 - MSI, 2 - MSI-X)\n"
> diff --git a/tools/pci/pcitest.sh b/tools/pci/pcitest.sh
> index 75ed48ff29900..770f4d6df34b6 100644
> --- a/tools/pci/pcitest.sh
> +++ b/tools/pci/pcitest.sh
> @@ -11,6 +11,7 @@ do
>  	pcitest -b $bar
>  	bar=`expr $bar + 1`
>  done
> +pcitest -C
>  echo
>  
>  echo "Interrupt tests"
> -- 
> 2.47.0
> 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH] misc: pci_endpoint_test: Add consecutive BAR test

[Niklas Cassel]

On Sun, Nov 24, 2024 at 08:09:22AM +0530, Manivannan Sadhasivam wrote:
> On Sat, Nov 16, 2024 at 04:20:45AM +0100, Niklas Cassel wrote:
> > Add a more advanced BAR test that writes all BARs in one go, and then reads
> > them back and verifies that the value matches the BAR number bitwise OR:ed
> > with offset, this allows us to verify:
> > -The BAR number was what we intended to read.
> > -The offset was what we intended to read.
> > 
> > This allows us to detect potential address translation issues on the EP.
> > 
> > Reading back the BAR directly after writing will not allow us to detect the
> > case where inbound address translation on the endpoint incorrectly causes
> > multiple BARs to be redirected to the same memory region (within the EP).
> > 
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks for the review!

Could this patch please be picked up?


Kind regards,
Niklas

Re: [PATCH] misc: pci_endpoint_test: Add consecutive BAR test

[Krzysztof Wilczyński]

Hello,

> Add a more advanced BAR test that writes all BARs in one go, and then reads
> them back and verifies that the value matches the BAR number bitwise OR:ed
> with offset, this allows us to verify:
> -The BAR number was what we intended to read.
> -The offset was what we intended to read.
> 
> This allows us to detect potential address translation issues on the EP.
> 
> Reading back the BAR directly after writing will not allow us to detect the
> case where inbound address translation on the endpoint incorrectly causes
> multiple BARs to be redirected to the same memory region (within the EP).

Applied to misc, thank you!

[01/01] misc: pci_endpoint_test: Add consecutive BAR test
        https://git.kernel.org/pci/pci/c/fe986f9ef0b9

	Krzysztof