Re: [PATCH v8 09/40] x86/compressed: Add helper for validating pages in the decompression stage

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux