[PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Niklas Cassel]

Hello all,

The pci-epf-test driver recently moved to the pci_epc_mem_map() API.
This API call handle unaligned addresses seamlessly, if the EPC driver
being used has implemented the .align_addr callback.

This means that pci-epf-test no longer need any special padding to the
buffers that is allocated on the host side. (This was only done in order
to satisfy the EPC's alignment requirements.)

In fact, to test that the pci_epc_mem_map() API is working as intended,
it is important that the host side does not only provide buffers that
are nicely aligned.

However, since not all EPC drivers have implemented the .align_addr
callback, add support for capabilities in pci-epf-test, and if the
EPC driver implements the .align_addr callback, set a new
CAP_UNALIGNED_ACCESS capability. If CAP_UNALIGNED_ACCESS is set, do not
allocate overly sized buffers on the host side.

For EPC drivers that have not implemented the .align_addr callback, this
series will not introduce any functional changes.


Kind regards,
Niklas


Changes since v1:
-Improved commit message (thank you Frank)
-Renamed CAP_HAS_ALIGN_ADDR to CAP_UNALIGNED_ACCESS (thank you Damien)


Niklas Cassel (2):
  PCI: endpoint: pci-epf-test: Add support for capabilities
  misc: pci_endpoint_test: Add support for capabilities

 drivers/misc/pci_endpoint_test.c              | 21 +++++++++++++++++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++
 2 files changed, 40 insertions(+)

-- 
2.47.0

[PATCH v2 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Niklas Cassel]

The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
which allocates the backing memory using dma_alloc_coherent(), which will
return zeroed memory regardless of __GFP_ZERO was set or not.

This means that running a new version of pci-endpoint-test.c (host side)
with an old version of pci-epf-test.c (EP side) will not see any
capabilities being set (as intended), so this is backwards compatible.

Additionally, the EP side always allocates at least 128 bytes for the test
BAR (excluding the MSI-X table), this means that adding another register at
offset 0x30 is still within the 128 available bytes.

For now, we only add the CAP_UNALIGNED_ACCESS capability.

Set CAP_UNALIGNED_ACCESS if the EPC driver can handle any address (because
it implements the .align_addr callback).

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index ef6677f34116..7351289ecddd 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -44,6 +44,8 @@
 
 #define TIMER_RESOLUTION		1
 
+#define CAP_UNALIGNED_ACCESS		BIT(0)
+
 static struct workqueue_struct *kpcitest_workqueue;
 
 struct pci_epf_test {
@@ -74,6 +76,7 @@ struct pci_epf_test_reg {
 	u32	irq_type;
 	u32	irq_number;
 	u32	flags;
+	u32	caps;
 } __packed;
 
 static struct pci_epf_header test_header = {
@@ -739,6 +742,20 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
 	}
 }
 
+static void pci_epf_test_set_capabilities(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
+	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	struct pci_epc *epc = epf->epc;
+	u32 caps = 0;
+
+	if (epc->ops->align_addr)
+		caps |= CAP_UNALIGNED_ACCESS;
+
+	reg->caps = cpu_to_le32(caps);
+}
+
 static int pci_epf_test_epc_init(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -763,6 +780,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
 		}
 	}
 
+	pci_epf_test_set_capabilities(epf);
+
 	ret = pci_epf_test_set_bar(epf);
 	if (ret)
 		return ret;
-- 
2.47.0

[PATCH v2 2/2] misc: pci_endpoint_test: Add support for capabilities

[Niklas Cassel]

The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
which allocates the backing memory using dma_alloc_coherent(), which will
return zeroed memory regardless of __GFP_ZERO was set or not.

This means that running a new version of pci-endpoint-test.c (host side)
with an old version of pci-epf-test.c (EP side) will not see any
capabilities being set (as intended), so this is backwards compatible.

Additionally, the EP side always allocates at least 128 bytes for the test
BAR (excluding the MSI-X table), this means that adding another register at
offset 0x30 is still within the 128 available bytes.

For now, we only add the CAP_UNALIGNED_ACCESS capability.

If CAP_UNALIGNED_ACCESS is set, that means that the EP side supports
reading/writing to an address without any alignment requirements.

Thus, if CAP_UNALIGNED_ACCESS is set, make sure that the host side does
not add any extra padding to the buffers that we allocate (which was only
done in order to get the buffers to satisfy certain alignment requirements
by the endpoint controller).

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee..caae815ab75a 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -69,6 +69,9 @@
 #define PCI_ENDPOINT_TEST_FLAGS			0x2c
 #define FLAG_USE_DMA				BIT(0)
 
+#define PCI_ENDPOINT_TEST_CAPS			0x30
+#define CAP_UNALIGNED_ACCESS			BIT(0)
+
 #define PCI_DEVICE_ID_TI_AM654			0xb00c
 #define PCI_DEVICE_ID_TI_J7200			0xb00f
 #define PCI_DEVICE_ID_TI_AM64			0xb010
@@ -805,6 +808,22 @@ static const struct file_operations pci_endpoint_test_fops = {
 	.unlocked_ioctl = pci_endpoint_test_ioctl,
 };
 
