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