Received-SPF: pass (google.com: domain of linux-kernel+bounces-11383-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <0ba8e579-2b6f-4e9f-a38c-097694f14d3c@intel.com>
Date: Tue, 26 Dec 2023 14:09:29 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 01/10] iommu/vt-d: add wrapper functions for page
 allocations
Content-Language: en-US
To: Pasha Tatashin <pasha.tatashin@soleen.com>, David Rientjes
	<rientjes@google.com>
CC: Andrew Morton <akpm@linux-foundation.org>, <alim.akhtar@samsung.com>,
	<alyssa@rosenzweig.io>, <asahi@lists.linux.dev>, <baolu.lu@linux.intel.com>,
	<bhelgaas@google.com>, <cgroups@vger.kernel.org>, <corbet@lwn.net>,
	<david@redhat.com>, <dwmw2@infradead.org>, <hannes@cmpxchg.org>,
	<heiko@sntech.de>, <iommu@lists.linux.dev>, <jernej.skrabec@gmail.com>,
	<jonathanh@nvidia.com>, <joro@8bytes.org>, <krzysztof.kozlowski@linaro.org>,
	<linux-doc@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-mm@kvack.org>,
	<linux-rockchip@lists.infradead.org>, <linux-samsung-soc@vger.kernel.org>,
	<linux-sunxi@lists.linux.dev>, <linux-tegra@vger.kernel.org>,
	<lizefan.x@bytedance.com>, <marcan@marcan.st>, <mhiramat@kernel.org>,
	<m.szyprowski@samsung.com>, <paulmck@kernel.org>, <rdunlap@infradead.org>,
	<robin.murphy@arm.com>, <samuel@sholland.org>,
	<suravee.suthikulpanit@amd.com>, <sven@svenpeter.dev>,
	<thierry.reding@gmail.com>, <tj@kernel.org>, <tomas.mudrunka@gmail.com>,
	<vdumpa@nvidia.com>, <wens@csie.org>, <will@kernel.org>,
	<yu-cheng.yu@intel.com>
References: <20231130201504.2322355-1-pasha.tatashin@soleen.com>
 <20231130201504.2322355-2-pasha.tatashin@soleen.com>
 <776e17af-ae25-16a0-f443-66f3972b00c0@google.com>
 <CA+CK2bA8iJ_w8CSx2Ed=d2cVSujrC0-TpO7U9j+Ow-gfk1nyfQ@mail.gmail.com>
