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);
> }
>
>
