Re: [PATCH Part1 RFC v2 14/20] x86/sev: Add helper for validating pages in early enc attribute changes

[Brijesh Singh]

On 5/26/21 5:39 AM, Borislav Petkov wrote:
> On Fri, Apr 30, 2021 at 07:16:10AM -0500, Brijesh Singh wrote:
>> +static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool validate)
>> +{
>> +	unsigned long vaddr_end;
>> +	int rc;
>> +
>> +	vaddr = vaddr & PAGE_MASK;
>> +	vaddr_end = vaddr + (npages << PAGE_SHIFT);
>> +
>> +	while (vaddr < vaddr_end) {
>> +		rc = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
>> +		if (rc) {
>> +			WARN(rc, "Failed to validate address 0x%lx ret %d", vaddr, rc);
> WARN does return the condition it warned on, look at its definition.
>
> IOW, you can do (and btw e_fail is not needed either):
>
>                 rc = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
>                 if (WARN(rc, "Failed to validate address 0x%lx ret %d", vaddr, rc))
> 			sev_es_terminate(1, GHCB_TERM_PVALIDATE);
>
> Ditto for the other WARN.

Okay, I agree with comment, I will go through each code block and use
the return value from the WARN().

..
>> +	unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>> +
>> +	if (dec) {
>> +		/* If the paddr needs to be accessed decrypted, make the page shared before memcpy. */
>> +		early_snp_set_memory_shared((unsigned long)__va(paddr), paddr, npages);
>> +
>> +		memcpy(dst, src, sz);
>> +
>> +		/* Restore the page state after the memcpy. */
>> +		early_snp_set_memory_private((unsigned long)__va(paddr), paddr, npages);
>> +	} else {
>> +		memcpy(dst, src, sz);
>> +	}
> Hmm, this function needs reorg. How about this:

I think with your edits its looking similar to what I had in v1. In v1
early_snp_set_memory_shared() was not doing sev_snp_active()  check
whereas it does now.I didn't check the sev_snp_active() in this function
but I agree that it can be reorg. I am good with your changes. thanks


>
> /*
>  * When SNP is active, change the page state from private to shared before
>  * copying the data from the source to destination and restore after the copy.
>  * This is required because the source address is mapped as decrypted by the
>  * caller of the routine.
>  */
> static inline void __init snp_memcpy(void *dst, void *src, size_t sz,
>                                      unsigned long paddr, bool decrypt)
> {
>         unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
>
>         if (!sev_snp_active() || !decrypt) {
>                 memcpy(dst, src, sz);
>                 return;
> 	}
>
>         /*
>          * If the paddr needs to be accessed decrypted, mark the page
>          * shared in the RMP table before copying it.
>          */
>         early_snp_set_memory_shared((unsigned long)__va(paddr), paddr, npages);
>
>         memcpy(dst, src, sz);
>
>         /* Restore the page state after the memcpy. */
>         early_snp_set_memory_private((unsigned long)__va(paddr), paddr, npages);
> }
>
>