From: "Liu, Jingqi" <jingqi.liu@intel.com>
In-Reply-To: <CA+CK2bA8iJ_w8CSx2Ed=d2cVSujrC0-TpO7U9j+Ow-gfk1nyfQ@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ajBrQm9tK3JYNEh6Zkd2d2p6aVpWWDBMTWdxNHBQN0JGV3VqOE9HZlBldkVV?=
 =?utf-8?B?NnFGNUpBdENRVGFNZ1hqZjlhcVByMDdEektqb3U0SHgzV1RhWUNQYUIwY0xF?=
 =?utf-8?B?cFZZMFplbW90d241eno1WTZRWlo2SHFiQmpYdW94TktOSUNXVWRjYm9tNW8v?=
 =?utf-8?B?Z0V1aTQxUjRpbklkNTBUeEhsMnF2QlhDbmNVdmRBRTBlWVlJRmNsY0Exb2Iz?=
 =?utf-8?B?SGx6TG9YTGw1UTF4Q3hCOGtjVExwT3Zhc3Q3V0ViRFNVeFJkQ2lORUZ2RVZi?=
 =?utf-8?B?SUVTV3Q2MzJ0azZBWWpMWGRqa2hQY0FnTWRYV2gvdk8xaFB4TlVLdklDVm1P?=
 =?utf-8?B?NmVxbjVNeEtzbWVsbFdvRWRZbHFDbWdva2p6aWVXMVp4WE9rcngvUGxTbDJO?=
 =?utf-8?B?UlNHMTlYWE9WYjVCQm5nendzcXhFb1QyREtySmZ0eXptNG9XTWlDamh2TFdq?=
 =?utf-8?B?MW1iajVjSUtaWWdLVllFcXN5Z3NscDh5WDQ3NktyaGFPakpsb0lUY2hBNFNT?=
 =?utf-8?B?SzhacmFsTytiS2JQL25JanA5cExNQVhaVzVMWGs5VGpVNmRHS2pzNXZkQVZs?=
 =?utf-8?B?OElLMDJzWmJiUG85eTlnZXpLdWxBSjNoRkVBSXlJQlZUMVlwOWFpVGJIamJI?=
 =?utf-8?B?UDdvSkNWUWVWTnNXM1RjdEJ5WlNFRFF5KzgxcEwxb2dUcWtRSkVsMDV5SFd2?=
 =?utf-8?B?RXNJZHowSzBMOHVvWWFBcmtiMnRZVEtLTXdyOExGTWFuSVZITEI5NCtkRVhS?=
 =?utf-8?B?OEFQNHNCSitKTG5sN3hlWWF0TjdiQjJ4VzYxVzRwdjZqcGRvT0Y1eG5wS1Vy?=
 =?utf-8?B?OTU5ZVZFN3RVbWJ1a1h2Ymo3TXZBM0xzS1E5VDJFMXFtb2t1YVFadHE1aFZI?=
 =?utf-8?B?b3h4RHJxSjFwYVFXeU9ZTXczdlk3QklqQmlCY3lCcndMVDFCcURtMUpFTTdM?=
 =?utf-8?B?ZWtCWWNETEs5YTE2YWpTajBYNW5xOGpxZXhoQjhsSmVUckx5NzNObGtUeith?=
 =?utf-8?B?c0dyUVlJTXJacVl0NDJtREh4VklrNVQ3R1d6a1VCcmFZKzNWQ05EZE5yaGRn?=
 =?utf-8?B?OVE5MDdEdlRVMzJVQk5JK1dHZi90R0M1cjNWVEt6WmtYcEdGcmgya28wUTdU?=
 =?utf-8?B?eVRGc2lUWktSWHl5MC9kQXlSWldJZlo2eXlNS0o1NXhPbDhTeWd1a2ZDeC8z?=
 =?utf-8?B?dDkyK2NqSFlaVVVSR1FXQW8zRlRsTVlRS1UzWkVUQ2hxUGFXbE5YS0ViWHlC?=
 =?utf-8?B?VitqNnRnbC92dHg5dWpLQWU1SDErL2FzM1BQbHF3LzN6V205VmR4WGNqbjEy?=
 =?utf-8?B?VXZJaytoaElDYXBST0k3cndMT045dElldkJHYnBJaGgwbUFLMWJFNnlDSG1z?=
 =?utf-8?B?RjZjbEh0RjhSL096MEozcUhKYlhYTFJ6NFJsak9lTGpGTm1Od0RidDlYS2I2?=
 =?utf-8?B?RVh3SEpEL0lQRGJ6R21aVmVPVXB3WmxsOE91NTFndG9Ic1JBNjF1b0wyU2Rw?=
 =?utf-8?B?Uy93MGViOGFCVFQvcVMwL3J2cDIvR3E3K00ydWZnQisrdU5uUUNEa1U5NEtD?=
 =?utf-8?B?Z2NxOXlkeHFIamdQOGRENmtwbzRWb0xxZk5kZUFGVEI5L28yVUJ0MVZQbERV?=
 =?utf-8?B?M2ViWjRNQThIYWk3NmJXNnB0V3NwQjF5Rm43ZG1MRkFsVVFGNkJGQ2pYUGJL?=
 =?utf-8?B?S0dhMWdIUFhiNjNOc1FVRUpSMldOZkpUQWZ2ZUNkdE5OQzJEb2lPV29oQ2VK?=
 =?utf-8?B?blovcnI1NXhTL0ZQZ2QvM2VXMkFlR2lIYTgvYWdNa2U5RDl5UmVTSXhXM1hU?=
 =?utf-8?B?SHlvUDFXdG5URUpaUnNPaHlQbE1EbGIyQVJUVUpxT3hYTkd1b3ptVk45eWkw?=
 =?utf-8?B?ZVl5WjBQZ2lkNWI3WHpXL1RadDdsVm9vMmtIaDVFci9VRjdsaWF1MnVtOTRU?=
 =?utf-8?B?cGZhak9zbXlGdytCK0piNmlhenZ1Nk9JNmpSalJhVmVORkxEaXN2TTJqMkE2?=
 =?utf-8?B?UkV6MUxHREloNG9RMzZZU0xRaCt3VnhmQU5pUjJqeVB5TDF1elFGcGYrQ2ZZ?=
 =?utf-8?B?RzkxY0FtMDBXK0twRXdUbk81TlRrbm96Tmd3OHdNdVJiYzhFN0h0ZGFXU0Er?=
 =?utf-8?Q?3y9OqVlNLA5Ur3SmszelvsZuF?=
X-MS-Exchange-CrossTenant-Network-Message-Id: ce551948-3a19-45fd-81e9-08dc05d94db8
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5469.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Dec 2023 06:10:05.3321
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qy7TaEZXs462FSdSWGMKIBTyc88uxEEGAC2WAd+kvyqlkPZ2boaR78CyO+ZFoxEk8WItl0kK7BWkW5fm1d6lTw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5451
X-OriginatorOrg: intel.com


