On 12/17/21 2:47 PM, Venu Busireddy wrote:

quoted

 	 * the caches.
 	 */
-	if ((set | clr) & _PAGE_ENC)
+	if ((set | clr) & _PAGE_ENC) {
 		clflush_page(address);
 
+		/*
+		 * If the encryption attribute is being cleared, then change
+		 * the page state to shared in the RMP table.
+		 */
+		if (clr)

This function is also called by set_page_non_present() with clr set to
_PAGE_PRESENT. Do we want to change the page state to shared even when
the page is not present? If not, shouldn't the check be (clr & _PAGE_ENC)?

I am not able to follow your comment. Here we only pay attention to the
encryption attribute, if encryption attribute is getting cleared then
make PSC. In the case ov set_page_non_present(), the outer if() block
will return false.  Am I missing something ?

quoted

+	/*
+	 * If private -> shared then invalidate the page before requesting the

This comment is confusing. We don't know what the present state is,
right? If we don't, shouldn't we just say:

    If the operation is SNP_PAGE_STATE_SHARED, invalidate the page before
    requesting the state change in the RMP table.

By default all the pages are private, so I don't see any issue with
saying "private -> shared".

quoted

+	 * state change in the RMP table.
+	 */
+	if (op == SNP_PAGE_STATE_SHARED && pvalidate(paddr, RMP_PG_SIZE_4K, 0))
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PVALIDATE);
+
+	/* Issue VMGEXIT to change the page state in RMP table. */
+	sev_es_wr_ghcb_msr(GHCB_MSR_PSC_REQ_GFN(paddr >> PAGE_SHIFT, op));
+	VMGEXIT();
+
+	/* Read the response of the VMGEXIT. */
+	val = sev_es_rd_ghcb_msr();
+	if ((GHCB_RESP_CODE(val) != GHCB_MSR_PSC_RESP) || GHCB_MSR_PSC_RESP_VAL(val))
+		sev_es_terminate(SEV_TERM_SET_LINUX, GHCB_TERM_PSC);
+
+	/*
+	 * Now that page is added in the RMP table, validate it so that it is
+	 * consistent with the RMP entry.

The page is not "added", right? Shouldn't we just say:

Technically, PSC modifies the RMP entry, so I should use that  instead
of calling "added".

    Validate the page so that it is consistent with the RMP entry.

Yes, I am okay with it.

Venu