Date:   Mon, 3 Jan 2022 17:28:19 -0600
From:   Venu Busireddy <venu.busireddy@oracle.com>
To:     Brijesh Singh <brijesh.singh@amd.com>
Cc:     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
Message-ID: <YdOGk5b0vYSpP1Ws@dt>
References: <20211210154332.11526-1-brijesh.singh@amd.com>
 <20211210154332.11526-13-brijesh.singh@amd.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211210154332.11526-13-brijesh.singh@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?OfBFvksm4vyra/TILSA8hFBqzI2z4FciJ7SHANLMFzpj4Bk3tCduiHEbW3Wq?=
 =?us-ascii?Q?02OJm1kmA/W9KNMuUUtjNvq1Yk4IdjPkD+DNPJ+AxvEu5by8/71OQB8NhdUK?=
 =?us-ascii?Q?fkp5klWGq5i4oZIUvrS5o9l6cRpl7PQa8TuLWe6SOTpLD5CbPxakzqFuHP1i?=
 =?us-ascii?Q?BEvxf+mmaw1SWasu2iSzBpuZFCzEtn//+8ykVmHMuO2fXUIaPyBiHkBNcnZc?=
 =?us-ascii?Q?gjapyDL/BJqQ4bAxmnfbChqFrcXTeeX9AxSZJaD7BKcW199k70ZyRUjiOvB/?=
 =?us-ascii?Q?oqGvPuEov9fmDB/Nx+oNen8O8JRdg+UjPp3cmkmGk4r4j4I/mb+6dHQ7aMAc?=
 =?us-ascii?Q?ZjsqAT+O0Wqd/acq3cr2sbCBEoOcSE8W8w2IaRpIsvOQRnoArowGUD8OCThM?=
 =?us-ascii?Q?NFdtLja9K5I2thQ3vw/062U8icYgZn+Yov3TRcUkhjl2CjiA/cAXbCCf7aiU?=
 =?us-ascii?Q?b89o+3VmSu94U0c4jusc0c8djZaPJt2hK1FHwDShf/ctETOZQJtAnlRatLgh?=
 =?us-ascii?Q?DcIEg702D6oEx3ZHGFHj0WOs8RtXChI/PwKY6i4z7qAXBOaf04TtbawJLFPS?=
 =?us-ascii?Q?7Vm5BpjoJmDaEyRywyxC1A2Y0zNRV4nO/qW3N1SIDobqQEtiIi1zNi3izdgg?=
 =?us-ascii?Q?9GTRMjowxHryj5YMcrA0AIWGenBuM1Gi7aL8sYNrOeS0B1d101C03dqEZMRj?=
 =?us-ascii?Q?ezjvAAne4jB5p3oSMoxT1c+NXdhUv32FWnk1aXoZmyyIWTAIVKeHAfWVC7ip?=
 =?us-ascii?Q?y9kz8OMloi/UNlkyYycuPkMqmM3kkZ1MWLr6su/d4YYYbfOeOMZHq5UJvW9u?=
 =?us-ascii?Q?RZY6fE5kPQcNbmG7QeDeJz7FV9IDPCzgBCjwN12yOHusUuwjgsQlxJiJ0MBs?=
 =?us-ascii?Q?3enMBPzjJ4uksQcP1mpUVB1cI6fd49bU74/FfhuX2B27Z4KIwrMAuDSPpUSn?=
 =?us-ascii?Q?74Z9KO7fSu9RVgGSgpUZeaMGMgb0pfLtPpO8EJsC8Yv+DeDQ+b+e0NU5+JXJ?=
 =?us-ascii?Q?eiOJBJ/E36B+ApuVV7Hv964P/ejYK6HTymG6hsE4bmd+ItMzjKQEJt9BHDmF?=
 =?us-ascii?Q?ST5wH4HkyFIVpKPmdT9Gs1hPe0qDRDR7eVnFiv37EZvqN9nqXNSx0Ur0lfDb?=
 =?us-ascii?Q?ZZPhr36h7s76KnhxZ+vLjjyf6VmH5oXd+LPA11Y6f41bRzT8oX3TqOAY6ufn?=
 =?us-ascii?Q?lGAIWpN9oVYX/H1abDYn3mdnfUToUWC3FZgrIwMWeDllyCGEJQuTJFFGKL0Y?=
 =?us-ascii?Q?r3g4iKfirfh5lheuju8rZOZyWNx5fUurnANiPWaOBHFQ+uR8Vhaq1naZLPEI?=
 =?us-ascii?Q?L00TWCPmM+BaY7PjrxHm4U2SJeHznqae5SQIa8MnmrRL1VOyxBVCgwfwGPB7?=
 =?us-ascii?Q?3EFRyHIyg8DWlDmbn8vmCzAvMQSaGajV43Q4sYKzqOwjp+2RNHer3+73tvZc?=
 =?us-ascii?Q?U7Omo+KgRtxSJLliWcU9494MoOLRXY3/MAdeMF94cpMQNluXKZBGpH55zsoW?=
 =?us-ascii?Q?h7+WUJTuFwIwd2H8Byg/khCf0balx5pR/u7ybKCR7zZbOI68p1RzAVV6oV1f?=
 =?us-ascii?Q?gQ/Dn74Bg56kQeoGTEkRi+2quHSOEOnncqs1AiLMA7jbO986vWh+mqWsg4fm?=
 =?us-ascii?Q?dNsGiRxVwKAeVT2qPXeFCr9DRcldhoGTmPduyvdTV/vG5OxtjpnVNNWrnXFa?=
 =?us-ascii?Q?AXDjGw=3D=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5b77e7dd-9038-4af9-c871-08d9cf10c167
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB2576.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Jan 2022 23:28:32.2565
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: l0SBb5bmwFgSdHntsfrn6ljpS2L08EVLL9iEOzLsGOI1UlVpB4348eD7I+gA9hXVFdqffmhc7L1ahdu2VoEMGM2UEohRLO2n3OXC6c7AIm4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR10MB3022
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10216 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 mlxscore=0 adultscore=0
 bulkscore=0 suspectscore=0 malwarescore=0 spamscore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2201030157