+static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
+{
+	struct pci_dev *pdev = test->pdev;
+	struct device *dev = &pdev->dev;
+	u32 caps;
+	bool ep_can_do_unaligned_access;
+
+	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
+
+	ep_can_do_unaligned_access = caps & CAP_UNALIGNED_ACCESS;
+	dev_dbg(dev, "CAP_UNALIGNED_ACCESS: %d\n", ep_can_do_unaligned_access);
+
+	if (ep_can_do_unaligned_access)
+		test->alignment = 0;
+}
+
 static int pci_endpoint_test_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *ent)
 {
@@ -906,6 +925,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		goto err_kfree_test_name;
 	}
 
+	pci_endpoint_test_get_capabilities(test);
+
 	misc_device = &test->miscdev;
 	misc_device->minor = MISC_DYNAMIC_MINOR;
 	misc_device->name = kstrdup(name, GFP_KERNEL);
-- 
2.47.0

Re: [PATCH v2 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Frank Li]

On Thu, Nov 21, 2024 at 04:23:20PM +0100, Niklas Cassel wrote:
> The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
> which allocates the backing memory using dma_alloc_coherent(), which will
> return zeroed memory regardless of __GFP_ZERO was set or not.
>
> This means that running a new version of pci-endpoint-test.c (host side)
> with an old version of pci-epf-test.c (EP side) will not see any
> capabilities being set (as intended), so this is backwards compatible.
>
> Additionally, the EP side always allocates at least 128 bytes for the test
> BAR (excluding the MSI-X table), this means that adding another register at
> offset 0x30 is still within the 128 available bytes.
>
> For now, we only add the CAP_UNALIGNED_ACCESS capability.
>
> Set CAP_UNALIGNED_ACCESS if the EPC driver can handle any address (because
> it implements the .align_addr callback).
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index ef6677f34116..7351289ecddd 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -44,6 +44,8 @@
>
>  #define TIMER_RESOLUTION		1
>
> +#define CAP_UNALIGNED_ACCESS		BIT(0)
> +
>  static struct workqueue_struct *kpcitest_workqueue;
>
>  struct pci_epf_test {
> @@ -74,6 +76,7 @@ struct pci_epf_test_reg {
>  	u32	irq_type;
>  	u32	irq_number;
>  	u32	flags;
> +	u32	caps;
>  } __packed;
>
>  static struct pci_epf_header test_header = {
> @@ -739,6 +742,20 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
>  	}
>  }
>
> +static void pci_epf_test_set_capabilities(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> +	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +	struct pci_epc *epc = epf->epc;
> +	u32 caps = 0;
> +
> +	if (epc->ops->align_addr)
> +		caps |= CAP_UNALIGNED_ACCESS;
> +
> +	reg->caps = cpu_to_le32(caps);
> +}
> +
>  static int pci_epf_test_epc_init(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -763,6 +780,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
>  		}
>  	}
>
> +	pci_epf_test_set_capabilities(epf);
> +
>  	ret = pci_epf_test_set_bar(epf);
>  	if (ret)
>  		return ret;
> --
> 2.47.0
>

Re: [PATCH v2 2/2] misc: pci_endpoint_test: Add support for capabilities

[Frank Li]

On Thu, Nov 21, 2024 at 04:23:21PM +0100, Niklas Cassel wrote:
> The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
> which allocates the backing memory using dma_alloc_coherent(), which will
> return zeroed memory regardless of __GFP_ZERO was set or not.
>
> This means that running a new version of pci-endpoint-test.c (host side)
> with an old version of pci-epf-test.c (EP side) will not see any
> capabilities being set (as intended), so this is backwards compatible.
>
> Additionally, the EP side always allocates at least 128 bytes for the test
> BAR (excluding the MSI-X table), this means that adding another register at
> offset 0x30 is still within the 128 available bytes.
>
> For now, we only add the CAP_UNALIGNED_ACCESS capability.
>
> If CAP_UNALIGNED_ACCESS is set, that means that the EP side supports
> reading/writing to an address without any alignment requirements.
>
> Thus, if CAP_UNALIGNED_ACCESS is set, make sure that the host side does
> not add any extra padding to the buffers that we allocate (which was only
> done in order to get the buffers to satisfy certain alignment requirements
> by the endpoint controller).
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

I think 4byte align for readl/writel still required?

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee..caae815ab75a 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -69,6 +69,9 @@
>  #define PCI_ENDPOINT_TEST_FLAGS			0x2c
>  #define FLAG_USE_DMA				BIT(0)
>
> +#define PCI_ENDPOINT_TEST_CAPS			0x30
> +#define CAP_UNALIGNED_ACCESS			BIT(0)
> +
>  #define PCI_DEVICE_ID_TI_AM654			0xb00c
>  #define PCI_DEVICE_ID_TI_J7200			0xb00f
>  #define PCI_DEVICE_ID_TI_AM64			0xb010
> @@ -805,6 +808,22 @@ static const struct file_operations pci_endpoint_test_fops = {
>  	.unlocked_ioctl = pci_endpoint_test_ioctl,
>  };
>
> +static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
> +{
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +	u32 caps;
> +	bool ep_can_do_unaligned_access;
> +
> +	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> +
> +	ep_can_do_unaligned_access = caps & CAP_UNALIGNED_ACCESS;
> +	dev_dbg(dev, "CAP_UNALIGNED_ACCESS: %d\n", ep_can_do_unaligned_access);
> +
> +	if (ep_can_do_unaligned_access)
> +		test->alignment = 0;
> +}
> +
>  static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  				   const struct pci_device_id *ent)
>  {
> @@ -906,6 +925,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  		goto err_kfree_test_name;
>  	}
>
> +	pci_endpoint_test_get_capabilities(test);
> +
>  	misc_device = &test->miscdev;
>  	misc_device->minor = MISC_DYNAMIC_MINOR;
>  	misc_device->name = kstrdup(name, GFP_KERNEL);
> --
> 2.47.0
>

