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
