Re: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 10/30/2017 12:21 PM, Borislav Petkov wrote:
...


Useless forward declarations.


Actually its helpful in other patches. I was trying to avoid making too many code movement in other patches to eliminate the forward declarations. I guess I can fix in v7.


  static struct psp_device *psp_alloc_struct(struct sp_device *sp)
  {
  	struct device *dev = sp->dev;

...

+static int sev_do_cmd_locked(int cmd, void *data, int *psp_ret)

You can use the "__" prefix to denote that it is a lower-level helper:

__sev_do_cmd
__sev_do_cmd_locked

Ditto for the other locked functions.

noted

...

+static int sev_platform_init_locked(struct sev_data_init *data, int *error)
+{
+	struct psp_device *psp = psp_master;
+	struct sev_data_init *input = NULL;
+	int rc = 0;
+
+	if (!psp)
+		return -ENODEV;
+
+	if (psp->sev_state == SEV_STATE_INIT)
+		return 0;
+
+	if (!data) {
+		input = kzalloc(sizeof(*input), GFP_KERNEL);
+		if (!input)
+			return -ENOMEM;
+
+		data = input;
+	}

You can do the allocation in the enclosing function, outside of the
critical region so that you can keep it shorter.

Or even better: if you're going to synchronize the commands with a
mutex, you can define a static struct sev_data_init input in this file
which you always hand in and then you can save yourself the kmalloc
calls.


If the buffer is allocated on the stack then there is no guarantee that __pa() will gives us a valid physical address. IIRC, when CONFIG_VMAP_STACK=y then stack space is mapped similar to vmalloc'd storage and __pa() will not work.

Since we need to pass the physical address to PSP hence variable allocated on the stack will not work.

I can certainly move the allocation outside, but then it may increase the code size in other functions. If its not a big deal then I would prefer to keep what we have.


...
+
+int sev_platform_shutdown(int *error)
+{
+	if (error)
+		*error = 0;
+
+	return 0;
+}

I'm guessing that that's just bare-bones and it will get filled up in
the next patches. Otherwise it looks pretty useless.


Well, we are not expanding in other patches. I was also debating on what to do with this function. Since we need sev_platform_init() hence it made sense to add sev_platform_shutdown() as well. If we add the function then I wanted to make sure that we set the *error = SUCCESS so that caller knows that function succeeded.


If it is just to block the user from sending SHUTDOWN to the PSP
master, just do that in the ioctl directly - no need to call some empty
functions.


The function is not used by userspace ioctl, its used by kvm drv when it launch/terminates the SEV guest.

-Brijesh



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux