Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <52b8f850-af8c-1971-9729-c5de37875bf9@amd.com>
Date:   Fri, 2 Jun 2023 17:09:38 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH 1/3] PCI: endpoint: support an alignment aware
 map/unmaping
Content-Language: en-US
To:     Damien Le Moal <dlemoal@kernel.org>,
        Kishon Vijay Abraham I <kvijayab@amd.com>,
        Shunsuke Mie <mie@igel.co.jp>,
        Jingoo Han <jingoohan1@gmail.com>
Cc:     Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Kishon Vijay Abraham I <kishon@kernel.org>,
        Kunihiko Hayashi <hayashi.kunihiko@socionext.com>,
        Hou Zhiqiang <Zhiqiang.Hou@nxp.com>,
        Frank Li <Frank.Li@nxp.com>, Li Chen <lchen@ambarella.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230113090350.1103494-1-mie@igel.co.jp>
 <20230113090350.1103494-2-mie@igel.co.jp>
 <e417f2c9-1fcb-cf57-3524-1408c9aae5fa@amd.com>
 <978b63ac-90b5-b909-d259-0668b77f1cc8@kernel.org>
From:   Kishon Vijay Abraham I <kvijayab@amd.com>
In-Reply-To: <978b63ac-90b5-b909-d259-0668b77f1cc8@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?eDV1Q1RTTlhsTmQzMmVxUnd1WDEvejdJV04yd2dkZ1hNL1hES0w4K0xCNXhR?=
 =?utf-8?B?RGhSQzJJWVpsdSs1YXJyT3Y3VjBlU0Z2ajlOSWxnYzFVTXY2V2s0V2ROemFn?=
 =?utf-8?B?SXovRHNPaDNMUHFJbW1Najk4L2ZQNzg3c3BHZ2p1S3lvMjk3cnk0L2tldlhD?=
 =?utf-8?B?V1JFdENqays1aUNIVDF0RHJJS3NRQWpPNTNsakhsV2J3VVFQSWM4NGtXd0x4?=
 =?utf-8?B?TE5XSkZmVXcrZzVMd3ZLR002ekE5QmpvY0tzZ0E4V2tFOFlTb3RFZEpiSGJE?=
 =?utf-8?B?dmRoUFRFSktLODFCK1V2cGlDMzBCdDZpdCs3TXU4SElWdkFLMXlkSUNDbWZo?=
 =?utf-8?B?SnpraW5tdHRBdHJBZ3FYcS9MRUgrZ20yS0xQVEFsc3Zac0cveitBc24vblBJ?=
 =?utf-8?B?WGJNUFhLTXpCcWRMMVNQdUJrODBncDVxK2M5TVd0dlFab1lWbEpDRlM1WTZ4?=
 =?utf-8?B?anNFRHdXQ2xvZm84c0pJbGJuSVlDMWxyZmJ4ajFhTkFhajNteU9lanliNkUz?=
 =?utf-8?B?WGFLUE5VZ3c3ZXE0U0VWamJYa0VrMnUxb2d3SVF0OU1oRUNQaHNpSDF2WDJD?=
 =?utf-8?B?SFBNUjR2alBFT1VsQllSbmlDWldiNjlaNEVMT3RCekx1MHZnYnNPZVl1citp?=
 =?utf-8?B?NzF5S0ZyajBvUThpSUw3SWtXQnMyU2oyWmNudzhXUG91bDFNSVVITU53T1hv?=
 =?utf-8?B?Zk1zYndFY2NUcTM0Rno4MitlVThBYm03VnQ4UmtlWnVoSGFXUGFMMGE5bTds?=
 =?utf-8?B?aUhQWm1qVm56UmxkYjVvYVdYeFVPcWN5QTRxejRGeWtMMDRVems3cmpoV09z?=
 =?utf-8?B?UE96dEFSanJwTi81RjdaSUh6SHdhN3JHRmZRaXdSUU9DY3BOSzZnVEdiL21H?=
 =?utf-8?B?L3dlRzBrMXZIRXVsQ1NYUGJic0dodk12L2w0L3JtcHQvSDBpRXBZT1BIT2FG?=
 =?utf-8?B?TXZjajR1U29ydkUrMTRLdWpRbjVJRkNZSDVBUmRTUUdOLzExYlh6TldRUVV1?=
 =?utf-8?B?M1FPV0VtdUdVdE1aSStxdkx0VzdDejVlZFRHSFdsM283S3o4WVpBYmNRL1Nm?=
 =?utf-8?B?T01aRW1Ba2N1dkNIRDljTWljaXV1c2thcU5ON0hON0hnV3ZqSHJrZlAzUXM4?=
 =?utf-8?B?ak5rM3YvdHJlUWxEemxtbjdQZkhFQS9vVXBhMCthRVoxMXJUdFdjeFNuS3dH?=
 =?utf-8?B?bENFbXlRR3M3b2hjQ2NwMjhyRWtpVHZ1UGhHNXh4dXFDKy9UVUdpZkdjNFJh?=
 =?utf-8?B?U3BzakRuSU4xdER6RXl6SWJBMVpuTE5pYytUTGJuTDNCUUdCajVwbk9FSmg0?=
 =?utf-8?B?ckdHMXcxTlp3dDFvK0JZOC9qY3RHVGFibGhRSWpwT2VLYWtQbnpOVFBaa0Js?=
 =?utf-8?B?OWN1bm5meG9aOHAxTnZwbTloendzWUhESGp4YnpMKytweDZMUzRLTWt4Ymxn?=
 =?utf-8?B?b2tBa3dIT1hJVVJPZ3UxNDdYSW1YT09rL25FdDV5S011ZjQ3MXVQckdxRTY4?=
 =?utf-8?B?Tmk5ZW1lVUJSSFFEdEFFUk4zdnNUU1dtcllzNjhBMDJTK3dnUnY0dWdLamNO?=
 =?utf-8?B?NVdiUVlYa0wyL3pLaHAvMHZBcXJiQ3N5a0xMeVJMRXVMZ3dsZUw4MVFIcTJH?=
 =?utf-8?B?WFZvOWl0OFhWZEZKMHJUWi9xNEtFOXYwU0l2RUk2K0J4WjFuK0pQeDQ4V2ZW?=
 =?utf-8?B?RnNQbVpSRGZGOWpORFZ6WXBQZmNWV0ZTNDN6TXdmNWRzdDVoUGFsZEV6Qzgw?=
 =?utf-8?B?YUZZSTVZaVB1c01NQmVjemRxQi8rbHhqdjJKRHV6eW9Lb0N1SEhiVC9yUEN4?=
 =?utf-8?B?dFpwYjE2bVNCenVxWXZFNWw1OGVTdk9vQW1hWm9ZdkViMTJtVkVaYU5XYXFX?=
 =?utf-8?B?ZzhNQ1Y3MlZqZXVzQ21KcGlqdTlVbk82QitZOVV2Ry9EVTU4N21kR28yRW9q?=
 =?utf-8?B?UkF2TlRCU0hKSTdTSnl4TnQvSUFPd1pzYXIzWDJjYndUYU4zSy8xNk1kVDky?=
 =?utf-8?B?Q2ZkNHBlaWw3Y201L1F2UGJ6emp5N1ZHcUl1RE1IdzI3T0lPZkw1ZUU4cVNj?=
 =?utf-8?B?cmUrV0UrU0pDQmlhcGtiMklRNTVuQUd4TW5mZldsNmdWTW0wNFpWZ2ZXTGs3?=
 =?utf-8?Q?gCpQ1YF5WpNAfeP16P7tz0wzt?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a02afed9-abd7-46e6-c5a4-08db635e1748
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5839.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Jun 2023 11:39:57.7727
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: J0L0oNlP/+bMEknqC8qG/q32sFhMIGUQyI4e+dg/nRDYGUipEmkM9SJk7OCw3AFQmB4XpQrfLpyiF3XYQXVSLQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CYYPR12MB8924
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/2/2023 5:13 AM, 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.

