Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Brijesh Singh <brijesh.singh@amd.com>
Subject: Re: [PATCH Part2 v5 06/45] x86/sev: Invalid pages from direct map
 when adding it to RMP table
To:     Borislav Petkov <bp@alien8.de>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.org,
        linux-crypto@vger.kernel.org, Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Joerg Roedel <jroedel@suse.de>,
        Tom Lendacky <thomas.lendacky@amd.com>,
        "H. Peter Anvin" <hpa@zytor.com>, Ard Biesheuvel <ardb@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        Sean Christopherson <seanjc@google.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Andy Lutomirski <luto@kernel.org>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        Sergio Lopez <slp@redhat.com>, Peter Gonda <pgonda@google.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>,
        David Rientjes <rientjes@google.com>,
        Dov Murik <dovmurik@linux.ibm.com>,
        Tobin Feldman-Fitzthum <tobin@ibm.com>,
        Michael Roth <michael.roth@amd.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Andi Kleen <ak@linux.intel.com>, tony.luck@intel.com,
        marcorr@google.com, sathyanarayanan.kuppuswamy@linux.intel.com
References: <20210820155918.7518-1-brijesh.singh@amd.com>
 <20210820155918.7518-7-brijesh.singh@amd.com> <YVR5cOQOJxy12DcR@zn.tnic>
Message-ID: <60d6a70d-22ab-9e17-b243-7f5669b4b70d@amd.com>
Date:   Thu, 30 Sep 2021 09:19:52 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.14.0
In-Reply-To: <YVR5cOQOJxy12DcR@zn.tnic>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Content-Language: en-US
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dkRyc2QzWWZDOVo1RUNMZGJ2bnV0NWdCalZabG90TEVKMHJJV25rdXBUNjVQ?=
 =?utf-8?B?NFBNd1NPVW5oYnRwcjEzWVhwVS9iRjFjRlVPdFRtQXV2STdMREJjeVNHeFFm?=
 =?utf-8?B?M2RUUUh6T0NHaWxmMkI3K0x1YlpmMUlSQkE3OG1ZSmhrc3Z5VU5URWczczdi?=
 =?utf-8?B?QTgxQ25aNEJHU3JlNjIrQUdjYktDdHE2b2RjNDlCRWw3eG1ydXdXV0tZenly?=
 =?utf-8?B?TmJJZDlad1pCWk8vM1RUOTlWMlExL1VjeEVQSnQrR0lDbnZUUkpaR2s2dkRp?=
 =?utf-8?B?OXNiZmNGUHh6MlJwbkk4Vk0rVEsrRHlLTVpkay84WGZvaGdxYml0WGVVa3h0?=
 =?utf-8?B?V1ZYemJUbmdUZVQ0MWdOWTd2eGhFRnllQkpDRWFWWGcwdWJoWE5kL3NPSFlo?=
 =?utf-8?B?alRPMmw3aUQrbFlsak00U1dTdm52ZG11M21VSDhveTltUUs4MVBsRlZrVnUr?=
 =?utf-8?B?UUNxYTJkZy9DbkdjNnBMZXJielFSVVBSU0p1VlR5WW5uNVRhSis4Uk43WDZT?=
 =?utf-8?B?TnEvRldnZmVkbktqdnB6UHZpdmNwNE1lTk51RFV1SnpRWEJzd3NwVmVhNHdG?=
 =?utf-8?B?UHJEUXdlQS9obDMzYytzMW5jbXgxN3FWaWx5RWJsSnEwV3FaNEpldHMwVkhv?=
 =?utf-8?B?Ni9GNXRSRFR4UGFoeEZhcmJqejBBTG5GQnpYR2pSZm5vR0Z5OGZqZ05XWEl2?=
 =?utf-8?B?b2Q3ZUcwZTg1QmQxdzNySm9qSHNXVTlSb0xsaTB2WWg3RmtHcWxUM0pndUhl?=
 =?utf-8?B?VWwyNHZBL1M2MSsxOHk5THlIeEcydnU5QXhaTFlaMEp6VTVlWXhGempMTHlj?=
 =?utf-8?B?b0ZZaDFKRjQrTnlQOGtHZXRSVjRoaDhrL1BWQTVHU3ZHZG00QVZYZTJvbmV0?=
 =?utf-8?B?bXRLeExOalJxOU9xTHBkMUJENkxLbWtuNzZOcTA2UHBjWVpZZ1NRK3JJOFE3?=
 =?utf-8?B?VHVneFU0RmZ5UXQ3Ri9vNDIzYmNIM2ZxY3dYbStlTUFoZ0RETlRsU0xFMEtR?=
 =?utf-8?B?L2pvWW9KZU1DV2xOdUFmcUdVWEpNRktJQk13QnovaW16NDY4YkJZRFViWHdu?=
 =?utf-8?B?QUwyRkl0Mytad2hnVkc3Nk5jN3JWVjJsVlBiR3c5Mzg5TVhOaXpTTytLdVNv?=
 =?utf-8?B?NzhNTmEzazUvVTJWKy85V2E4aHJSd1plSmo2b3hLSnQvVVpKbXQ2cHJoeFhT?=
 =?utf-8?B?Ym5ta3BoRkhsL1poQUo0N1p3TWM4YXhTeFZwbmkwSXkvZHZleURjTUlnMUxX?=
 =?utf-8?B?bTNKazdkWkcvcXZQcmVidDJsU0ZqWDh4U255bFpXYXFUc0ZUT3g4MFJmQ3pY?=
 =?utf-8?B?aUt2SkRlZlhjbGpsMzJiQXJ4eG1SRHNVa01YdmU2RUVkaStldnRLNkw2MFZt?=
 =?utf-8?B?YmhQL2hSNll2MjlJNlp4a0ZGeDBuUkhTdWwrV29oKy9ZWFhjRXNsVlJTcHBh?=
 =?utf-8?B?UWVLTzZtY1p5TGVSM3k4S3M3WXJUU0FQMFU0ODI1Nyt1SmxkV0xzYUVMaFJp?=
 =?utf-8?B?djBZeFdhNWFiY09UWm5xMlJIZmdIc1NidkthR0czVWpKQ2ZnNVNPcDFGWTE3?=
 =?utf-8?B?cnRDOU02M054bWtjWEZSendGcUFVUEdWSEF6T1FBemF2SjA2L1UzNFVoQ3pG?=
 =?utf-8?B?MkovQjlFbUxSRnQ5LytDNmJGSW1vdmhzSUdNMU5KWExBMkJtKzZmazNqNURo?=
 =?utf-8?B?V1NPMXlGemZnN1hEeE1BbTdlTENXRFpKOG5RSkZJS0V1Tzk5SDg1TVN1S2o3?=
 =?utf-8?Q?rRWnDxwh1PKSHj86qtDtmwaj6f4gedXoKSCwLsL?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 56b4d23f-46d8-4b25-f7fa-08d9842e244d
