Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
Message-ID: <f4228262-02f9-3af5-8ef5-be109b5d3d13@amd.com>
Date:   Fri, 6 Oct 2023 14:24:11 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.2.1
Subject: Re: [PATCH 10/13] x86/tdx: Convert shared memory back to private on
 kexec
Content-Language: en-US
To:     "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Kuppuswamy Sathyanarayanan 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        Elena Reshetova <elena.reshetova@intel.com>,
        Jun Nakajima <jun.nakajima@intel.com>,
        Rick Edgecombe <rick.p.edgecombe@intel.com>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        kexec@lists.infradead.org, linux-coco@lists.linux.dev,
        linux-kernel@vger.kernel.org
References: <20231005131402.14611-1-kirill.shutemov@linux.intel.com>
 <20231005131402.14611-11-kirill.shutemov@linux.intel.com>
 <8d0e4e71-0614-618a-0f84-55eeb6d27a6d@amd.com>
 <20231005212828.veeekxqc7rwvrbig@box>
 <e0459b50-7e21-7548-8151-9010ee88b0a6@amd.com>
 <20231005222839.jt2du72xogg3c5ny@box>
From:   "Kalra, Ashish" <ashish.kalra@amd.com>
In-Reply-To: <20231005222839.jt2du72xogg3c5ny@box>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MElUUVVTaGwzN3M1YTJDaXdCQkY1MnJPakdUQ2VncWRrVzV1MFgvNGxqN2xi?=
 =?utf-8?B?WXlud2pqcDBaazg2RHpwdmVpOGp6Y0wrc2JySjN5UERYeTdDc3FMaDgyT1Ba?=
 =?utf-8?B?VzREZkxqNUtoWWZtdHlhbHl2L1ozZHpCWmozWVRGVGwvTnpFajJTaGZsS2h1?=
 =?utf-8?B?RG95dU9Yd0RoRHpiK1BEcng1c2VTQW5SeDYzY0FMTEFqdG5JdUhYcDNVdTNY?=
 =?utf-8?B?YmlOVUx4Y3VnQ0NuSGpzaG5IK0lybWZHOER0OThZN3F4KzZNMnFZa1NoNmJx?=
 =?utf-8?B?MGlkYWVGRDkxTWt3cmJvNlBTaUk2VTNZYXhsVWE3Y3VHL3RlcDNXd2ZINXdz?=
 =?utf-8?B?UWxqR0VsVlI3NG9ZZmdFRlYzSDJxNWpMdjU0T2dJczBubFlrd0tVVEpEUWF2?=
 =?utf-8?B?NXpqRkxITHE5MXVCYS9tM1k2Q0FpNTUzMlpLSG5zd0lzZjBmdnZxekphNlBG?=
 =?utf-8?B?TnpWa1dEbmxpa3pCcnNxSDFKNS9CK2Z0ZXE0NFk2N1lvLzc5N2NTbTNLL3cv?=
 =?utf-8?B?Tk41UjAvN0NmbXBOWWVURE1zL21TYVlSWk0zc2tDZjQ5UkZlbnJ4dmZPMTVC?=
 =?utf-8?B?OGhtTjloY0diU1VSRklLZk01VkFHcnNxajBXVlI1TExGbjB5OGNDdS9BMGs3?=
 =?utf-8?B?Sk41cWRCdGMxSzc5U1BSUU9ydWFqMlJmSkZQNjhBaXByZUQyaTNnRmltcWNt?=
 =?utf-8?B?dDVzdlRucXg3TmIxSnZPa3dKYjFDR0R4N3lCSGZxaVZ1Vmh2dzBRdkdnUWln?=
 =?utf-8?B?VVJNYW5FY0wxUVYxNGVXNW9jenlsbFRDSnNqcVRWamlmMk4yZEdrb1d0enNF?=
 =?utf-8?B?R1kydEtiRnBTRXMydVpTRjRRd0d4U2F6bjRDNXdlWm5mK0tvYWVlbG1jTHcy?=
 =?utf-8?B?b0V3dVdFVkFNYzNOcjhpdEZGVlJTZ2RRSnNWZndNZjZyOFM0cExzSGw1aW5u?=
 =?utf-8?B?ZTl2Mk1yWWRaN01uQ05KRDVCS05yRWVoOFBlVHY5cWJCY0krQStzbm1qZG8r?=
 =?utf-8?B?U01ZQU1VZmplb3hUY05rOHBESjZoWUFvMDlOMGhZcEE3YnUraDg4UCtIcHFX?=
 =?utf-8?B?ZW1DNXZTQVM1eitqSWJqMUNhV0FEYVg4WFVVSURMWFcxbnpPd0Nrc25WQnN1?=
 =?utf-8?B?ZC9aUEswODkyUk9kaWFqOXdNUWVNZWowWXp5b3k4SkxnVU51S25qanJXS0xT?=
 =?utf-8?B?MVh4OGtEeXllcm1vQ09pNVBRWHhVdkFXVTluaklyTEZsRlFCZVJ2TWo0d21Y?=
 =?utf-8?B?aTBtSnpvNDRHeDZuUzh2VnJSY09Qcmt6RngrVmZOVTUrOStXWGZQTWJmWm1M?=
 =?utf-8?B?Skg0Z3VxZHp3M2krL2JYN2JBeE91cWEzNHJZaXZ3RUt5T3VFaEtuTXFwcTdM?=
 =?utf-8?B?Qk8vbkFaUkV4MUJySWVxZjRsK2JRc3Q1M2RJZ2Y3dUxyLzlYU2ZNZytBc2s4?=
 =?utf-8?B?QnhHKzI1enlicHRjRXo0WlhhRDdSZWlWalArNnBqZ1N3NzJzQ3dzS0pZVXpF?=
 =?utf-8?B?Qm02Tm1mOTcwNmZ1Y3Vqbk5oRzNQVzNtckJ2V3loQ1J2K1dsaUVSOGlZSHhx?=
 =?utf-8?B?MGRRZ1ZOVnVLZzJoeDd6eXh4ZDZsaXZHZFJEalFzbmIvaWplbTBrU1JuQ3F2?=
 =?utf-8?B?NWxhOSsvN2lCaTdDQTU2ekh6NUNYVkpKOVE0dWM2Vy9aMU9MQ2FTOGthUHow?=
 =?utf-8?B?VjNNK1pNSzQ2Ykw0bGs1MmlQc1BSQmlzRndaZmxWYkUyOHpKY1phSW1SSEhJ?=
 =?utf-8?B?YVZTN1ZNZ0R2Sk85OTIzY0pHcmlwTzhITzVYc0xUUGR6aDhCd3FUYzlPSlg1?=
 =?utf-8?B?MVlUNXl6Ni9EMUhjL1dkc0tXL1kxa3h6V0srZ0hTakJ6TnZ0NzNIb0hoeSto?=
 =?utf-8?B?RkpISk53QTVFeG4yQU05NDF3UTdPai9SSmJOZE9YcE8rbmt0RkZXR1UvTzFJ?=
 =?utf-8?B?TXRzMDFPbVZ4TjZBbUl2Y2hVN3hwdGxtS29MVFNqbFhRdVdCM0NxSGdhMHdr?=
 =?utf-8?B?V2p2SUhoSGhWSTZmeTR6Q0dKaWx0U0t1M2l0cEs2ZkprNjdyQU9FcGg5UUt2?=
 =?utf-8?B?RE5iZnByeEloTUxRcmFYektHM01PM0NYR1luc2JHTy9USUgrbHI4MFZoTnRi?=
 =?utf-8?Q?jX2YmaUQwbg5k8z3T0eoQEjvb?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c6d3876c-d632-43c1-587d-08dbc6a1d308
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2767.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Oct 2023 19:24:13.7638
 (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: xo23yfCzROj67U9Pdzuujmoj9O+vDAvmHb6b8cgFiIIvDNZ+sD5EJi7rgqEjJFlhChrtscvHGX8QdCLh5sTerA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB8221
X-Spam-Status: No, score=-0.1 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on morse.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (morse.vger.email [0.0.0.0]); Fri, 06 Oct 2023 12:24:48 -0700 (PDT)


On 10/5/2023 5:28 PM, Kirill A. Shutemov wrote:
> On Thu, Oct 05, 2023 at 05:01:23PM -0500, Kalra, Ashish wrote:
>> On 10/5/2023 4:28 PM, Kirill A. Shutemov wrote:
>>> On Thu, Oct 05, 2023 at 01:41:38PM -0500, Kalra, Ashish wrote:
>>>>> +static void unshare_all_memory(bool unmap)
>>>>> +{
>>>>> +	unsigned long addr, end;
>>>>> +	long found = 0, shared;
>>>>> +
>>>>> +	/*
>>>>> +	 * Walk direct mapping and convert all shared memory back to private,
>>>>> +	 */
>>>>> +
>>>>> +	addr = PAGE_OFFSET;
>>>>> +	end  = PAGE_OFFSET + get_max_mapped();
>>>>> +
>>>>> +	while (addr < end) {
>>>>> +		unsigned long size;
>>>>> +		unsigned int level;
>>>>> +		pte_t *pte;
>>>>> +
>>>>> +		pte = lookup_address(addr, &level);
>>>>
>>>> IIRC, you were earlier walking the direct mapping using
>>>> walk_page_range_novma(), any particular reason to use lookup_address()
>>>> instead ?
>>>
>>> walk_page_range_novma() wants mmap lock to be taken, but it is tricky as
>>> we run here from atomic context in case of crash.
>>>
>>> I considered using trylock to bypass the limitation, but it is a hack.
>>>
>>>>
>>>>> +		size = page_level_size(level);
>>>>> +
>>>>> +		if (pte && pte_decrypted(*pte)) {
>>>>
>>>> Additionally need to add check for pte_none() here to handle physical memory
>>>> holes in direct mapping.
>>>
>>> lookup_address() returns NULL for none entries.
>>>
>>
>> Looking at lookup_address_in_pgd(), at pte level it is simply returning
>> pte_offset_kernel() and there does not seem to be a check for returning NULL
>> if pte_none() ?
> 
> Hm. You are right.
> 
> I think it yet another quirk in how lookup_address() implemented. We need
> to make it straight too.
> 
> There's two options: either make lookup_address() return pointer for entry
> even if it is NULL, or add check for pte_none() after pte_offset_kernel()
> and return NULL if it is true.
> 
> I like the first option more as it allows caller to populate the entry if
> it wants.

Yes, i like the first option.

> 
>>>>> +			int pages = size / PAGE_SIZE;
>>>>> +
>>>>> +			/*
>>>>> +			 * Touching memory with shared bit set triggers implicit
>>>>> +			 * conversion to shared.
>>>>> +			 *
>>>>> +			 * Make sure nobody touches the shared range from
>>>>> +			 * now on.
>>>>> +			 *
>>>>> +			 * Bypass unmapping for crash scenario. Unmapping
>>>>> +			 * requires sleepable context, but in crash case kernel
>>>>> +			 * hits the code path with interrupts disabled.
>>>>
>>>> In case of SNP we will need to temporarily enable interrupts during this
>>>> unsharing as we invoke set_memory_encrypted() which then hits a BUG_ON() in
>>>> cpa_flush() if interrupts are disabled.
>>>
>>> Do you really need full set_memory_encrypted()? Can't you do something
>>> ligher?
>>>
>> We need to modify the PTE for setting c-bit to 1 so that will require
>> cpa_flush(), though probably can add something lighter to do
>> clflush_cache_range() directly ?
> 
> For TDX, I don't touch shared bit as nobody suppose to touch the memory
> after the point (ans set_memory_np() enforces it for !crash case).
> 
> Can't SNP do the same?
> 

No, we need to make the PSC call for HV to update the RMP, then set 
C-bit=1 in the PTE and then do a PVALIDATE to switch the page back to 
private, so it needs something like a full set_memory_encrypted().

Thanks,
Ashish