Re: [RFC PATCH v1 21/28] KVM: introduce KVM_SEV_ISSUE_CMD ioctl

[Brijesh Singh <brijesh.singh@amd.com>]

Hi Paolo,


On 10/13/2016 05:45 AM, Paolo Bonzini wrote:
>
>
> On 23/08/2016 01:28, Brijesh Singh wrote:
>> The ioctl will be used by qemu to issue the Secure Encrypted
>> Virtualization (SEV) guest commands to transition a guest into
>> into SEV-enabled mode.
>>
>> a typical usage:
>>
>> struct kvm_sev_launch_start start;
>> struct kvm_sev_issue_cmd data;
>>
>> data.cmd = KVM_SEV_LAUNCH_START;
>> data.opaque = &start;
>>
>> ret = ioctl(fd, KVM_SEV_ISSUE_CMD, &data);
>>
>> On SEV command failure, data.ret_code will contain the firmware error code.
>
> Please modify the ioctl to require the file descriptor for the PSP.  A
> program without access to /dev/psp should not be able to use SEV.
>

I am not sure if I fully understand this feedback. Let me summaries what 
we have right now.

At highest level SEV key management commands are divided into two sections:

- platform  management : commands used during platform provisioning. PSP 
drv provides ioctl's for these commands. Qemu will not use these 
ioctl's, i believe these ioctl will be used by other tools.

- guest management: command used during guest life cycle. PSP drv 
exports various function and KVM drv calls these function when it 
receives the SEV_ISSUE_CMD ioctl from qemu.

If I understanding correctly then you are recommending that instead of 
exporting various functions from PSP drv we should expose one function 
for the all the guest command handling (something like this).

int psp_issue_cmd_external_user(struct file *filep,
			    	int cmd, unsigned long addr,
			    	int *psp_ret)
{
	/* here we check to ensure that file->f_ops is a valid
	 * psp instance.
          */
	if (filep->f_ops != &psp_fops)
		return -EINVAL;

	/* handle the command */
	return psp_issue_cmd (cmd, addr, timeout, psp_ret);
}

In KVM driver use something like this to invoke the PSP command handler.

int kvm_sev_psp_cmd (struct kvm_sev_issue_cmd *input,
		     unsigned long data)
{
	int ret;
	struct fd f;

	f = fdget(input->psp_fd);
	if (!f.file)
		return -EBADF;
	....

	psp_issue_cmd_external_user(f.file, input->cmd,
				    data, &input->psp_ret);
	....
}

Please let me know if I understood this correctly.

>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  arch/x86/include/asm/kvm_host.h |    3 +
>>  arch/x86/kvm/x86.c              |   13 ++++
>>  include/uapi/linux/kvm.h        |  125 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 141 insertions(+)
>>