Re: [RFC PATCH v2 30/32] kvm: svm: Add support for SEV DEBUG_ENCRYPT command

[Paolo Bonzini]

On 02/03/2017 16:18, Brijesh Singh wrote:
> +	data = (void *) get_zeroed_page(GFP_KERNEL);

The page does not need to be zeroed, does it?

> +
> +	if ((len & 15) || (dst_addr & 15)) {
> +		/* if destination address and length are not 16-byte
> +		 * aligned then:
> +		 * a) decrypt destination page into temporary buffer
> +		 * b) copy source data into temporary buffer at correct offset
> +		 * c) encrypt temporary buffer
> +		 */
> +		ret = __sev_dbg_decrypt_page(kvm, dst_addr, data, &argp->error);

Ah, I see now you're using this function here for read-modify-write.
data is already pinned here, so even if you keep the function it makes
sense to push pinning out of __sev_dbg_decrypt_page and into
sev_dbg_decrypt.

> +		if (ret)
> +			goto err_3;
> +		d_off = dst_addr & (PAGE_SIZE - 1);
> +
> +		if (copy_from_user(data + d_off,
> +					(uint8_t *)debug.src_addr, len)) {
> +			ret = -EFAULT;
> +			goto err_3;
> +		}
> +
> +		encrypt->length = PAGE_SIZE;

Why decrypt/re-encrypt all the page instead of just the 16 byte area
around the [dst_addr, dst_addr+len) range?

> +		encrypt->src_addr = __psp_pa(data);
> +		encrypt->dst_addr =  __sev_page_pa(inpages[0]);
> +	} else {
> +		if (copy_from_user(data, (uint8_t *)debug.src_addr, len)) {
> +			ret = -EFAULT;
> +			goto err_3;
> +		}

Do you need copy_from_user, or can you just pin/unpin memory as for
DEBUG_DECRYPT?

Paolo

> +		d_off = dst_addr & (PAGE_SIZE - 1);
> +		encrypt->length = len;
> +		encrypt->src_addr = __psp_pa(data);
> +		encrypt->dst_addr = __sev_page_pa(inpages[0]);
> +		encrypt->dst_addr += d_off;
> +	}
> +
> +	encrypt->handle = sev_get_handle(kvm);
> +	ret = sev_issue_cmd(kvm, SEV_CMD_DBG_ENCRYPT, encrypt, &argp->error);