Received-SPF: pass (google.com: domain of linux-kernel+bounces-108722-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <37cfa724-f9ed-41ef-bad4-f00246a4ee8a@amd.com>
Date: Wed, 20 Mar 2024 15:26:41 +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>,
 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>
Cc: linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20240317-pci-ep-genalloc-v1-1-70fe52a3b9be@linaro.org>
Content-Language: en-US
From: Kishon Vijay Abraham I <kvijayab@amd.com>
In-Reply-To: <20240317-pci-ep-genalloc-v1-1-70fe52a3b9be@linaro.org>
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?bzUrUGlxY0kydkhYZ3U5b3NrWit5S2RZc3lVd0VOa1p6Qytqb2hDVldvM3pT?=
 =?utf-8?B?ZjdMcXNEMG5TRG9WOVVsODdsUXRDS2k4ZkFhTHRPNWpXQkZtUDJkaXBpNXRC?=
 =?utf-8?B?WDdSSVNIZEVKaHI3VnRUWU1yRjhIanZIYWx3RWJhSm1HTXdkNnI2MUlseHI3?=
 =?utf-8?B?MUk1WTJUQTVGYnp5SGlBdHVNSTh2NzNCdDVqaDMyQit2OFJUN00ydFEzaTNK?=
 =?utf-8?B?RTZ6cTJkSXRXMyt1M1UzODN6VmdaZXc3TGNIU21xaHl2Q2N5Nm94RE5PRnhq?=
 =?utf-8?B?blZUUlVsZ2gwM0JpRDBmaklEM09LdFdaWlIzQVQvUFUzWWFYM3F1LzZnanRZ?=
 =?utf-8?B?TnBscm4xc2VUci94bzBvU0JUZjVXY0xRNXcxUThQNDhKYjZneGxMaHJzNURW?=
 =?utf-8?B?eTk0eGo3K0hTZjR5bEFvMDVOV2tzamUySitwQ2NSM2FlK2tDR2YybktpYzNh?=
 =?utf-8?B?Wkp6SjNRaHh1NGdjMXk1YnpYbzgwUllZUlJrV1ZnYWJkMXhJVlNCOTJpNkxM?=
 =?utf-8?B?elRyeGNCc3RPOE4xR3lwaFgyczZzRjFYcWUzclJQbXdCUUpUdUJVTU93V0lj?=
 =?utf-8?B?RGxOMXdORkFKRVZqKzVWN0FKMzl3MFlockhvOXlvM2FLdTIwVVRLUENDUVFW?=
 =?utf-8?B?cFFZdDhKUkJpRU5pNVQ0WTQwRXJIVjgrVTRRdm9vOHYwaVhwM2JLbjdJeXNy?=
 =?utf-8?B?MmlTUkNldVlpaWk4WUg2ajBJRU9KS1ZWOWpZTm1yY25nRnA3S1d1NHRtTk5T?=
 =?utf-8?B?c1B2R2l0RmpOamtqQ0ErU1BvSVlFcVBaY3MyVnJIU0h2eC83NUdVOUtWTERs?=
 =?utf-8?B?UjF1T3RmV1YrUmQ1aWluYVp5czMwd1FBc3NnNE8yUmU2ZW5RakY5ZmZ2cWRL?=
 =?utf-8?B?Z1d2UXdEaVk2UlRvY2ZudXJhYzZRTjdtU0c4RGI5SVR1OEZVREFzeElzOGZh?=
 =?utf-8?B?WlZkRHNUQmlsYUgvdFFpT3hURDJZZlRlaThZYVArL2tOVXRGS0JxWUVVRzdm?=
 =?utf-8?B?L1ZXcWIvRmQ2Q3ZvWkphaHVOMXNUOW5JZTNodmJmbUl1QklLRVk1S09GVG9S?=
 =?utf-8?B?cDdlQms2MUJmQmhhOG9kVmZrNk83RjhMMmY5dUk1NlFWampIanYzK1RyL0NO?=
 =?utf-8?B?cDBpcFJmTzNveDlyQTZVcmNJYW1ZNnk2S0RnblVTM0JLOGltV1hXSkUzYTI5?=
 =?utf-8?B?clpQTnVVMWZhZlozMnNIL25raVZOZlRJZDYyaWFTVUtQbWM3TGVLU2NISE9z?=
 =?utf-8?B?dXBKcytDUXJvdDA2UmxBVFFLckNmd0dXZnVDR3JMMHlERXAyaVZTRlIwRTlC?=
 =?utf-8?B?ZCtha3BjZ0ZtQnhWcnRTVk9iNjcvQnd1RlpzdzRaN0dnVjdUUWZwczllNW5B?=
 =?utf-8?B?ekhaOW5vZTh0UmJMV1lHMW5ETnhVOVV0dXhWelVibkhhZVU0TFdad0MxSDBx?=
 =?utf-8?B?NkxDUnVWSEoyUVVkcmxtenhhUTF3blBDTlNiZ2hOTVVoTmhmRFdvb3lldFF4?=
 =?utf-8?B?eGYwcXR0Z1JzNTluenN2c3lRVzNqNkZlRGlJVEp1VmRNa3p4Skl4TEJXT29G?=
 =?utf-8?B?WXdIVVcxcEpsSUNYMXVqYlVieTBaYjJiUHc4bFBoa1c2djJ3TzZlUC9yYndJ?=
 =?utf-8?B?NzdjYldNV3BUd2EvK20wOGdlQTB2UFg1a1NvcThBcWR2M1NGQjBXeGRsdVJq?=
 =?utf-8?B?MWUydEQ4L3c1L2xqYmxyN0xzVGhJY0dFRjJ2eWpwNjhGYW9PczhRb2JPbUZx?=
 =?utf-8?B?UUYwcGh0NDhGait4ejBGZithMUpHQ1BIaGpNajVCbGRaMHI0K044V2pTSmho?=
 =?utf-8?B?NDRQcEJwK3k1MitIc0hESW40dkJhend1ekdzQU1CWGxvR1hLOU4vU0ZtWDFV?=
 =?utf-8?B?QjhpRVY1VzJ4LzlqRnU5dE5zSUJLdjFRUzVZNGJnSWdaSDBHVHlMNXdQWGZ5?=
 =?utf-8?B?L0FORTRSVWVDUmNnN0ZYMHpPcUtOYzNlVytxT3RJUGVOcGIxRU1EN21wRFQ2?=
 =?utf-8?B?aHJVbHl5RDdJUE9BKy9sZDQzUEdzWFBUc0g4NGF3WEZSNkRVblR3aThhVGNM?=
 =?utf-8?B?VkU3OHFxSS9BL0lWQURydWRoaXlQdUpPNDhJTVhZYnFIRWx2bkhiaXJEUjhN?=
 =?utf-8?Q?qIivlKBmFgrJ09Ci4Nr5KGez8?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a23c8377-20a2-488e-7c0b-08dc48c4105d
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5827.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Mar 2024 09:56:50.8321
 (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: 6gKdDsp2fp7qFKFXadna2oYN4dF92nqy7JjGPkmENj13xutTYm+hFfrhwycqrvnQd5auHS4q386SKNv9bl9gKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA0PR12MB8693

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.

Thanks,
Kishon