lkml.org 
[lkml]   [2017]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Part2 PATCH v6.1 16/38] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support
From
Date


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

\
 
 \ /
  Last update: 2017-10-30 18:50    [W:0.104 / U:0.936 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site