Re: [PATCH v2 2/2] misc: pci_endpoint_test: Add support for capabilities

[Niklas Cassel]

On Thu, Nov 21, 2024 at 02:43:19PM -0500, Frank Li wrote:
> On Thu, Nov 21, 2024 at 04:23:21PM +0100, Niklas Cassel wrote:
> > The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
> > which allocates the backing memory using dma_alloc_coherent(), which will
> > return zeroed memory regardless of __GFP_ZERO was set or not.
> >
> > This means that running a new version of pci-endpoint-test.c (host side)
> > with an old version of pci-epf-test.c (EP side) will not see any
> > capabilities being set (as intended), so this is backwards compatible.
> >
> > Additionally, the EP side always allocates at least 128 bytes for the test
> > BAR (excluding the MSI-X table), this means that adding another register at
> > offset 0x30 is still within the 128 available bytes.
> >
> > For now, we only add the CAP_UNALIGNED_ACCESS capability.
> >
> > If CAP_UNALIGNED_ACCESS is set, that means that the EP side supports
> > reading/writing to an address without any alignment requirements.
> >
> > Thus, if CAP_UNALIGNED_ACCESS is set, make sure that the host side does
> > not add any extra padding to the buffers that we allocate (which was only
> > done in order to get the buffers to satisfy certain alignment requirements
> > by the endpoint controller).
> >
> > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> 
> I think 4byte align for readl/writel still required?

I've been running this series on rk3588 (which has CAP_UNALIGNED_ACCESS set),
so alignment will be set to 0 in pci_endpoint_test_{copy,read,write}().
When running:
pcitest -r -s 1
pcitest -r -s 2
pcitest -r -s 3
pcitest -r -s 4

many, many times, and dma_alloc_coherent() always returned an address that
was 4 byte aligned, regardless of input size.


Additionally, when setting alignment to 0, the code in
pci_endpoint_test_{copy,read,write}() will behave exactly as it did before
the "alignment support" was added in commit:
https://github.com/torvalds/linux/commit/13107c60681f19fec25af93de86442ac9373e43f


So I think this patch is good as is.


Kind regards,
Niklas

> 
> Reviewed-by: Frank Li <Frank.Li@nxp.com>
> 
> > ---
> >  drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >

Re: [PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Manivannan Sadhasivam]

