Received-SPF: pass (google.com: domain of linux-kernel+bounces-149504-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6984ee8c-7848-46f8-a421-1b31d8c2231a@amd.com>
Date: Thu, 18 Apr 2024 10:14:57 +0530
User-Agent: Mozilla Thunderbird Beta
Subject: Re: [PATCH] PCI: endpoint: Migrate to Genalloc framework for outbound
 window memory allocation
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: Jingoo Han <jingoohan1@gmail.com>,
 Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
 Lorenzo Pieralisi <lpieralisi@kernel.org>,
 =?UTF-8?Q?Krzysztof_Wilczy=C5=84ski?= <kw@linux.com>,
 Rob Herring <robh@kernel.org>, Bjorn Helgaas <bhelgaas@google.com>,
 Kishon Vijay Abraham I <kishon@kernel.org>, linux-pci@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <20240317-pci-ep-genalloc-v1-1-70fe52a3b9be@linaro.org>
 <37cfa724-f9ed-41ef-bad4-f00246a4ee8a@amd.com>
 <20240320112928.GB2525@thinkpad> <20240414130058.GA2231@thinkpad>
Content-Language: en-US
From: Kishon Vijay Abraham I <kvijayab@amd.com>
In-Reply-To: <20240414130058.GA2231@thinkpad>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?U1pPT051RkZ6SFMxZS8vaHpzajdJWlpOYUVIVVc3Kzl6QmhwRytWSmFFYTh1?=
 =?utf-8?B?aWZyY2FMbFVoY0ZvTCtlNldwQTY5TFlBS09yK1o1VDNLb01KRG5hSVJ0ZVZr?=
 =?utf-8?B?MXcwbHNURUY4UVBKc05QQ1BjVGN5Uk44VFNhUnhoU1g0SGZVTDBmb3dveUZT?=
 =?utf-8?B?QjJ0N1JqbzhHOG0zQ3lmZlN3ZHhrNVhwcHB4SWpQczhvdi9pRXNrQXpTY05M?=
 =?utf-8?B?LzJ2cUgwVnVJWUJvWWNtUEljaGdCdWlzcHcvbko4V2g5WVkwa3M3TzBJL0Fr?=
 =?utf-8?B?K0plZjUwWjJXSTYvdWR2RDR3ZFJKZnZnWGNmTXA0eVh0dWNjUlRVeUpBQktk?=
 =?utf-8?B?eW9JVFgwODAxU1AwUGNaWWNzSXh6U1lVWExqeU1sSTY1TU1RYUNKZHQ4YWEv?=
 =?utf-8?B?WHJTcGc2NklkSWZMdjUxT2RxdDgxWlBFa3FPWmtLVDVrMWJlTXh6TkZnTHFt?=
 =?utf-8?B?RHU3aUVNWmtsejJUWGtSTU9yQjI5RFRlK3FMZEV0NnZZTmtLa3dRL2VMVCtz?=
 =?utf-8?B?Y0RGVjJvWmZPWUQ2NEVCdTVTdE1pYVlFY0N4TDRsVlEyWTZGRkJvNi9aV1c4?=
 =?utf-8?B?TVF3ZG43ZCtQRk1OMU54SEwxNlFxRURBOFV4SkZsZ0JGdllzMUFQUGtTMHVP?=
 =?utf-8?B?TEhobGJGaEN6eXFuY0dDZXViN2svRzJPVjZGZ1JMQTRBWTI2OE1STE5LMlEr?=
 =?utf-8?B?K2Vsd2F4N3g5UDRCYXhRa29mMk5ncE16QVJ6MXhNZzEzTHZ6aWhKRWttbWpm?=
 =?utf-8?B?K2RmbERaOGtSTXVkbVoxcEYzbzlRakI2b04vd3p3bU9XZ0dEMVYzSUt0azNO?=
 =?utf-8?B?bzJ4dFBMK2w3dVI2ekRpOWVtRTNnS3o0eGZERFhPeHptai9CUTNsL0MwQXhT?=
 =?utf-8?B?VHNIclQ0dkhnWUJ2QmR6QTF4K2ZxNXNvT3cyQU9wN3hLYUxqbkROSnJmQjZt?=
 =?utf-8?B?VERsb1VtRzk3VHdHQW9mbWg0bGdMUkFBcjMydjc2NmFSZE9rWlE3Wmcycmgv?=
 =?utf-8?B?SGRnWVE5d2hKS1BKZHR4d21rS0luS3ErYWE0cW5OQk8yb1Y5OXltQ2J2NmhZ?=
 =?utf-8?B?WDhQZm5iUWt4MzFXUFp5aUJuNTMreE51YnpIbHplWm1ZSVB3Z3cxZEtzV1Ri?=
 =?utf-8?B?KzJCN0tZbzVNUnhmM1UyVDlFN1dnQmdWbmZYVkp3NWZYNFJvb1h5TjJsN3V4?=
 =?utf-8?B?S0ZiczliSnlyOVM1dGlCYm9uNlYrV2hIbUh1T3p6ZnpDdnY3ZENvdDljdjN3?=
 =?utf-8?B?aXdmMjBkOVorckQ4c0drZUNFV2ZSMi92S3c0ZUZpQWN5WGxVeVdJODE2aE1l?=
 =?utf-8?B?YVpRQTdRMmVpN0pDZ09rdTZUa0U2R2Jzd1NBWitHQ1NTSnFqWVBxbDBLMHNW?=
 =?utf-8?B?Qk9IWTBEcTR2UXA3M05TUDk2WkQxMmpTcWYyVzVBMFg2cTFGR05qK3N4dVEy?=
 =?utf-8?B?dEtsUEovSTNuM0RVNjVxeWo3T3FhbXhxRTdqNjgvWlpoT3dweDRKeU95Q2p6?=
 =?utf-8?B?QTNLazdCTHpsbWhrUGRtQzVUdE9iL2JSK29rMUdvOWszYzJId2h6c2xVc2FW?=
 =?utf-8?B?N2NYVVNhbGhNRHVqSTliSXBYT0JDdzVld1dtSVc2YkVvNG5PRWxKMjlPUWRN?=
 =?utf-8?B?S2lEQWF1d2tiZEtHMlltYnZJWitmWFYyaTh0UVNRUjhFbUVEMjUrYUM4SWla?=
 =?utf-8?B?Nm9kQkx4M2VRVFp0S2EwR2lUbUZmUzlxUGo1OTl2SUVmcVVuSGEvbGJucUdM?=
 =?utf-8?B?YzQ4Tk9YR213amtwaE00UUdZakZIRk5Cd2VrTWpiYlpFc3dXMmtBajZYNlRh?=
 =?utf-8?B?MXpQcXFERlVyajJDRmJvZlNPc1NRejJJN0xQdVR5N0U2dFBHd3gwL2FZSkh6?=
 =?utf-8?B?dE9tejhGeGhnd3JzdGR2Q0JmQnR6MnNrVndqRTlNYXNvU2d0QmdQa0V1Q25R?=
 =?utf-8?B?ZzhYMlFhN3gvU3VoN0Vua2h5TnMySmtSNUtDeExrYmo5TG1BczNXUHc4VTY4?=
 =?utf-8?B?UjIvZXhXaUVDNExjT0ZScGE5WlNoeDUwZ2lFa1pxRlBrQ0xHcktsKzNEVDMv?=
 =?utf-8?B?R1BTZjBoSEhKemtNa0g1UStrZkRmaUhVLzl0VHRsUkZzdHcyMWZBQTM3OEs3?=
 =?utf-8?Q?fvCWxaNOkm3FabUIN7vZD+LeI?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 69b79964-fb84-4fb6-ede0-08dc5f625201
X-MS-Exchange-CrossTenant-AuthSource: DS7PR12MB5839.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2024 04:45:07.0945
 (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: rXCvpxikfBaJ6Zx31DeXN7ILrLO9f0BtaK+w68eKp6eJfdQVDFeXq93+7ME1ZZlqXbjpm/cJge4MiDz1blC9JA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY8PR12MB7145

Hi Mani,

On 4/14/2024 6:30 PM, Manivannan Sadhasivam wrote:
> On Wed, Mar 20, 2024 at 04:59:28PM +0530, Manivannan Sadhasivam wrote:
>> On Wed, Mar 20, 2024 at 03:26:41PM +0530, Kishon Vijay Abraham I wrote:
>>> Hi Mani,
>>>
>>> On 3/17/2024 11:39 AM, Manivannan Sadhasivam wrote:
>>>> As proposed during the last year 'PCI Endpoint Subsystem Open Items
>>>> Discussion' of Linux Plumbers conference [1], let's migrate to Genalloc
>>>> framework for managing the endpoint outbound window memory allocation.
>>>>
>>>> PCI Endpoint subsystem is using a custom memory allocator in pci-epc-mem
>>>> driver from the start for managing the memory required to map the host
>>>> address space (outbound) in endpoint. Even though it works well, it
>>>> completely defeats the purpose of the 'Genalloc framework', a general
>>>> purpose memory allocator framework created to avoid various custom memory
>>>> allocators in the kernel.
>>>>
>>>> The migration to Genalloc framework is done is such a way that the existing
>>>> API semantics are preserved. So that the callers of the EPC mem APIs do not
>>>> need any modification (apart from the pcie-designware-epc driver that
>>>> queries page size).
>>>>
>>>> Internally, the EPC mem driver now uses Genalloc framework's
>>>> 'gen_pool_first_fit_order_align' algorithm that aligns the allocated memory
>>>> based on the requested size as like the previous allocator. And the
>>>> page size passed during pci_epc_mem_init() API is used as the minimum order
>>>> for the memory allocations.
>>>>
>>>> During the migration, 'struct pci_epc_mem' is removed as it is seems
>>>> redundant and the existing 'struct pci_epc_mem_window' in 'struct pci_epc'
>>>> is now used to hold the address windows of the endpoint controller.
>>>>
>>>> [1] https://lpc.events/event/17/contributions/1419/
>>>
>>> Thank you for working on this!
>>>>
>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>> ---
>>>>    drivers/pci/controller/dwc/pcie-designware-ep.c |  14 +-
>>>>    drivers/pci/endpoint/pci-epc-mem.c              | 182 +++++++++---------------
>>>>    include/linux/pci-epc.h                         |  25 +---
>>>>    3 files changed, 81 insertions(+), 140 deletions(-)
>>>>
>>> .
>>> .
>>>> diff --git a/drivers/pci/endpoint/pci-epc-mem.c b/drivers/pci/endpoint/pci-epc-mem.c
>>>> index a9c028f58da1..f9e6e1a6aeaa 100644
>>>> --- a/drivers/pci/endpoint/pci-epc-mem.c
>>>> +++ b/drivers/pci/endpoint/pci-epc-mem.c
>>>> @@ -4,37 +4,18 @@
>>>>     *
>>>>     * Copyright (C) 2017 Texas Instruments
>>>>     * Author: Kishon Vijay Abraham I <kishon@ti.com>
>>>> + *
>>>> + * Copyright (C) 2024 Linaro Ltd.
>>>> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>     */
>>>> +#include <linux/genalloc.h>
>>>>    #include <linux/io.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/slab.h>
>>>>    #include <linux/pci-epc.h>
>>>> -/**
>>>> - * pci_epc_mem_get_order() - determine the allocation order of a memory size
>>>> - * @mem: address space of the endpoint controller
>>>> - * @size: the size for which to get the order
>>>> - *
>>>> - * Reimplement get_order() for mem->page_size since the generic get_order
>>>> - * always gets order with a constant PAGE_SIZE.
>>>> - */
>>>> -static int pci_epc_mem_get_order(struct pci_epc_mem *mem, size_t size)
>>>> -{
>>>> -	int order;
>>>> -	unsigned int page_shift = ilog2(mem->window.page_size);
>>>> -
>>>> -	size--;
>>>> -	size >>= page_shift;
>>>> -#if BITS_PER_LONG == 32
>>>> -	order = fls(size);
>>>> -#else
>>>> -	order = fls64(size);
>>>> -#endif
>>>> -	return order;
>>>> -}
>>>> -
>>>>    /**
>>>>     * pci_epc_multi_mem_init() - initialize the pci_epc_mem structure
>>>>     * @epc: the EPC device that invoked pci_epc_mem_init
>>>> @@ -48,17 +29,11 @@ int pci_epc_multi_mem_init(struct pci_epc *epc,
>>>>    			   struct pci_epc_mem_window *windows,
>>>>    			   unsigned int num_windows)
>>>>    {
>>>> -	struct pci_epc_mem *mem = NULL;
>>>> -	unsigned long *bitmap = NULL;
>>>> -	unsigned int page_shift;
>>>> +	struct pci_epc_mem_window *window = NULL;
>>>>    	size_t page_size;
>>>> -	int bitmap_size;
>>>> -	int pages;
>>>>    	int ret;
>>>>    	int i;
>>>> -	epc->num_windows = 0;
>>>> -
>>>>    	if (!windows || !num_windows)
>>>>    		return -EINVAL;
>>>> @@ -70,45 +45,51 @@ int pci_epc_multi_mem_init(struct pci_epc *epc,
>>>>    		page_size = windows[i].page_size;
>>>>    		if (page_size < PAGE_SIZE)
>>>>    			page_size = PAGE_SIZE;
>>>> -		page_shift = ilog2(page_size);
>>>> -		pages = windows[i].size >> page_shift;
>>>> -		bitmap_size = BITS_TO_LONGS(pages) * sizeof(long);
>>>> -		mem = kzalloc(sizeof(*mem), GFP_KERNEL);
>>>> -		if (!mem) {
>>>> +		windows[i].pool = gen_pool_create(ilog2(page_size), -1);
>>>> +		if (!windows[i].pool) {
>>>>    			ret = -ENOMEM;
>>>> -			i--;
>>>> -			goto err_mem;
>>>> +			goto err_free_mem;
>>>> +		}
>>>> +
>>>> +		gen_pool_set_algo(windows[i].pool, gen_pool_first_fit_order_align,
>>>> +				  NULL);
>>>> +
>>>> +		windows[i].virt_base = ioremap(windows[i].phys_base, windows[i].size);
>>>
>>> Do you have to ioremap upfront the entire window? This could be a problem in
>>> 32-bit systems which has limited vmalloc space. I have faced issues before
>>> when trying to map the entire memory window and had to manipulate vmalloc
>>> boot parameter.
>>>
>>> I'd prefer we find a way to do ioremap per allocation as before.
>>>
>>
>> Hmm, thanks for pointing it out. Current genalloc implementation works on the
>> virtual address as opposed to physical address (that might be due to majority of
>> its users managing the virtual address only). That's the reason I did ioremap of
>> the entire window upfront.
>>
>> Let me see if we can somehow avoid this.
>>
> 
> Looks like we have to introduce some good amount of change to support dynamic
> ioremap with genalloc. But IMO it doesn't worth the effort to introduce these
> changes for some old systems which are supposed to go away soon.
> 
> So I think we can keep the old and genalloc based allocators and use the old one
> only for 32bit systems and genalloc allocator for the rest.
> 
> What do you think?

sure, that should be okay. But can we check with genalloc maintainers to 
see what they think?

Thanks,
Kishon