Function drivers already work with various restrictions of EP controller 
hardware. pci_epc_features was added to provide such restrictions to 
function drivers. Not sure why it has to be different here.
> 
>> 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.

You mean restrictions w.r.t OB window address and not PCIe address?
> 
> 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

Every EP controller can provide it's restrictions in pci_epc_features. 
Unless the alignment is going to change dynamically, don't see a need 
for adding new epc ops.

Not sure why the following cannot be handled from function driver?

From

        A                    A + S
         ┌────────────────────────┐
         │                        │
         │        OB WIN          │
         ├────────────────────────┤
mapping │                        │
         ▼                  B + S ▼
       B ┌────────────────────────┐
         │                        │
         │       PCI Address      │
         └────────────────────────┘

To


      A   A'│              A + S      A+S+alignment
       ┌────┼───────────────────┬──────┐
       │    │                   │      │
       │    │       OB WIN      │      │
       ├────┴───────────────────┴──────┤
       │                               |
       │                               |
    B' ▼   B                     B + S ▼
       ┌────┬──────────────────────────┐
       │    │                          │
       │    │     PCI Address          │
       └────┴──────────────────────────┘

So the changes in function driver will be
1) Get alignment value in epc_features
2) pci_epc_mem_alloc_addr()/pci_epc_map_addr() will take into account 
the alignment value (change in size parameter)
3) Access host memory from an offset in the provided 
pci_epc_mem_alloc_addr().