On Thu, Nov 21, 2024 at 04:23:19PM +0100, Niklas Cassel wrote:
> Hello all,
> 
> The pci-epf-test driver recently moved to the pci_epc_mem_map() API.
> This API call handle unaligned addresses seamlessly, if the EPC driver
> being used has implemented the .align_addr callback.
> 
> This means that pci-epf-test no longer need any special padding to the
> buffers that is allocated on the host side. (This was only done in order
> to satisfy the EPC's alignment requirements.)
> 
> In fact, to test that the pci_epc_mem_map() API is working as intended,
> it is important that the host side does not only provide buffers that
> are nicely aligned.
> 

I don't agree with the idea of testing the endpoint internal API behavior with
the pci_endpoint_test driver. The driver is supposed to test the functionality
of the endpoint, which it already does. By adding these kind of checks, we are
just going to make the driver bloat.

I suppose if the API behavior is wrong, then it should be caught in the existing
READ/WRITE tests, no?

> However, since not all EPC drivers have implemented the .align_addr
> callback, add support for capabilities in pci-epf-test, and if the
> EPC driver implements the .align_addr callback, set a new
> CAP_UNALIGNED_ACCESS capability. If CAP_UNALIGNED_ACCESS is set, do not
> allocate overly sized buffers on the host side.
> 

This also feels wrong to me. The host driver should care about the alignment
restrictions of the endpoint and then allocate the buffers accordingly, not the
other way.

That being said, I really like to get rid of the hardcoded
'pci_endpoint_test_data::alignment' field and make the driver learn about it
dynamically. I'm just thinking out loud here.

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

Re: [PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Niklas Cassel]

Hello Mani,

On Tue, Nov 26, 2024 at 06:50:20PM +0530, Manivannan Sadhasivam wrote:
> On Thu, Nov 21, 2024 at 04:23:19PM +0100, Niklas Cassel wrote:
> > Hello all,
> > 
> > The pci-epf-test driver recently moved to the pci_epc_mem_map() API.
> > This API call handle unaligned addresses seamlessly, if the EPC driver
> > being used has implemented the .align_addr callback.
> > 
> > This means that pci-epf-test no longer need any special padding to the
> > buffers that is allocated on the host side. (This was only done in order
> > to satisfy the EPC's alignment requirements.)
> > 
> > In fact, to test that the pci_epc_mem_map() API is working as intended,
> > it is important that the host side does not only provide buffers that
> > are nicely aligned.
> > 
> 
> I don't agree with the idea of testing the endpoint internal API behavior with
> the pci_endpoint_test driver. The driver is supposed to test the functionality
> of the endpoint, which it already does. By adding these kind of checks, we are
> just going to make the driver bloat.
> 
> I suppose if the API behavior is wrong, then it should be caught in the existing
> READ/WRITE tests, no?

As of today, certain EPC drivers have implemented .align_addr():
drivers/pci/controller/dwc/pcie-designware-ep.c (i.e. all DWC based EPC driver)
drivers/pci/controller/pcie-rockchip-ep.c

Drivers currently missing .align_addr():
drivers/pci/controller/cadence/pcie-cadence-ep.c
drivers/pci/controller/pcie-rcar-ep.c

For drivers that are implementing .align_addr(), there is currently nothing
that verifies that the .align_addr() is actually working
(because the host side driver unconditionally adds padding for the buffers.)

That is what this patch is trying to fix.


> 
> > However, since not all EPC drivers have implemented the .align_addr
> > callback, add support for capabilities in pci-epf-test, and if the
> > EPC driver implements the .align_addr callback, set a new
> > CAP_UNALIGNED_ACCESS capability. If CAP_UNALIGNED_ACCESS is set, do not
> > allocate overly sized buffers on the host side.
> > 
> 
> This also feels wrong to me. The host driver should care about the alignment
> restrictions of the endpoint and then allocate the buffers accordingly, not the
> other way.

In my opinion, originally the drivers/misc/pci_endpoint_test.c host side driver
had no special padding of the allocated buffers on the host side.

Then when support for an EPC which had an alignment requirement on the outbound
iATU, Kishon decided to add padding to the host side buffers in commit:
13107c60681f ("misc: pci_endpoint_test: Add support to provide aligned buffer addresses")

such that the EP could perform I/O to the host without any changes needed on EP
side. I think that this commit/approach was a mistake.

The proper solution for this would have been to add an EPC side API which maps
the "aligned" address, and then writes to an offset within that mapping.

This is what we have implemented in commits (which is now in Torvalds tree):
ce1dfe6d3289 ("PCI: endpoint: Introduce pci_epc_mem_map()/unmap()")
and
08cac1006bfc ("PCI: endpoint: test: Use pci_epc_mem_map/unmap()")


IMO, an EPF driver should be able to write to any PCI address.
(Especially since this can be solved trivially by EPF drivers simply using
pci_epc_mem_map()/unmap())

E.g. the upcoming NVMe EPF driver uses the NVMe host side driver.
This host side driver does no magic padding on host side for endpoints
(NVMe controllers) that have an iATU with outbound address alignment
restriction.

Imagine e.g. another EPF driver, for another existing protocol, e.g. AHCI.
Modifying existing generic Linux drivers (e.g. the AHCI driver), simply because
some random EPC driver has a specific outbound alignment requirement (that can
simply be ignored by using pci_epc_mem_map/unmap()) does not make sense IMO.


> 
> That being said, I really like to get rid of the hardcoded
> 'pci_endpoint_test_data::alignment' field and make the driver learn about it
> dynamically. I'm just thinking out loud here.

That would certainly be possible, by simply dedicating a new register to that
in the test BAR (struct pci_epf_test_reg). However, I think that that would be
a worse alternative compared to what this series is proposing.

The only ugliness in my opinion is that we are setting the CAP_UNALIGNED_ACCESS
capability based on if an EPC driver has implemented the .align_addr callback.

If we could simply implement .align_addr() in the two missing EPC drivers:
drivers/pci/controller/cadence/pcie-cadence-ep.c
drivers/pci/controller/pcie-rcar-ep.c

pci-epf-test.c could set the CAP_UNALIGNED_ACCESS capability unconditionally.

However, I do not have the hardware for those two drivers, so I cannot
implement .align_addr() for those myself.

So in order to be able to move forward, I think that simply letting
pci-epf-test.c check if the EPC driver which is currently in use has
implemented the .align_addr callback, is a tolerable ugliness.

Once all EPC drivers have implemented .align_addr(), we could change
pci-epf-test.c to unconditionally set the CAP_UNALIGNED_ACCESS capability.


Kind regards,
Niklas

Re: [PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Niklas Cassel]

On Wed, Nov 27, 2024 at 12:23:02PM +0100, Niklas Cassel wrote:
> 
> Once all EPC drivers have implemented .align_addr(), we could change
> pci-epf-test.c to unconditionally set the CAP_UNALIGNED_ACCESS capability.

...and it would also allow us to get rid of everything related to alignment
in drivers/misc/pci_endpoint_test.c.


Kind regards,
Niklas

Re: [PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Damien Le Moal]

On 11/27/24 20:23, Niklas Cassel wrote:

[...]

> IMO, an EPF driver should be able to write to any PCI address.
> (Especially since this can be solved trivially by EPF drivers simply using
> pci_epc_mem_map()/unmap())
> 
> E.g. the upcoming NVMe EPF driver uses the NVMe host side driver.
> This host side driver does no magic padding on host side for endpoints
> (NVMe controllers) that have an iATU with outbound address alignment
> restriction.

Not to mention that per NVMe specs, there should be no alignment constraints for
IO buffers. Anything is OK. And you can see it running this EPF driver while
going through a host boot sequence and using nvme-cli: the BIOS probing the NVMe
drive, GRUB looking at the NVMe drive and nvme-cli using on-stack buffers result
is high randomness of the buffer alignments. Trying to get that to work in all
cases led to the pci_epc_mem_map()/unmap() API :)

So having the test EPF driver allowing testing such unaligned pattern would
definitely help solidify the endpoint framework and its endpoint controller drivers.

> Imagine e.g. another EPF driver, for another existing protocol, e.g. AHCI.
> Modifying existing generic Linux drivers (e.g. the AHCI driver), simply because
> some random EPC driver has a specific outbound alignment requirement (that can
> simply be ignored by using pci_epc_mem_map/unmap()) does not make sense IMO.
> 
> 
>>
>> That being said, I really like to get rid of the hardcoded
>> 'pci_endpoint_test_data::alignment' field and make the driver learn about it
>> dynamically. I'm just thinking out loud here.
> 
> That would certainly be possible, by simply dedicating a new register to that
> in the test BAR (struct pci_epf_test_reg). However, I think that that would be
> a worse alternative compared to what this series is proposing.
> 
> The only ugliness in my opinion is that we are setting the CAP_UNALIGNED_ACCESS
> capability based on if an EPC driver has implemented the .align_addr callback.
> 
> If we could simply implement .align_addr() in the two missing EPC drivers:
> drivers/pci/controller/cadence/pcie-cadence-ep.c

At least for this one, it is likely exactly the same as for
drivers/pci/controller/pcie-rockchip-ep.c. The code for these 2 drivers is
suspiciously similar, which strongly hints at the fact that the HW is most
likely based on the same IP blocks.

> drivers/pci/controller/pcie-rcar-ep.c
> 
> pci-epf-test.c could set the CAP_UNALIGNED_ACCESS capability unconditionally.
> 
> However, I do not have the hardware for those two drivers, so I cannot
> implement .align_addr() for those myself.
> 
> So in order to be able to move forward, I think that simply letting
> pci-epf-test.c check if the EPC driver which is currently in use has
> implemented the .align_addr callback, is a tolerable ugliness.
> 
> Once all EPC drivers have implemented .align_addr(), we could change
> pci-epf-test.c to unconditionally set the CAP_UNALIGNED_ACCESS capability.
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Niklas Cassel]