On 12/15/2023 3:16 AM, Pasha Tatashin wrote:
> On Thu, Dec 14, 2023 at 12:58 PM David Rientjes <rientjes@google.com> wrote:
>>
>> On Thu, 30 Nov 2023, Pasha Tatashin wrote:
>>
>>> diff --git a/drivers/iommu/iommu-pages.h b/drivers/iommu/iommu-pages.h
>>> new file mode 100644
>>> index 000000000000..2332f807d514
>>> --- /dev/null
>>> +++ b/drivers/iommu/iommu-pages.h
>>> @@ -0,0 +1,199 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Copyright (c) 2023, Google LLC.
>>> + * Pasha Tatashin <pasha.tatashin@soleen.com>
>>> + */
>>> +
>>> +#ifndef __IOMMU_PAGES_H
>>> +#define __IOMMU_PAGES_H
>>> +
>>> +#include <linux/vmstat.h>
>>> +#include <linux/gfp.h>
>>> +#include <linux/mm.h>
>>> +
>>> +/*
>>> + * All page allocation that are performed in the IOMMU subsystem must use one of
>>> + * the functions below.  This is necessary for the proper accounting as IOMMU
>>> + * state can be rather large, i.e. multiple gigabytes in size.
>>> + */
>>> +
>>> +/**
>>> + * __iommu_alloc_pages_node - allocate a zeroed page of a given order from
>>> + * specific NUMA node.
>>> + * @nid: memory NUMA node id
>>
>> NUMA_NO_NODE if no locality requirements?
> 
> If no locality is required, there is a better interface:
> __iommu_alloc_pages(). That one will also take a look at the calling
> process policies to determine the proper NUMA node when nothing is
> specified. However, when policies should be ignored, and no locality
> required, NUMA_NO_NODE can be passed.
> 
>>
>>> + * @gfp: buddy allocator flags
>>> + * @order: page order
>>> + *
>>> + * returns the head struct page of the allocated page.
>>> + */
>>> +static inline struct page *__iommu_alloc_pages_node(int nid, gfp_t gfp,
>>> +                                                 int order)
>>> +{
>>> +     struct page *pages;
>>
>> s/pages/page/ here and later in this file.
> 
> In this file, where there a page with an "order", I reference it with
> "pages", when no order (i.e. order = 0), I reference it with "page"
> 
> I.e.: __iommu_alloc_page vs. __iommu_alloc_pages
> 
>>
>>> +
>>> +     pages = alloc_pages_node(nid, gfp | __GFP_ZERO, order);
>>> +     if (!pages)
>>
>> unlikely()?
> 
> Will add it.
> 
>>
>>> +             return NULL;
>>> +
>>> +     return pages;
>>> +}
>>> +
>>> +/**
>>> + * __iommu_alloc_pages - allocate a zeroed page of a given order.
>>> + * @gfp: buddy allocator flags
>>> + * @order: page order
>>> + *
>>> + * returns the head struct page of the allocated page.
>>> + */
>>> +static inline struct page *__iommu_alloc_pages(gfp_t gfp, int order)
>>> +{
>>> +     struct page *pages;
>>> +
>>> +     pages = alloc_pages(gfp | __GFP_ZERO, order);
>>> +     if (!pages)
>>> +             return NULL;
>>> +
>>> +     return pages;
>>> +}
>>> +
>>> +/**
>>> + * __iommu_alloc_page_node - allocate a zeroed page at specific NUMA node.
>>> + * @nid: memory NUMA node id
>>> + * @gfp: buddy allocator flags
>>> + *
>>> + * returns the struct page of the allocated page.
>>> + */
>>> +static inline struct page *__iommu_alloc_page_node(int nid, gfp_t gfp)
>>> +{
>>> +     return __iommu_alloc_pages_node(nid, gfp, 0);
>>> +}
>>> +
>>> +/**
>>> + * __iommu_alloc_page - allocate a zeroed page
>>> + * @gfp: buddy allocator flags
>>> + *
>>> + * returns the struct page of the allocated page.
>>> + */
>>> +static inline struct page *__iommu_alloc_page(gfp_t gfp)
>>> +{
>>> +     return __iommu_alloc_pages(gfp, 0);
>>> +}
>>> +
>>> +/**
>>> + * __iommu_free_pages - free page of a given order
>>> + * @pages: head struct page of the page
>>
>> I think "pages" implies more than one page, this is just a (potentially
>> compound) page?
> 
> Yes, more than one page, basically, when order may be > 0.
> 
>>> +/**
>>> + * iommu_free_page - free page
>>> + * @virt: virtual address of the page to be freed.
>>> + */
>>> +static inline void iommu_free_page(void *virt)
>>> +{
>>> +     iommu_free_pages(virt, 0);
>>> +}
>>> +
>>> +/**
>>> + * iommu_free_pages_list - free a list of pages.
>>> + * @pages: the head of the lru list to be freed.
>>
>> Document the locking requirements for this?
> 
> Thank you for the review. I will add info about locking requirements,
> in fact they are very relaxed.
> 
> These pages are added to the list by unmaps or remaps operation in
> Intel IOMMU implementation. These calls assume that whoever is doing
> those operations has exclusive access to the VA range in the page
> table of that operation. The pages in this freelist only belong to the
> former page-tables from the IOVA range for those operations.

These pages maybe be accessed concurrently by thread contexts other than 
the IOMMU driver (such as debugfs).

Thanks,
Jingqi
> 
>>> + */
>>> +static inline void iommu_free_pages_list(struct list_head *pages)
>>> +{
>>> +     while (!list_empty(pages)) {
>>> +             struct page *p = list_entry(pages->prev, struct page, lru);
>>> +
>>> +             list_del(&p->lru);
>>> +             put_page(p);
>>> +     }
>>> +}
>