Cc:     brijesh.singh@amd.com, x86@kernel.org,
        linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        linux-efi@vger.kernel.org, platform-driver-x86@vger.kernel.org,
        linux-coco@lists.linux.dev, linux-mm@kvack.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>,
        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>,
        Borislav Petkov <bp@alien8.de>,
        Michael Roth <michael.roth@amd.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Andi Kleen <ak@linux.intel.com>,
        "Dr . David Alan Gilbert" <dgilbert@redhat.com>,
        tony.luck@intel.com, marcorr@google.com,
        sathyanarayanan.kuppuswamy@linux.intel.com
Subject: Re: [PATCH v8 12/40] x86/sev: Add helper for validating pages in
 early enc attribute changes
To:     Venu Busireddy <venu.busireddy@oracle.com>
References: <20211210154332.11526-1-brijesh.singh@amd.com>
 <20211210154332.11526-13-brijesh.singh@amd.com> <YdOGk5b0vYSpP1Ws@dt>
From:   Brijesh Singh <brijesh.singh@amd.com>
Message-ID: <7934f88f-8e2b-ea45-6110-202ea8c2ad64@amd.com>
Date:   Tue, 11 Jan 2022 15:22:01 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.14.0
In-Reply-To: <YdOGk5b0vYSpP1Ws@dt>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?VE1WTmx5RTVCWm1xR25xdWRraUIyazhLcVRXUGJtWDBCMUdESEdlYzNGTXBR?=
 =?utf-8?B?ZTgxWmoyelQ5L2o1QTBVZkFMTTdiWjFPM3JIcVk0ZkFzZ1R4NVRabVJDaUQr?=
 =?utf-8?B?d0wranZCOC9jUEFtUXRtTkZwWHB6WGQ5eFl6Szd5VTB0em9aZmtTR3lBVWhL?=
 =?utf-8?B?a3RhMGZoYndHeHk2aS9NbGhrUHBWeDlvRnNVbUlNb2VMOHhEQXpIR0U1NTVq?=
 =?utf-8?B?dmRXSGRZRFhuRkMzTmNITWtUZFV3YlJYRzlxOEdCeG5ZY1c3dFcxM1R6K3Yx?=
 =?utf-8?B?bWlPMnVUcyt1WXY1d1V2emZpaEUza1lHQVVPb0FKcmNRS2d5N0JoTW43ZnZy?=
 =?utf-8?B?TzVLMjFjU3JWR1pTZ3J3QzVKTDdzcnVMdXEwQmtFRWowRUZnNVR0YWpFdkht?=
 =?utf-8?B?dk1PajA5SS9tdUtxN0V1M3VRcHc5UUZkSE1LLzJub3BsRExNanJOYTNLeGxE?=
 =?utf-8?B?MmZ5bFJEb0U2Sk5tRk4wQUFSRkppMTIwSlI0VlFzVnUrYzZBTCtxTFlrdWtC?=
 =?utf-8?B?N0ZkdUxDbmVxcGRMME51QnlTWFRiaDU3T0dRMW9UR2FMQnNQRzJ2UUxoTUJs?=
 =?utf-8?B?a3ZDY1l5T3ljSFdLaXpJbHVVbnFlbnhKdlhESGJPYzEzaFVyTXRyK1NqbGsr?=
 =?utf-8?B?OFRlSWRHNm16YmlwdWtnUnFUYlNyeGVtT25jTlZuWXhLTG51b2U2WkUzakFX?=
 =?utf-8?B?SmgwdnB6SEp6TEZYZFBPUmw0KzMzTTdsNDNEYks0U3FqZERVVmE4R014dU5H?=
 =?utf-8?B?c2FDenVIT2VyMUhNREpNTlpVM2gyU25iMkFlNjQzT0xIeU5FVHJLUmxUbDBH?=
 =?utf-8?B?aElTbmh5bG5jZWtSQ2RtdFhhOTB6M3FZa01SbUF0cnFVdXUrOThpSmgrNHBL?=
 =?utf-8?B?dlFmTGlVaWg0N2xIYVRBTVFhOGR0WDJDVmwzT2RxVXpXTDZuZlZTbFFiTTNj?=
 =?utf-8?B?YUtaNkNPckx5bnc1UGZuUms2N2VUTDVmR2duQURibllNaUNteC9JbUQ1VGdr?=
 =?utf-8?B?Rld0NjZlQlllVUJ3cExaQ2QrRkRSblJYZ3pLUEp0UUxwNjdLdFRPdVN1Wkxz?=
 =?utf-8?B?c3pjbXlNbkZaUWxsS3RvMG8wamx4ZHVHZkVydW5ocytNRzB4T000S0Q3RHRs?=
 =?utf-8?B?WEpuNEtjR0p2V1EzZktmdnlLdllZdFVCU0JITmh3dVNlbDVsOGpTb2U5NkRj?=
 =?utf-8?B?YnpwSUlaSXFIbHBaUTFWTTRNRGE1dVlHZlk1WndnSldjSXNMUVMvamdUZFU5?=
 =?utf-8?B?U1F2ZnZsOUVvWFgwYncyNjdWUjFwN0svMk9ZS2g5STJPSDZnY3kwMWkzeVVN?=
 =?utf-8?B?S042RWZzTi81VCtDYVkzNVNWYXV5Vm56L3pFc0kxanU0SVI0YzJuMC9kQWZz?=
 =?utf-8?B?NkUzZXhxazJaK3lLSGVvNkNXRFQ1ZWM2SzZISzNPT0w0M3R2TkRhMmNvSE1P?=
 =?utf-8?B?bzJLYTYwSG42WGpCeERRSTVTWHU2UDRveTZvK2Q5RW9uaWdPYWZoVnMrKzQ3?=
 =?utf-8?B?RURxcEd0RmtlbVRFTjQ5a3pRbjQ5Q1IrSk8vNitpQnZOaGdWSE56VVUvNGVE?=
 =?utf-8?B?WXhwS3lqTzVxUW5ESDVJZnVSNVlyYWZUalk4c3JPZWJNSVlCU0Zyd2x3UFo0?=
 =?utf-8?B?N3NOS2RRQ0lWdkxBa0VVakFZelY3bk1FcVpTNlNLSHRtTU1oa1hid2R6UWw4?=
 =?utf-8?B?MUFDS1Y4VDlZSTZielR2cHNGSWpqcDYwcUQxSmgxYnkyaWRwejZJQmdESmRP?=
 =?utf-8?B?TXJMais3QktvZE50WHhtQnp5dFlWaWt6dUNRL3U2NEdPTWttUHF1YUJ5TlBr?=
 =?utf-8?B?VWxaNW5nS3VzWmV2WEdqbUU2TUFvMktJTHV1VXROL2xqdUVQWkNTejRTaU1K?=
 =?utf-8?B?TzJFRE5OWFNtWHR5SEQ4QXcrcnNZeTJydDVaWExuanYrV0dpY1h4eU1SVTVL?=
 =?utf-8?B?bUJsOWV1Tk8xbkJianNDNlJDZVE0a0Vtc2ZKaEF0VCtFY21udGg4eHJvNWoz?=
 =?utf-8?B?bDlvMVBkeXY1TDFpR1FZemRPK2c3VGlZSjNZQUNNSUdDWk5pWENuR3VsRDg1?=
 =?utf-8?B?K1RINTVHaHVJQndYcjN1UFc1ejdEMVl6UWxYeGpCVGxZODc2cGZDMWJYYlAy?=
 =?utf-8?B?Z3g3b3NCM2d4QUs2UU03VW9DWUVscHNGWnJyaGRoZ2ppeTBZYjQvR1hKN0pR?=
 =?utf-8?Q?kJY3A1BXP44cT/SIwaTsYPE=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b457931-2d7f-4945-0179-08d9d5486a61
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB2714.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Jan 2022 21:22:05.1239
 (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: om7doy0CEe4nW7XmSMPuHgE1bJk/z1NRnO9d+Qd84TUlNMOqj4GagvweT8Q50hkK7V1v2G3iaho6mf/lkMld1g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3657
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Venu,


On 1/3/22 5:28 PM, Venu Busireddy wrote:
...

>> +
>> +	 /*
>> +	  * Ask the hypervisor to mark the memory pages as private in the RMP
>> +	  * table.
>> +	  */
> 
> Indentation is off. While at it, you may want to collapse it into a one
> line comment.
> 

Based on previous review feedback I tried to keep the comment to 80 
character limit.

>> +	early_set_page_state(paddr, npages, SNP_PAGE_STATE_PRIVATE);
>> +
>> +	/* Validate the memory pages after they've been added in the RMP table. */
>> +	pvalidate_pages(vaddr, npages, 1);
>> +}
>> +
>> +void __init early_snp_set_memory_shared(unsigned long vaddr, unsigned long paddr,
>> +					unsigned int npages)
>> +{
>> +	if (!cc_platform_has(CC_ATTR_SEV_SNP))
>> +		return;
>> +
>> +	/*
>> +	 * Invalidate the memory pages before they are marked shared in the
>> +	 * RMP table.
>> +	 */
> 
> Collapse into one line?
> 

same as above.

...

>> +		/*
>> +		 * ON SNP, the page state in the RMP table must happen
>> +		 * before the page table updates.
>> +		 */
>> +		early_snp_set_memory_shared((unsigned long)__va(pa), pa, 1);
> 
> I know "1" implies "true", but to emphasize that the argument is
> actually a boolean, could you please change the "1" to "true?"
> 

I assume you mean the last argument to the 
early_snp_set_memory_{private,shared}. Please note that its a number of 
pages (unsigned int). The 'true' does not make sense to me.

>> +	}
>> +
>>   	/* Change the page encryption mask. */
>>   	new_pte = pfn_pte(pfn, new_prot);
>>   	set_pte_atomic(kpte, new_pte);
>> +
>> +	/*
>> +	 * If page is set encrypted in the page table, then update the RMP table to
>> +	 * add this page as private.
>> +	 */
>> +	if (enc)
>> +		early_snp_set_memory_private((unsigned long)__va(pa), pa, 1);
> 
> Here too, could you please change the "1" to "true?"
> 

same as above.

thanks