On Wed, Nov 27, 2024 at 12:32:54PM +0100, Niklas Cassel wrote:
> On Wed, Nov 27, 2024 at 12:23:02PM +0100, Niklas Cassel wrote:
> > 
> > Once all EPC drivers have implemented .align_addr(), we could change
> > pci-epf-test.c to unconditionally set the CAP_UNALIGNED_ACCESS capability.
> 
> ...and it would also allow us to get rid of everything related to alignment
> in drivers/misc/pci_endpoint_test.c.

On futher thought, we probably want to keep that code in
drivers/misc/pci_endpoint_test.c, such that it can be backwards compatible
with pci-epf-test.c drivers that are old (before CAP_UNALIGNED_ACCESS).

So I still think that having a CAP_UNALIGNED_ACCESS is the best we can do.


Kind regards,
Niklas

Re: [PATCH v2 0/2] PCI endpoint test: Add support for capabilities

[Manivannan Sadhasivam]

On Wed, Nov 27, 2024 at 12:23:02PM +0100, Niklas Cassel wrote:
> Hello Mani,
> 
> On Tue, Nov 26, 2024 at 06:50:20PM +0530, Manivannan Sadhasivam wrote:
> > On Thu, Nov 21, 2024 at 04:23:19PM +0100, Niklas Cassel wrote:
> > > Hello all,
> > > 
> > > The pci-epf-test driver recently moved to the pci_epc_mem_map() API.
> > > This API call handle unaligned addresses seamlessly, if the EPC driver
> > > being used has implemented the .align_addr callback.
> > > 
> > > This means that pci-epf-test no longer need any special padding to the
> > > buffers that is allocated on the host side. (This was only done in order
> > > to satisfy the EPC's alignment requirements.)
> > > 
> > > In fact, to test that the pci_epc_mem_map() API is working as intended,
> > > it is important that the host side does not only provide buffers that
> > > are nicely aligned.
> > > 
> > 
> > I don't agree with the idea of testing the endpoint internal API behavior with
> > the pci_endpoint_test driver. The driver is supposed to test the functionality
> > of the endpoint, which it already does. By adding these kind of checks, we are
> > just going to make the driver bloat.
> > 
> > I suppose if the API behavior is wrong, then it should be caught in the existing
> > READ/WRITE tests, no?
> 
> As of today, certain EPC drivers have implemented .align_addr():
> drivers/pci/controller/dwc/pcie-designware-ep.c (i.e. all DWC based EPC driver)
> drivers/pci/controller/pcie-rockchip-ep.c
> 
> Drivers currently missing .align_addr():
> drivers/pci/controller/cadence/pcie-cadence-ep.c
> drivers/pci/controller/pcie-rcar-ep.c
> 
> For drivers that are implementing .align_addr(), there is currently nothing
> that verifies that the .align_addr() is actually working
> (because the host side driver unconditionally adds padding for the buffers.)
> 
> That is what this patch is trying to fix.
> 
> 
> > 
> > > However, since not all EPC drivers have implemented the .align_addr
> > > callback, add support for capabilities in pci-epf-test, and if the
> > > EPC driver implements the .align_addr callback, set a new
> > > CAP_UNALIGNED_ACCESS capability. If CAP_UNALIGNED_ACCESS is set, do not
> > > allocate overly sized buffers on the host side.
> > > 
> > 
> > This also feels wrong to me. The host driver should care about the alignment
> > restrictions of the endpoint and then allocate the buffers accordingly, not the
> > other way.
> 
> In my opinion, originally the drivers/misc/pci_endpoint_test.c host side driver
> had no special padding of the allocated buffers on the host side.
> 
> Then when support for an EPC which had an alignment requirement on the outbound
> iATU, Kishon decided to add padding to the host side buffers in commit:
> 13107c60681f ("misc: pci_endpoint_test: Add support to provide aligned buffer addresses")
> 
> such that the EP could perform I/O to the host without any changes needed on EP
> side. I think that this commit/approach was a mistake.
> 
> The proper solution for this would have been to add an EPC side API which maps
> the "aligned" address, and then writes to an offset within that mapping.
> 
> This is what we have implemented in commits (which is now in Torvalds tree):
> ce1dfe6d3289 ("PCI: endpoint: Introduce pci_epc_mem_map()/unmap()")
> and
> 08cac1006bfc ("PCI: endpoint: test: Use pci_epc_mem_map/unmap()")
> 
> 
> IMO, an EPF driver should be able to write to any PCI address.
> (Especially since this can be solved trivially by EPF drivers simply using
> pci_epc_mem_map()/unmap())
> 
> E.g. the upcoming NVMe EPF driver uses the NVMe host side driver.
> This host side driver does no magic padding on host side for endpoints
> (NVMe controllers) that have an iATU with outbound address alignment
> restriction.
> 
> Imagine e.g. another EPF driver, for another existing protocol, e.g. AHCI.
> Modifying existing generic Linux drivers (e.g. the AHCI driver), simply because
> some random EPC driver has a specific outbound alignment requirement (that can
> simply be ignored by using pci_epc_mem_map/unmap()) does not make sense IMO.
> 

Sounds fair. Thanks for the explanation.

> 
> > 
> > That being said, I really like to get rid of the hardcoded
> > 'pci_endpoint_test_data::alignment' field and make the driver learn about it
> > dynamically. I'm just thinking out loud here.
> 
> That would certainly be possible, by simply dedicating a new register to that
> in the test BAR (struct pci_epf_test_reg). However, I think that that would be
> a worse alternative compared to what this series is proposing.
> 
> The only ugliness in my opinion is that we are setting the CAP_UNALIGNED_ACCESS
> capability based on if an EPC driver has implemented the .align_addr callback.
> 
> If we could simply implement .align_addr() in the two missing EPC drivers:
> drivers/pci/controller/cadence/pcie-cadence-ep.c
> drivers/pci/controller/pcie-rcar-ep.c
> 
> pci-epf-test.c could set the CAP_UNALIGNED_ACCESS capability unconditionally.
> 
> However, I do not have the hardware for those two drivers, so I cannot
> implement .align_addr() for those myself.
> 
> So in order to be able to move forward, I think that simply letting
> pci-epf-test.c check if the EPC driver which is currently in use has
> implemented the .align_addr callback, is a tolerable ugliness.
> 
> Once all EPC drivers have implemented .align_addr(), we could change
> pci-epf-test.c to unconditionally set the CAP_UNALIGNED_ACCESS capability.
> 

Rather not as you mentioned in following threads to keep backwards compatibility
with old EPF drivers.

- Mani

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

Re: [PATCH v2 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Manivannan Sadhasivam]

On Thu, Nov 21, 2024 at 04:23:20PM +0100, Niklas Cassel wrote:
> The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
> which allocates the backing memory using dma_alloc_coherent(), which will
> return zeroed memory regardless of __GFP_ZERO was set or not.
> 
> This means that running a new version of pci-endpoint-test.c (host side)
> with an old version of pci-epf-test.c (EP side) will not see any
> capabilities being set (as intended), so this is backwards compatible.
> 
> Additionally, the EP side always allocates at least 128 bytes for the test
> BAR (excluding the MSI-X table), this means that adding another register at
> offset 0x30 is still within the 128 available bytes.
> 
> For now, we only add the CAP_UNALIGNED_ACCESS capability.
> 
> Set CAP_UNALIGNED_ACCESS if the EPC driver can handle any address (because
> it implements the .align_addr callback).
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

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

Just a small nit below.

> ---
>  drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index ef6677f34116..7351289ecddd 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -44,6 +44,8 @@
>  
>  #define TIMER_RESOLUTION		1
>  
> +#define CAP_UNALIGNED_ACCESS		BIT(0)
> +
>  static struct workqueue_struct *kpcitest_workqueue;
>  
>  struct pci_epf_test {
> @@ -74,6 +76,7 @@ struct pci_epf_test_reg {
>  	u32	irq_type;
>  	u32	irq_number;
>  	u32	flags;
> +	u32	caps;

Can we rename the 'magic' register? It is not used since the beginning and I
don't know if we will ever have a usecase for it.

- Mani

>  } __packed;
>  
>  static struct pci_epf_header test_header = {
> @@ -739,6 +742,20 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
>  	}
>  }
>  
> +static void pci_epf_test_set_capabilities(struct pci_epf *epf)
> +{
> +	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> +	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> +	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
> +	struct pci_epc *epc = epf->epc;
> +	u32 caps = 0;
> +
> +	if (epc->ops->align_addr)
> +		caps |= CAP_UNALIGNED_ACCESS;
> +
> +	reg->caps = cpu_to_le32(caps);
> +}
> +
>  static int pci_epf_test_epc_init(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> @@ -763,6 +780,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
>  		}
>  	}
>  
> +	pci_epf_test_set_capabilities(epf);
> +
>  	ret = pci_epf_test_set_bar(epf);
>  	if (ret)
>  		return ret;
> -- 
> 2.47.0
> 

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

