Message-ID: <8d74de55-5f89-f887-95bb-db2e8f77cfc0@igel.co.jp>
Date:   Mon, 5 Jun 2023 19:34:32 +0900
From:   Shunsuke Mie <mie@...l.co.jp>
To:     Damien Le Moal <dlemoal@...nel.org>,
        Kishon Vijay Abraham I <kvijayab@....com>,
        Jingoo Han <jingoohan1@...il.com>
Cc:     Gustavo Pimentel <gustavo.pimentel@...opsys.com>,
        Lorenzo Pieralisi <lpieralisi@...nel.org>,
        Rob Herring <robh@...nel.org>,
        Krzysztof Wilczyński <kw@...ux.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Kishon Vijay Abraham I <kishon@...nel.org>,
        Kunihiko Hayashi <hayashi.kunihiko@...ionext.com>,
        Hou Zhiqiang <Zhiqiang.Hou@....com>,
        Frank Li <Frank.Li@....com>, Li Chen <lchen@...arella.com>,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware
 map/unmaping


On 2023/06/02 21:21, Damien Le Moal wrote:
> On 6/2/23 18:42, Shunsuke Mie wrote:
>> Hi Damien,
>>
>> On 2023/06/02 8:43, Damien Le Moal wrote:
>>> On 6/2/23 00:06, Kishon Vijay Abraham I wrote:
>>>> Hi Shunsuke,
>>>>
>>>> On 1/13/2023 2:33 PM, Shunsuke Mie wrote:
>>>>> Add an align_mem operation to the EPC ops, which function is used to
>>>>> pci_epc_map/unmap_addr(). These change to enable mapping for any alignment
>>>>> restriction of EPC. The map function maps an aligned memory to include a
>>>>> requested memory region.
>>>> I'd prefer all the PCIe address alignment restriction be handled in the
>>>> endpoint function drivers and not inside the core layer (esp in map and
>>>> unmap calls).
>>> That is a really *bad* idea ! Most function drivers should be able to work with
>>> any EP controller hardware. Asking these drivers to support all the alignment
>>> peculiarities of every possible EP controller is impossible.
>>>
>>>> IMO, get the pci address alignment restriction using pci_epc_features.
>>>> And use a bigger size (based on alignment restriction) in
>>>> pci_epc_mem_alloc_addr() and access the allocated window using an offset
>>>> (based on alignment value). You can add separate helpers if required.
>>> That is too simplistic and not enough. Example: Rick and I working on an nvme
>>> function driver are facing a lot of issues with the EPC API for mem & mapping
>>> management because we have 0 control over the PCI address that the host will
>>> use. Alignment is all over the place, and the current EPC memory API
>>> restrictions (window size limitations) make it impossible to transparently
>>> handle all cases. We endup with NVMe command failures simply because of the API
>>> limitations.
>> I think so to.
>>
>> I'm also proposing virtio-console function driver[1]. I suppose the
>> virtio function
>> driver and your nvme function driver are the same in that the spec is
>> defined and
>> host side driver must work as is.
>>
>> [1]
>> https://lore.kernel.org/linux-pci/20230427104428.862643-4-mie@igel.co.jp/
>>
>>> And sure, we can modify that driver to better support the EP controller we are
>>> using (rockchip). But we need to support other EP controllers as well. So API
>>> changes are definitely needed. Working on that. That is not easy as the mapping
>>> API and its semantic impacts data transfers (memcpy_from|toio and DMA).
>>>
>>> I do have a patch that does something similar as this one, but at a much higher
>>> level with a helper function that gives the function driver the offset into the
>>> allocated memory region to use for mapping a particular PCI address. And then
>>> this helper is then in turn used into a new pci_epc_map() function which does
>>> mem alloc + mapping in one go based on the EPC constraints. That hides all
>>> alignment details to the function drivers, which greatlyu simplyfies the code.
>>> But that is not enough as alignment also implies that we have to deal with
>>> boundaries (due to limited window size) and so sometimes endpu failing a mapping
>>> that is too large because the host used a PCI address close to the boundary.
>>> More work is needed to have pci_epc_map() also hide that with tricks like
>>> allowing the allocation and mapping of multiple contiguous windows. So EPC ops
>>> API changes are also needed.
>> Could you submit the your changes if you can?
>>
>> I'd like to solve the current EPC limitation for the mapping in a better
>> way and avoid doing similar work.
> Will try to cleanup my patches and send an RFC next week. Need to rebase,
> cleanup etc. Not sure I can make it soon as I am busy with other things for 6.5
> right now.
>
> You can have a look at the work in progress here:
>
> https://github.com/damien-lemoal/linux/tree/rockpro64_ep_v21
>
> There are a bunch of epf and epc core patches as well as some rockchip driver
> patches. The first half of the patches on top of Linus 6.3 tag patch are already
> in pci-next.

I'd like to see the repo. Thank you for your cooperation.

Best,

Shunsuke

Powered by blists - more mailing lists