Received-SPF: pass (google.com: domain of linux-crypto+bounces-1314-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: <5cdd2093-b007-404d-96a8-89b3aa6e6e4b@amd.com>
Date: Tue, 9 Jan 2024 16:19:02 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v1 15/26] x86/sev: Introduce snp leaked pages list
Content-Language: en-US
To: Vlastimil Babka <vbabka@suse.cz>, Michael Roth <michael.roth@amd.com>,
 x86@kernel.org
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, linux-mm@kvack.org,
 linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org,
 tglx@linutronix.de, mingo@redhat.com, jroedel@suse.de,
 thomas.lendacky@amd.com, hpa@zytor.com, ardb@kernel.org,
 pbonzini@redhat.com, seanjc@google.com, vkuznets@redhat.com,
 jmattson@google.com, luto@kernel.org, dave.hansen@linux.intel.com,
 slp@redhat.com, pgonda@google.com, peterz@infradead.org,
 srinivas.pandruvada@linux.intel.com, rientjes@google.com, tobin@ibm.com,
 bp@alien8.de, kirill@shutemov.name, ak@linux.intel.com, tony.luck@intel.com,
 sathyanarayanan.kuppuswamy@linux.intel.com, alpergun@google.com,
 jarkko@kernel.org, nikunj.dadhania@amd.com, pankaj.gupta@amd.com,
 liam.merwick@oracle.com, zhi.a.wang@intel.com
References: <20231230161954.569267-1-michael.roth@amd.com>
 <20231230161954.569267-16-michael.roth@amd.com>
 <f221ad9d-6fc3-466b-bacf-23986b8655f5@suse.cz>
