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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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

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