X-Proofpoint-GUID: Qfz8r0mQqiQxzL6hAvx18UN-RlzkM6Xh
X-Proofpoint-ORIG-GUID: Qfz8r0mQqiQxzL6hAvx18UN-RlzkM6Xh
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021-12-10 09:43:04 -0600, Brijesh Singh wrote:
> The early_set_memory_{encrypt,decrypt}() are used for changing the

s/encrypt,decrypt/encrypted,decrypted/

> page from decrypted (shared) to encrypted (private) and vice versa.
> When SEV-SNP is active, the page state transition needs to go through
> additional steps.
> 
> If the page is transitioned from shared to private, then perform the
> following after the encryption attribute is set in the page table:
> 
> 1. Issue the page state change VMGEXIT to add the page as a private
>    in the RMP table.
> 2. Validate the page after its successfully added in the RMP table.
> 
> To maintain the security guarantees, if the page is transitioned from
> private to shared, then perform the following before clearing the
> encryption attribute from the page table.
> 
> 1. Invalidate the page.
> 2. Issue the page state change VMGEXIT to make the page shared in the
>    RMP table.
> 
> The early_set_memory_{encrypt,decrypt} can be called before the GHCB

s/encrypt,decrypt/encrypted,decrypted/

> is setup, use the SNP page state MSR protocol VMGEXIT defined in the GHCB
> specification to request the page state change in the RMP table.
> 
> While at it, add a helper snp_prep_memory() that can be used outside
> the sev specific files to change the page state for a specified memory
> range.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>

And with a few other nits below:

Reviewed-by: Venu Busireddy <venu.busireddy@oracle.com>

> diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
> +
> +	 /*
> +	  * 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.

> +	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?

> +	pvalidate_pages(vaddr, npages, 0);
> +
> +	 /* Ask hypervisor to mark the memory pages shared in the RMP table. */

Indentation is off.

> +		/*
> +		 * 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?"

> +	}
> +
>  	/* 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?"

Venu