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:4.438 / U:0.468 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site