From: "Kalra, Ashish" <ashish.kalra@amd.com>
In-Reply-To: <f221ad9d-6fc3-466b-bacf-23986b8655f5@suse.cz>
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?U00vZUFXY05xMUs5aDdhZFZramZTajFmM2FDUGRLNlRLL2ZmYytlVlJ2NlpC?=
 =?utf-8?B?dVhzTHhMS3ZKQ1pZZTQ1YnM1cTc4TkNNZkZjR0ZEWjBGcVBhbTR3NSswVy9l?=
 =?utf-8?B?NnowNW9iS3hKYlRWY2hqZW9Sd2tBZHBnT3Y2aU40TlREUEpxS0dHK0c2MEg0?=
 =?utf-8?B?ZkZ6Vksxb1h4U2tEdW9vVDF0RnJyZjBlZFBXd0dWblF5aG1LRk9HTk5BSU4y?=
 =?utf-8?B?MnpkTXpaWDBKbG4yLytLNGEyc2RCa0ZHaEhLdFRkcTBhOTIvZXhVelhBQ0Zu?=
 =?utf-8?B?ZzBBWjVUS2QyZm8rYVJYQUdib3dsVjhYaVVSL2V5bThaSVl1NnVIeEVqQkJP?=
 =?utf-8?B?azBSbTRGd0lCVVB1QWcwK3lmUUlkcWhKU0RMc1pZTmlHMDdNMy9GWk1jU2JM?=
 =?utf-8?B?ckYySmNEWS9nV0NmcjVCQTVLVnRYUWJLZnZWQTF4L3dyNFUrRzRlVmsxSGoz?=
 =?utf-8?B?VlNDLzgxSDB1TThSMDAvRkJPbXZySWljWG44VGpsOVFwK0Z0eE9OSzlzSVlD?=
 =?utf-8?B?Mk44NXFvK0JaQW9yV3QrVHYrK1BabmsrUzV6SXRydGJic0RtWTNZcnR2TDU3?=
 =?utf-8?B?RlZmWWswTzJqV1VIS3d0S0Y3dDJhb0JSK04yS25JckxVdU01UHkwbUx4clVu?=
 =?utf-8?B?bE81QVpydDIzdkgyT252NHpGN0xuK2xQRFZOU0RBaGZvaFkvY25iN2VEcXc3?=
 =?utf-8?B?MzZ0QzRsYkpsdnFOS3ZVTlMySUZ4MENzdVQzQ05HRVU2KzBCYm1TaC85TWJI?=
 =?utf-8?B?V0orZFcxUnBzRjhGYXBLNmMyN1laRjhWWkJFZEdEeXVTTEo3cUJPcFRUUUhL?=
 =?utf-8?B?M3VsWEVrY05DOUFXOGcyWHdTRmJ6L3dFSkJUNDM2akE3QTkxVDNkZzhHNGdm?=
 =?utf-8?B?RnNnc2JqK3VYTEw2TVA5NUcwaDNtWjU4YU8yTlVldzJ0Zk1mb2lSUERQOUVD?=
 =?utf-8?B?Q0Z0N2JBeHRIRlBWbSt1M0hUejVTYlY3WjRnRVl1SG9ucjNyMzFpTHpWWlJN?=
 =?utf-8?B?UWlpamoyTytWVWxkZHFEMWVwbkpWQlBubHFXTVVtWk42WUZPeitmWmhlRmVl?=
 =?utf-8?B?cSsvNkhNb2lha2MyWDBSZ2RHT3ZidkpLd0x0bTc1T2ViSit4eWhoaksxMmRa?=
 =?utf-8?B?TXpWeldMTy9RaEhUYUR6SlIxZmlyTlVYYlBBblJIaW9tVTJ0SDJUOW4wQU1a?=
 =?utf-8?B?Y2FMUWNDOW9EeEJ1cmp6UlhBdDlXWSszMlRYSDEvY09Id21jM0F0MkpITkJk?=
 =?utf-8?B?OGhBVEk3aXpGY0ZyRllQYkgrY0g1ajgrUW1ueFNDNDMweU9ZMGs0MmllMEU1?=
 =?utf-8?B?TThZdnBKalp1VHZMSjlzSit1a2ZaN2VRNXRVRzlud0xETVJoK2RPcTVpTm1G?=
 =?utf-8?B?VTJNSlg5ejlEN2t2enM4d1NOQ3lIbU9JTmVjQW9WaFcwM2tYbDZqQXJKTWY2?=
 =?utf-8?B?REJ0TkxxLzMzOWpZWUM0QWEzUllQK1ZFVktCSndKVlRjaE45RlNUTnUwRWxK?=
 =?utf-8?B?RHpKRlM2SER3T25NUVR6K000VkczY1g1WTVFY3VYNmtwWVRvMDRxWVFqc0dk?=
 =?utf-8?B?dnFuU3RRQ3lWZHoyTXJlRy9IcGFLL0VzbG5YaWttMHZpUUZwaW0weTQvRVdW?=
 =?utf-8?B?OVhva1RnSXZCK3FMRGdESmVHbllGUVJieHFUNGJvaklKUEJTcHpUZmNtdjRl?=
 =?utf-8?B?OG1lSXBvcXR4blVHVlg5RnNQZTgvMXhFNHhtdXdQVWxlQlVERGVUS2NQNVpI?=
 =?utf-8?B?L3NmZCtVaDVwL2xKdzZJTEh1emxZOUllcS80SE1zSE9XQjROY0ZvWGMzQXNE?=
 =?utf-8?B?N3pRcFVBcTcySWdMdWliYzFYT1QwWUJ3bG1uZWFNaUpzamgzSy9Kak1rWE0y?=
 =?utf-8?B?VWZ0bXBGeHVpdDNJcVJhL05FWGVWMGdveXhsanU0ZXAvWit4ekkzRTk1bVI0?=
 =?utf-8?B?cFJnVTJ2dEEzV0loZWsyaWN1QjlUdzhwcExCMXRZQS95WkNVaUZaL0kwTGpq?=
 =?utf-8?B?S3dLRE12V1R3M1BGV1c1RUFhdW1QMW92NGxvTjdWeDJnbnhiUGEvZjVqK1ZZ?=
 =?utf-8?B?M0F6OXJQdTJpSTNIbWpmMDVwaGl5bEJUZnZyNm9FaWkxZTZvcnljQ2RzMzJj?=
 =?utf-8?Q?bvM2OaOt3OioNN0rx2JcyYcmQ?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7e0c4860-42cb-4455-bedc-08dc1160fe0c
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2024 22:19:05.7949
 (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: rlJmXGWhY5tCxB+ITpClkD1nGrM6lU+uWbxcxs+3cFzJ7sifpwYn74nqzPXJl6Iv82FIb3dQa4l/kzxur22x/A==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8333

Hello Vlastimil,

On 1/8/2024 4:45 AM, Vlastimil Babka wrote:
> On 12/30/23 17:19, Michael Roth wrote:
>> From: Ashish Kalra <ashish.kalra@amd.com>
>>
>> Pages are unsafe to be released back to the page-allocator, if they
>> have been transitioned to firmware/guest state and can't be reclaimed
>> or transitioned back to hypervisor/shared state. In this case add
>> them to an internal leaked pages list to ensure that they are not freed
>> or touched/accessed to cause fatal page faults.
>>
>> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
>> [mdr: relocate to arch/x86/virt/svm/sev.c]
>> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Hi, sorry I didn't respond in time to the last mail discussing previous
> version in
> https://lore.kernel.org/all/8c1fd8da-912a-a9ce-9547-107ba8a450fc@amd.com/
> due to upcoming holidays.
>
> I would rather avoid the approach of allocating container objects:
> - it's allocating memory when effectively losing memory, a dangerous thing
> - are all the callers and their context ok with GFP_KERNEL?
> - GFP_KERNEL_ACCOUNT seems wrong, why would we be charging this to the
> current process, it's probably not its fault the pages are leaked? Also the
> charging can fail?
> - given the benefit of having leaked pages on a list is basically just
> debugging (i.e. crash dump or drgn inspection) this seems too heavy
>
> I think it would be better and sufficient to use page->lru for order-0 and
> head pages, and simply skip tail pages (possibly with adjusted warning
> message for that case).
>
> Vlastimil
>
> <snip

Considering the above thoughts, this is updated version of 
snp_leak_pages(), looking forward to any review comments/feedback you 
have on the same:

void snp_leak_pages(u64 pfn, unsigned int npages)
{
         struct page *page = pfn_to_page(pfn);

         pr_debug("%s: leaking PFN range 0x%llx-0x%llx\n", __func__, 
pfn, pfn + npages);

         spin_lock(&snp_leaked_pages_list_lock);
         while (npages--) {
                 /*
                  * Reuse the page's buddy list for chaining into the leaked
                  * pages list. This page should not be on a free list 
currently
                  * and is also unsafe to be added to a free list.
                  */
                 if ((likely(!PageCompound(page))) || (PageCompound(page) &&
                     !PageTail(page) && compound_head(page) == page))
                         /*
                          * Skip inserting tail pages of compound page as
                          * page->buddy_list of tail pages is not usable.
                          */
                         list_add_tail(&page->buddy_list, 
&snp_leaked_pages_list);
                 sev_dump_rmpentry(pfn);
                 snp_nr_leaked_pages++;
                 pfn++;
                 page++;
         }
         spin_unlock(&snp_leaked_pages_list_lock);
}

Thanks, Ashish