Re: [PATCH v2 2/2] misc: pci_endpoint_test: Add support for capabilities

[Manivannan Sadhasivam]

On Thu, Nov 21, 2024 at 04:23:21PM +0100, Niklas Cassel wrote:
> The test BAR is on the EP side is allocated using pci_epf_alloc_space(),
> which allocates the backing memory using dma_alloc_coherent(), which will
> return zeroed memory regardless of __GFP_ZERO was set or not.
> 
> This means that running a new version of pci-endpoint-test.c (host side)
> with an old version of pci-epf-test.c (EP side) will not see any
> capabilities being set (as intended), so this is backwards compatible.
> 
> Additionally, the EP side always allocates at least 128 bytes for the test
> BAR (excluding the MSI-X table), this means that adding another register at
> offset 0x30 is still within the 128 available bytes.
> 
> For now, we only add the CAP_UNALIGNED_ACCESS capability.
> 
> If CAP_UNALIGNED_ACCESS is set, that means that the EP side supports
> reading/writing to an address without any alignment requirements.
> 
> Thus, if CAP_UNALIGNED_ACCESS is set, make sure that the host side does
> not add any extra padding to the buffers that we allocate (which was only
> done in order to get the buffers to satisfy certain alignment requirements
> by the endpoint controller).
> 
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

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

