Messages in this thread |  | | Date | Fri, 17 Dec 2021 17:24:43 -0600 | Subject | Re: [PATCH v8 09/40] x86/compressed: Add helper for validating pages in the decompression stage | From | Brijesh Singh <> |
| |
On 12/17/21 2:47 PM, Venu Busireddy wrote:
>> * 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 ?
>> + /* >> + * 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".
>> + * 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
|  |