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