One suggestion below.

> ---
>  drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index 3aaaf47fa4ee..caae815ab75a 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -69,6 +69,9 @@
>  #define PCI_ENDPOINT_TEST_FLAGS			0x2c
>  #define FLAG_USE_DMA				BIT(0)
>  
> +#define PCI_ENDPOINT_TEST_CAPS			0x30
> +#define CAP_UNALIGNED_ACCESS			BIT(0)
> +
>  #define PCI_DEVICE_ID_TI_AM654			0xb00c
>  #define PCI_DEVICE_ID_TI_J7200			0xb00f
>  #define PCI_DEVICE_ID_TI_AM64			0xb010
> @@ -805,6 +808,22 @@ static const struct file_operations pci_endpoint_test_fops = {
>  	.unlocked_ioctl = pci_endpoint_test_ioctl,
>  };
>  
> +static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
> +{
> +	struct pci_dev *pdev = test->pdev;
> +	struct device *dev = &pdev->dev;
> +	u32 caps;
> +	bool ep_can_do_unaligned_access;
> +
> +	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> +
> +	ep_can_do_unaligned_access = caps & CAP_UNALIGNED_ACCESS;
> +	dev_dbg(dev, "CAP_UNALIGNED_ACCESS: %d\n", ep_can_do_unaligned_access);

IDK if the users really need to know about this flag, nor it will assist in any
debugging. Otherwise, I'd suggest to drop this debug print and just do:

	if (pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS))
		test->alignment = 0;

- Mani

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

Re: [PATCH v2 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Niklas Cassel]

Hello Mani,