> 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

pci_epc_map() was added only to perform mapping functionality. I'd 
prefer it stays that way instead of adding bunch of other things into it.

Thanks,
Kishon

> 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.
> 
> 
>>
>> Thanks,
>> Kishon
>>
>>>
>>> Signed-off-by: Shunsuke Mie <mie@igel.co.jp>
>>> ---
>>>    drivers/pci/endpoint/pci-epc-core.c | 57 ++++++++++++++++++++++++-----
>>>    include/linux/pci-epc.h             | 10 +++--
>>>    2 files changed, 53 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/pci/endpoint/pci-epc-core.c b/drivers/pci/endpoint/pci-epc-core.c
>>> index 2542196e8c3d..60d586e05e7d 100644
>>> --- a/drivers/pci/endpoint/pci-epc-core.c
>>> +++ b/drivers/pci/endpoint/pci-epc-core.c
>>> @@ -430,8 +430,12 @@ EXPORT_SYMBOL_GPL(pci_epc_set_msix);
>>>     * Invoke to unmap the CPU address from PCI address.
>>>     */
>>>    void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>> -			phys_addr_t phys_addr)
>>> +			phys_addr_t phys_addr, void __iomem *virt_addr, size_t size)
>>>    {
>>> +	u64 aligned_phys;
>>> +	void __iomem *aligned_virt;
>>> +	size_t offset;
>>> +
>>>    	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
>>>    		return;
>>>    
>>> @@ -441,9 +445,22 @@ void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>    	if (!epc->ops->unmap_addr)
>>>    		return;
>>>    
>>> +	if (epc->ops->align_mem) {
>>> +		mutex_lock(&epc->lock);
>>> +		aligned_phys = epc->ops->align_mem(epc, phys_addr, &size);
>>> +		mutex_unlock(&epc->lock);
>>> +	} else {
>>> +		aligned_phys = phys_addr;
>>> +	}
>>> +
>>> +	offset = phys_addr - aligned_phys;
>>> +	aligned_virt = virt_addr - offset;
>>> +
>>>    	mutex_lock(&epc->lock);
>>> -	epc->ops->unmap_addr(epc, func_no, vfunc_no, phys_addr);
>>> +	epc->ops->unmap_addr(epc, func_no, vfunc_no, aligned_phys);
>>>    	mutex_unlock(&epc->lock);
>>> +
>>> +	pci_epc_mem_free_addr(epc, aligned_phys, aligned_virt, size);
>>>    }
>>>    EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>>>    
>>> @@ -458,26 +475,46 @@ EXPORT_SYMBOL_GPL(pci_epc_unmap_addr);
>>>     *
>>>     * Invoke to map CPU address with PCI address.
>>>     */
>>> -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>> -		     phys_addr_t phys_addr, u64 pci_addr, size_t size)
>>> +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>> +		u64 pci_addr, phys_addr_t *phys_addr, size_t size)
>>>    {
>>>    	int ret;
>>> +	u64 aligned_addr;
>>> +	size_t offset;
>>> +	void __iomem *virt_addr;
>>>    
>>>    	if (IS_ERR_OR_NULL(epc) || func_no >= epc->max_functions)
>>> -		return -EINVAL;
>>> +		return ERR_PTR(-EINVAL);
>>>    
>>>    	if (vfunc_no > 0 && (!epc->max_vfs || vfunc_no > epc->max_vfs[func_no]))
>>> -		return -EINVAL;
>>> +		return ERR_PTR(-EINVAL);
>>>    
>>>    	if (!epc->ops->map_addr)
>>> -		return 0;
>>> +		return ERR_PTR(-ENOPTSUPP);
>>> +
>>> +	if (epc->ops->align_mem) {
>>> +		mutex_lock(&epc->lock);
>>> +		aligned_addr = epc->ops->align_mem(epc, pci_addr, &size);
>>> +		mutex_unlock(&epc->lock);
>>> +	} else {
>>> +		aligned_addr = pci_addr;
>>> +	}
>>> +
>>> +	offset = pci_addr - aligned_addr;
>>> +
>>> +	virt_addr = pci_epc_mem_alloc_addr(epc, phys_addr, size);
>>> +	if (!virt_addr)
>>> +		return ERR_PTR(-ENOMEM);
>>>    
>>>    	mutex_lock(&epc->lock);
>>> -	ret = epc->ops->map_addr(epc, func_no, vfunc_no, phys_addr, pci_addr,
>>> -				 size);
>>> +	ret = epc->ops->map_addr(epc, func_no, vfunc_no, *phys_addr, aligned_addr, size);
>>>    	mutex_unlock(&epc->lock);
>>> +	if (ret)
>>> +		return ERR_PTR(ret);
>>>    
>>> -	return ret;
>>> +	*phys_addr += offset;
>>> +
>>> +	return virt_addr + offset;
>>>    }
>>>    EXPORT_SYMBOL_GPL(pci_epc_map_addr);
>>>    
>>> diff --git a/include/linux/pci-epc.h b/include/linux/pci-epc.h
>>> index a48778e1a4ee..8f29161bce80 100644
>>> --- a/include/linux/pci-epc.h
>>> +++ b/include/linux/pci-epc.h
>>> @@ -84,6 +84,7 @@ struct pci_epc_ops {
>>>    			       phys_addr_t phys_addr, u8 interrupt_num,
>>>    			       u32 entry_size, u32 *msi_data,
>>>    			       u32 *msi_addr_offset);
>>> +	u64	(*align_mem)(struct pci_epc *epc, u64 addr, size_t *size);
>>>    	int	(*start)(struct pci_epc *epc);
>>>    	void	(*stop)(struct pci_epc *epc);
>>>    	const struct pci_epc_features* (*get_features)(struct pci_epc *epc,
>>> @@ -218,11 +219,12 @@ int pci_epc_set_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>    		    struct pci_epf_bar *epf_bar);
>>>    void pci_epc_clear_bar(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>    		       struct pci_epf_bar *epf_bar);
>>> -int pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>> -		     phys_addr_t phys_addr,
>>> -		     u64 pci_addr, size_t size);
>>> +void __iomem *pci_epc_map_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>> +			       u64 pci_addr, phys_addr_t *phys_addr,
>>> +			       size_t size);
>>>    void pci_epc_unmap_addr(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>> -			phys_addr_t phys_addr);
>>> +			phys_addr_t phys_addr, void __iomem *virt_addr,
>>> +			size_t size);
>>>    int pci_epc_set_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no,
>>>    		    u8 interrupts);
>>>    int pci_epc_get_msi(struct pci_epc *epc, u8 func_no, u8 vfunc_no);
>