X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Sep 2021 16:19:56.5155
 (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: eHopUICMXmiPfy/5Af9ezwWFRuCR45HQ27iJj5hYWLmgP9WXWr1KWPQmTxCi/9ZhySwaPDgjHZji134X9FeQnw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR12MB4557
Precedence: bulk
List-ID: <linux-crypto.vger.kernel.org>
X-Mailing-List: linux-crypto@vger.kernel.org

Hi Boris,


On 9/29/21 9:34 AM, Borislav Petkov wrote:

...


>> Improve the rmp_make_private() to
>> invalid state so that pages cannot be used in the direct-map after its
>> added in the RMP table, and restore to its default valid permission after
>> the pages are removed from the RMP table.
> That sentence needs rewriting into proper english.
>
> The more important thing is, though, this doesn't talk about *why*
> you're doing this: you want to remove pages from the direct map when
> they're in the RMP table because something might modify the page and
> then the RMP check will fail?

I'll work to improve the commit description.


> Also, set_direct_map_invalid_noflush() simply clears the Present and RW
> bit of a pte.
>
> So what's up?

The set_direct_map_invalid_noflush() does two steps

1) Split the host page table (if needed)

2) Clear the present and RW bit from pte

In previous patches I was using the set_memory_4k() to split the direct
map before adding the pages in the RMP table. Based on Sean's review
comment [1], I switched to using set_direct_map_invalid_noflush() so
that page is not just split but also removed from the direct map. The
thought process is if in the future  set_direct_map_default_noflush() is
improved to restore the large mapping then it will all work transparently.

[1] https://lore.kernel.org/lkml/YO9kP1v0TAFXISHD@google.com/#t


>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  arch/x86/kernel/sev.c | 61 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
>> index 8627c49666c9..bad41deb8335 100644
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -2441,10 +2441,42 @@ int psmash(u64 pfn)
>>  }
>>  EXPORT_SYMBOL_GPL(psmash);
>>  
>> +static int restore_direct_map(u64 pfn, int npages)
> restore_pages_in_direct_map()
>
>> +{
>> +	int i, ret = 0;
>> +
>> +	for (i = 0; i < npages; i++) {
>> +		ret = set_direct_map_default_noflush(pfn_to_page(pfn + i));
>> +		if (ret)
>> +			goto cleanup;
>> +	}
> So this is looping over a set of virtually contiguous pages, I presume,
> and if so, you should add a function called
>
> 	set_memory_p_rw()
>
> to arch/x86/mm/pat/set_memory.c which does
>
> 	return change_page_attr_set(&addr, numpages, __pgprot(_PAGE_PRESENT | _PAGE_RW), 0);
>
> so that you can do all pages in one go.

I will look into it.

thanks