On Sat, Nov 30, 2024 at 01:42:45PM +0530, Manivannan Sadhasivam wrote:
> >  
> >  struct pci_epf_test {
> > @@ -74,6 +76,7 @@ struct pci_epf_test_reg {
> >  	u32	irq_type;
> >  	u32	irq_number;
> >  	u32	flags;
> > +	u32	caps;
> 
> Can we rename the 'magic' register? It is not used since the beginning and I
> don't know if we will ever have a usecase for it.

It is actually used!

When doing PCITEST_BAR (pci_endpoint_test_bar()),
and barno == test->test_reg_bar, we are only filling the first 4 bytes,
rather than filling the whole BAR:
https://github.com/torvalds/linux/blob/v6.13-rc1/drivers/misc/pci_endpoint_test.c#L293-L294

These first 4 bytes are stored in the magic register.

I do agree that the magic register name is slightly misleading, but that
seems completely unrelated to this patch.

If you can come up with a better name, send a patch and you shall have
my Reviewed-by tag :)


Kind regards,
Niklas

Re: [PATCH v2 2/2] misc: pci_endpoint_test: Add support for capabilities

[Niklas Cassel]

On Sat, Nov 30, 2024 at 01:51:19PM +0530, Manivannan Sadhasivam wrote:
> >  
> > +static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
> > +{
> > +	struct pci_dev *pdev = test->pdev;
> > +	struct device *dev = &pdev->dev;
> > +	u32 caps;
> > +	bool ep_can_do_unaligned_access;
> > +
> > +	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> > +
> > +	ep_can_do_unaligned_access = caps & CAP_UNALIGNED_ACCESS;
> > +	dev_dbg(dev, "CAP_UNALIGNED_ACCESS: %d\n", ep_can_do_unaligned_access);
> 
> IDK if the users really need to know about this flag, nor it will assist in any
> debugging. Otherwise, I'd suggest to drop this debug print and just do:
> 
> 	if (pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS))
> 		test->alignment = 0;
> 
> - Mani

I do think that there is value in having a debug print that prints the
capabilities, especially if more caps are added in the future.

Let me send a v3 that dumps all caps (as hex) in a single print instead,
i.e. simply:

caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps);


Kind regards,
Niklas

Re: [PATCH v2 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Manivannan Sadhasivam]

On Tue, Dec 03, 2024 at 05:43:10AM +0100, Niklas Cassel wrote:
> Hello Mani,
> 
> On Sat, Nov 30, 2024 at 01:42:45PM +0530, Manivannan Sadhasivam wrote:
> > >  
> > >  struct pci_epf_test {
> > > @@ -74,6 +76,7 @@ struct pci_epf_test_reg {
> > >  	u32	irq_type;
> > >  	u32	irq_number;
> > >  	u32	flags;
> > > +	u32	caps;
> > 
> > Can we rename the 'magic' register? It is not used since the beginning and I
> > don't know if we will ever have a usecase for it.
> 
> It is actually used!
> 
> When doing PCITEST_BAR (pci_endpoint_test_bar()),
> and barno == test->test_reg_bar, we are only filling the first 4 bytes,
> rather than filling the whole BAR:
> https://github.com/torvalds/linux/blob/v6.13-rc1/drivers/misc/pci_endpoint_test.c#L293-L294
> 
> These first 4 bytes are stored in the magic register.
> 

heh, not so evident... thanks anyway.

> I do agree that the magic register name is slightly misleading, but that
> seems completely unrelated to this patch.
> 
> If you can come up with a better name, send a patch and you shall have
> my Reviewed-by tag :)
> 

How about 'scratchpad'?

- Mani

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

Re: [PATCH v2 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Niklas Cassel]

On Sun, Dec 08, 2024 at 06:12:08PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Dec 03, 2024 at 05:43:10AM +0100, Niklas Cassel wrote:
> > Hello Mani,
> > 
> > On Sat, Nov 30, 2024 at 01:42:45PM +0530, Manivannan Sadhasivam wrote:
> > > >  
> > > >  struct pci_epf_test {
> > > > @@ -74,6 +76,7 @@ struct pci_epf_test_reg {
> > > >  	u32	irq_type;
> > > >  	u32	irq_number;
> > > >  	u32	flags;
> > > > +	u32	caps;
> > > 
> > > Can we rename the 'magic' register? It is not used since the beginning and I
> > > don't know if we will ever have a usecase for it.
> > 
> > It is actually used!
> > 
> > When doing PCITEST_BAR (pci_endpoint_test_bar()),
> > and barno == test->test_reg_bar, we are only filling the first 4 bytes,
> > rather than filling the whole BAR:
> > https://github.com/torvalds/linux/blob/v6.13-rc1/drivers/misc/pci_endpoint_test.c#L293-L294
> > 
> > These first 4 bytes are stored in the magic register.
> > 
> 
> heh, not so evident... thanks anyway.
> 
> > I do agree that the magic register name is slightly misleading, but that
> > seems completely unrelated to this patch.
> > 
> > If you can come up with a better name, send a patch and you shall have
> > my Reviewed-by tag :)
> > 
> 
> How about 'scratchpad'?

Sounds good to me.

When sending a patch, don't forget to update:

Documentation/PCI/endpoint/pci-test-function.rst:       1) PCI_ENDPOINT_TEST_MAGIC
Documentation/PCI/endpoint/pci-test-function.rst:* PCI_ENDPOINT_TEST_MAGIC
drivers/misc/pci_endpoint_test.c:#define PCI_ENDPOINT_TEST_MAGIC

in addition to renaming the struct member in struct pci_epf_test_reg.


Kind regards,
Niklas