Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

On 10/11/2017 09:19 AM, Borislav Petkov wrote:

On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:

Basically we need some variable which is outside the per-device
structure so that we don't end up creating multiple /dev/sev nodes. If
needed, I think we can remove 'has_sev_fops' variable from struct
psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
struct psp_device. The 'has_sev_fops' is  mainly used in sev_exit(). If
we decide to dynamic alloc sev_misc then in  sev_exit() we can use
psp->sev_misc != NULL instead of psp->has_sev_ops.


Yap, that sounds better than an explicit variable.



Sure, I will implement it and send you v5.2. thanks

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> Basically we need some variable which is outside the per-device
> structure so that we don't end up creating multiple /dev/sev nodes. If
> needed, I think we can remove 'has_sev_fops' variable from struct
> psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
> struct psp_device. The 'has_sev_fops' is  mainly used in sev_exit(). If
> we decide to dynamic alloc sev_misc then in  sev_exit() we can use
> psp->sev_misc != NULL instead of psp->has_sev_ops.

Yap, that sounds better than an explicit variable.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Tue, Oct 10, 2017 at 01:43:22PM -0500, Tom Lendacky wrote:
> Maybe for the very first implementation we could do that and that was what
> was originally done for the CCP.  But as you can see the CCP does not have
> a set register offset between various iterations of the device and it can
> be expected the same will hold true for the PSP.  This just makes future
> changes easier in order to support newer devices.

Well, that's a simple switch-case statement which maps device iteration
to offset.

> I would prefer to keep things separated as they are.  The common code is
> one place and the pci/platform specific code resides in unique files. For
> sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined
> vs. adding #ifdefs into sp-dev.c or sp.c.  The code is working nicely and,
> at least to me, seems easily maintainable this way.

But you do see that you're doing a bunch of unnecessary things during
probing. And all those different devices: SP, CCP, PSP, TEE and whatnot,
they're just too granulary and it is a bunch of registration code and
one compilation unit calling into the other for no good reason. Or at
least I don't see it.

> If we really want to avoid the extra calls during probing, etc.
> then we can take a closer look afterwards and determine what is the
> best approach taking into account the CCP and some of the other PSP
> functionality that is coming.

And that coming functionality won't make things easier - you'll most
likely have more things wanting to talk to more other things. But ok,
your call. Just note that changing things later is always harder.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Tom Lendacky]

On 10/10/2017 10:00 AM, Brijesh Singh wrote:



On 10/09/2017 10:21 AM, Borislav Petkov wrote:
...




03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456


Btw, what do those PCI functions each do? Public PPR doesn't have them
documented.



Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468 
provides the support CCP support directly on the x86-side and device id 
0x1456 provides the support for both CCP and PSP features through the AMD 
Secure Processor (AMD-SP).





Sure, and if you manage all the devices in a single driver, you can
simply keep them all in a linked list or in an array and iterating over
them is trivial.

Because right now you have

1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection

2. at some point, it does sp-dev.c::sp_init() which decides whether CCP 
or PSP


3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
sp->dev_vdata->psp_vdata which is nothing more than a simple offset
0x10500 which is where the PSP io regs are. For example, if this offset
is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
0x10500. No need for all that passing of structs around.


Maybe for the very first implementation we could do that and that was what
was originally done for the CCP.  But as you can see the CCP does not have
a set register offset between various iterations of the device and it can
be expected the same will hold true for the PSP.  This just makes future
changes easier in order to support newer devices.



4. and finally, after that *whole* init has been done, you get to do
->set_psp_master_device(sp);

Or, you can save yourself all that jumping through hoops, merge sp-pci.c
and sp-dev.c into a single sp.c and put *everything* sp-related into
it. And then do the whole work of picking hw apart, detection and
configuration in sp_pci_probe() and have helper functions preparing and
configuring the device.

At the end, it adds it to the list of devices sp.c manages and done. You
actually have that list already:

static LIST_HEAD(sp_units);

in sp-dev.c.

You don't need the set_master thing either - you simply set the
sp_dev_master pointer inside sp.c




I was trying to avoid putting PSP/SEV specific changes in sp-dev.* files. 
But if sp.c approach is acceptable to the maintainer then I can work 
towards merging sp-dev.c and sp-pci.c into sp.c and then add the PSP/SEV 
support.


I would prefer to keep things separated as they are.  The common code is
one place and the pci/platform specific code resides in unique files. For
sp-pci.c, it can be excluded from compilation if CONFIG_PCI is not defined
vs. adding #ifdefs into sp-dev.c or sp.c.  The code is working nicely and,
at least to me, seems easily maintainable this way.  If we really want to
avoid the extra calls during probing, etc. then we can take a closer look
afterwards and determine what is the best approach taking into account
the CCP and some of the other PSP functionality that is coming.

Thanks,
Tom





sp_init() can then go and you can replace it with its function body,
deciding whether it is a CCP or PSP and then call the respective
function which is also in sp.c or ccp-dev.c

And then all those separate compilation units and the interfaces between
them disappear - you have only the calls into the PSP and that's it.

Btw, the CCP thing could remain separate initially, I guess, with all
that ccp-* stuff in there.



Yep, if we decide to go with your recommended approach then we should 
leave the CCP as-is for now.




I was trying to follow the CCP  model -- in which sp-dev.c simply
forwards the call to ccp-dev.c which does the real work.


And you don't really need that - you can do the real work directly in
sp-dev.c or sp.c or whatever.
 >> Currently, sev-dev.c contains barebone common code. IMO, keeping all

the PSP private functions and data structure outside the sp-dev.c/.h
is right thing.


By this model probably, but it causes all that init and registration
jump-through-hoops for no real reason. It is basically wasting cycles
and energy.

I'm all for splitting if it makes sense. But right now I don't see much
sense in this - it is basically a bunch of small compilation units
calling each other. And they could be merged into a single sp.c which
does it all in one go, without a lot of blabla.



Additionally, I would like to highlight that if we decide to go with
moving all the PSP functionality in sp-dev.c then we have to add #ifdef
CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
the sp-dev.c gets compiled for all architectures (including aarch64,
i386 and x86_64).


That's fine. You can build it on 32-bit but add to the init function

if (IS_ENABLED(CONFIG_X86_32))
    return -ENODEV;

and be done with it. No need for the ifdeffery.



OK, i will use IS_ENABLED where applicable.

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

On 10/09/2017 10:21 AM, Borislav Petkov wrote:
...




03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456


Btw, what do those PCI functions each do? Public PPR doesn't have them
documented.



Looking at the pci_device_id table (sp-pci.c), the devices id 0x1468 
provides the support CCP support directly on the x86-side and device id 
0x1456 provides the support for both CCP and PSP features through the 
AMD Secure Processor (AMD-SP).





Sure, and if you manage all the devices in a single driver, you can
simply keep them all in a linked list or in an array and iterating over
them is trivial.

Because right now you have

1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection

2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP

3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
sp->dev_vdata->psp_vdata which is nothing more than a simple offset
0x10500 which is where the PSP io regs are. For example, if this offset
is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
0x10500. No need for all that passing of structs around.

4. and finally, after that *whole* init has been done, you get to do
->set_psp_master_device(sp);

Or, you can save yourself all that jumping through hoops, merge sp-pci.c
and sp-dev.c into a single sp.c and put *everything* sp-related into
it. And then do the whole work of picking hw apart, detection and
configuration in sp_pci_probe() and have helper functions preparing and
configuring the device.

At the end, it adds it to the list of devices sp.c manages and done. You
actually have that list already:

static LIST_HEAD(sp_units);

in sp-dev.c.

You don't need the set_master thing either - you simply set the
sp_dev_master pointer inside sp.c




I was trying to avoid putting PSP/SEV specific changes in sp-dev.* 
files. But if sp.c approach is acceptable to the maintainer then I can 
work towards merging sp-dev.c and sp-pci.c into sp.c and then add the 
PSP/SEV support.




sp_init() can then go and you can replace it with its function body,
deciding whether it is a CCP or PSP and then call the respective
function which is also in sp.c or ccp-dev.c

And then all those separate compilation units and the interfaces between
them disappear - you have only the calls into the PSP and that's it.

Btw, the CCP thing could remain separate initially, I guess, with all
that ccp-* stuff in there.



Yep, if we decide to go with your recommended approach then we should 
leave the CCP as-is for now.




I was trying to follow the CCP  model -- in which sp-dev.c simply
forwards the call to ccp-dev.c which does the real work.


And you don't really need that - you can do the real work directly in
sp-dev.c or sp.c or whatever.
 >> Currently, sev-dev.c contains barebone common code. IMO, keeping all

the PSP private functions and data structure outside the sp-dev.c/.h
is right thing.


By this model probably, but it causes all that init and registration
jump-through-hoops for no real reason. It is basically wasting cycles
and energy.

I'm all for splitting if it makes sense. But right now I don't see much
sense in this - it is basically a bunch of small compilation units
calling each other. And they could be merged into a single sp.c which
does it all in one go, without a lot of blabla.



Additionally, I would like to highlight that if we decide to go with
moving all the PSP functionality in sp-dev.c then we have to add #ifdef
CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
the sp-dev.c gets compiled for all architectures (including aarch64,
i386 and x86_64).


That's fine. You can build it on 32-bit but add to the init function

if (IS_ENABLED(CONFIG_X86_32))
return -ENODEV;

and be done with it. No need for the ifdeffery.



OK, i will use IS_ENABLED where applicable.

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Sun, Oct 08, 2017 at 07:11:04PM -0500, Brijesh Singh wrote:
> There is a single security processor driver (ccp) which provides the
> complete functionality including PSP.  But the driver should be able to
> work with multiple devices. e.g In my 2P EPYC configuration, security
> processor driver is used for the following devices
> 
> 02:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1456

So we have a lot of drivers which claim more than one PCI device. It is
not mandatory to have a driver per PCI device. Actually, the decisive
argument is what is the easiest way to manage those devices and what
makes the communication between them the easiest.

> 03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1468
> 13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
> 1456

Btw, what do those PCI functions each do? Public PPR doesn't have them
documented.

> Some of the these devices support CCP only and some support both PSP and
> CCP. It's possible that multiple devices support the PSP functionality
> but only one of them is master which can be used for issuing SEV
> commands. There isn't a register which we can read to determine whether
> the device is master. We have to probe all the devices which supports
> the PSP to determine which one of them is a master.

Sure, and if you manage all the devices in a single driver, you can
simply keep them all in a linked list or in an array and iterating over
them is trivial.

Because right now you have

1. sp-pci.c::sp_pci_probe() execute upon the PCI device detection

2. at some point, it does sp-dev.c::sp_init() which decides whether CCP or PSP

3. If PSP, it calls pcp-dev.c::psp_dev_init() which gets that
sp->dev_vdata->psp_vdata which is nothing more than a simple offset
0x10500 which is where the PSP io regs are. For example, if this offset
is hardcoded, why are we even passing that vdata? Just set psp->io_regs =
0x10500. No need for all that passing of structs around.

4. and finally, after that *whole* init has been done, you get to do
->set_psp_master_device(sp);

Or, you can save yourself all that jumping through hoops, merge sp-pci.c
and sp-dev.c into a single sp.c and put *everything* sp-related into
it. And then do the whole work of picking hw apart, detection and
configuration in sp_pci_probe() and have helper functions preparing and
configuring the device.

At the end, it adds it to the list of devices sp.c manages and done. You
actually have that list already:

static LIST_HEAD(sp_units);

in sp-dev.c.

You don't need the set_master thing either - you simply set the
sp_dev_master pointer inside sp.c

sp_init() can then go and you can replace it with its function body,
deciding whether it is a CCP or PSP and then call the respective
function which is also in sp.c or ccp-dev.c

And then all those separate compilation units and the interfaces between
them disappear - you have only the calls into the PSP and that's it.

Btw, the CCP thing could remain separate initially, I guess, with all
that ccp-* stuff in there.

> I was trying to follow the CCP  model -- in which sp-dev.c simply
> forwards the call to ccp-dev.c which does the real work.

And you don't really need that - you can do the real work directly in
sp-dev.c or sp.c or whatever.

> Currently, sev-dev.c contains barebone common code. IMO, keeping all
> the PSP private functions and data structure outside the sp-dev.c/.h
> is right thing.

By this model probably, but it causes all that init and registration
jump-through-hoops for no real reason. It is basically wasting cycles
and energy.

I'm all for splitting if it makes sense. But right now I don't see much
sense in this - it is basically a bunch of small compilation units
calling each other. And they could be merged into a single sp.c which
does it all in one go, without a lot of blabla.

> Additionally, I would like to highlight that if we decide to go with
> moving all the PSP functionality in sp-dev.c then we have to add #ifdef
> CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
> the sp-dev.c gets compiled for all architectures (including aarch64,
> i386 and x86_64).

That's fine. You can build it on 32-bit but add to the init function

if (IS_ENABLED(CONFIG_X86_32))
return -ENODEV;

and be done with it. No need for the ifdeffery.

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

On 10/8/17 9:00 AM, Borislav Petkov wrote:
> On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
>> During the device probe, sev_ops_init() will be called for every device
>> instance which claims to support the SEV.  One of the device will be
>> 'master' but we don't the master until we probe all the instances. Hence
>> the probe for all SEV devices must return success.
> I still am wondering whether that design with multiple devices - master
> and non-master devices is optimal. Why isn't the security processor a
> single driver which provides the whole functionality, PSP including? Why
> do you need all that register and unregister glue and get_master bla if
> you can simply put the whole thing in a single module?

There is a single security processor driver (ccp) which provides the
complete functionality including PSP.  But the driver should be able to
work with multiple devices. e.g In my 2P EPYC configuration, security
processor driver is used for the following devices

02:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
03:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
13:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
14:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
22:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
23:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
31:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
32:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
41:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
42:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
51:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
52:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
61:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
62:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468
71:00.2 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1456
72:00.1 Encryption controller: Advanced Micro Devices, Inc. [AMD] Device
1468

Some of the these devices support CCP only and some support both PSP and
CCP. It's possible that multiple devices support the PSP functionality
but only one of them is master which can be used for issuing SEV
commands. There isn't a register which we can read to determine whether
the device is master. We have to probe all the devices which supports
the PSP to determine which one of them is a master.
 
> And the fact that you need a global variable to mark that you've
> registered the misc device should already tell you that something's
> not optimal. Because if you had a single driver, it will go, detect
> the whole functionality, initialize it, register services and be done
> with it. No registering of devices, no finding of masters, no global
> variables, no unnecessary glue.
>
> IOW, in this diagram:
>
>  +--- CCP
>  |
> AMD-SP --|
>  |+--- SEV
>  ||
>  + PSP ---*
>   |
>   + TEE
>
> why isn't the whole PSP functionality part of drivers/crypto/ccp/sp-dev.c ?
>
> That driver is almost barebones minimal thing. You can very well add the
> PSP/SEV stuff in there and if there's an *actual* reason to carve pieces
> out, only then to do so, not to split it now unnecessarily and make your
> life complicated for no reason.

I was trying to follow the CCP  model -- in which sp-dev.c simply
forwards the call to ccp-dev.c which does the real work. Currently,
sev-dev.c contains barebone common code. IMO, keeping all the PSP
private functions and data structure outside the sp-dev.c/.h is right
thing. Having said that, I am okay with moving all the PSP stuff in
sp-dev.c but before we do that lets see what CCP maintainers say.

Tom and Gary,  comments please ?
 
Additionally, I would like to highlight that if we decide to go with
moving all the PSP functionality in sp-dev.c then we have to add #ifdef
CONFIG_CRYPTO_DEV_SP_PSP because PSP feature depends on X86_66, whereas
the sp-dev.c gets compiled for all architectures (including aarch64,
i386 and x86_64).
 
>
> Or am I missing some obvious and important reason?

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Sun, Oct 08, 2017 at 08:30:47AM -0500, Brijesh Singh wrote:
> During the device probe, sev_ops_init() will be called for every device
> instance which claims to support the SEV.  One of the device will be
> 'master' but we don't the master until we probe all the instances. Hence
> the probe for all SEV devices must return success.

I still am wondering whether that design with multiple devices - master
and non-master devices is optimal. Why isn't the security processor a
single driver which provides the whole functionality, PSP including? Why
do you need all that register and unregister glue and get_master bla if
you can simply put the whole thing in a single module?

And the fact that you need a global variable to mark that you've
registered the misc device should already tell you that something's
not optimal. Because if you had a single driver, it will go, detect
the whole functionality, initialize it, register services and be done
with it. No registering of devices, no finding of masters, no global
variables, no unnecessary glue.

IOW, in this diagram:

 +--- CCP
 |
AMD-SP --|
 |+--- SEV
 ||
 + PSP ---*
  |
  + TEE

why isn't the whole PSP functionality part of drivers/crypto/ccp/sp-dev.c ?

That driver is almost barebones minimal thing. You can very well add the
PSP/SEV stuff in there and if there's an *actual* reason to carve pieces
out, only then to do so, not to split it now unnecessarily and make your
life complicated for no reason.

Or am I missing some obvious and important reason?

-- 
Regards/Gruss,
Boris.

SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 
(AG Nürnberg)
-- 

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Brijesh Singh]

On 10/7/17 1:40 PM, Borislav Petkov wrote:
...
> A bunch of fixes ontop:
>
> * sev_fops_registered is superfluous if you can use psp->has_sev_fops

I am okay with all your fixes except this one. I will add my comment below.

...
>  static int sev_ops_init(struct psp_device *psp)
>  {
>   struct miscdevice *misc = >sev_misc;
> - int ret = 0;
> + int ret;
>  
>   /*
> -  * SEV feature support can be detected on the multiple devices but the
> -  * SEV FW commands must be issued on the master. During probe time we
> -  * do not know the master hence we create /dev/sev on the first device
> -  * probe. sev_handle_cmd() finds the right master device to when issuing
> -  * the command to the firmware.
> +  * SEV feature support can be detected on multiple devices but the SEV
> +  * FW commands must be issued on the master. During probe, we do not
> +  * know the master hence we create /dev/sev on the first device probe.
> +  * sev_do_cmd() finds the right master device to which to issue the
> +  * command to the firmware.
>*/
> - if (!sev_fops_registered) {
> - misc->minor = MISC_DYNAMIC_MINOR;
> - misc->name = DEVICE_NAME;
> - misc->fops = _fops;
> -
> - ret = misc_register(misc);
> - if (!ret) {
> - sev_fops_registered = true;
> - psp->has_sev_fops = true;
> - init_waitqueue_head(>sev_int_queue);
> - dev_info(psp->dev, "registered SEV device\n");
> - }
> + if (psp->has_sev_fops)
> + return 0;
> +

This will always be false.  The struct psp_device is used for storing
per-device instance.

> + misc->minor = MISC_DYNAMIC_MINOR;
> + misc->name = DEVICE_NAME;
> + misc->fops = _fops;
> +
> + ret = misc_register(misc);
> + if (!ret) {
> + psp->has_sev_fops = true;
> + init_waitqueue_head(>sev_int_queue);
> + dev_info(psp->dev, "registered SEV device\n");
>   }


During the device probe, sev_ops_init() will be called for every device
instance which claims to support the SEV.  One of the device will be
'master' but we don't the master until we probe all the instances. Hence
the probe for all SEV devices must return success.

With your changes, the first device instance will able to create
/dev/sev node but all other instances will fail hence the probe routine
for other instances will also fail.

Basically we need some variable which is outside the per-device
structure so that we don't end up creating multiple /dev/sev nodes. If
needed, I think we can remove 'has_sev_fops' variable from struct
psp_device if we decide to dynamic alloc 'struct miscdevice sev_misc' in
struct psp_device. The 'has_sev_fops' is  mainly used in sev_exit(). If
we decide to dynamic alloc sev_misc then in  sev_exit() we can use
psp->sev_misc != NULL instead of psp->has_sev_ops.


>  
>   return ret;
> diff --git a/drivers/crypto/ccp/psp-dev.h b/drivers/crypto/ccp/psp-dev.h
> index 6e8f83b41521..60a33f5ff79f 100644
> --- a/drivers/crypto/ccp/psp-dev.h
> +++ b/drivers/crypto/ccp/psp-dev.h
> @@ -36,7 +36,7 @@
>  #define PSP_CMDBUFF_ADDR_HI PSP_C2PMSG(57)
>  #define PSP_FEATURE_REG  PSP_C2PMSG(63)
>  
> -#define PSP_P2CMSG(_num) (_num << 2)
> +#define PSP_P2CMSG(_num) ((_num) << 2)
>  #define PSP_CMD_COMPLETE_REG 1
>  #define PSP_CMD_COMPLETE PSP_P2CMSG(PSP_CMD_COMPLETE_REG)
>  
> diff --git a/include/linux/psp-sev.h b/include/linux/psp-sev.h
> index 2b334fd853c9..10b843cce75f 100644
> --- a/include/linux/psp-sev.h
> +++ b/include/linux/psp-sev.h
> @@ -512,8 +512,7 @@ struct sev_data_dbg {
>   u32 len;/* In */
>  } __packed;
>  
> -#if defined(CONFIG_CRYPTO_DEV_SP_PSP)
> -
> +#ifdef CONFIG_CRYPTO_DEV_SP_PSP
>  /**
>   * sev_platform_init - perform SEV INIT command
>   *
> @@ -562,9 +561,9 @@ int sev_platform_status(struct sev_data_status *status, 
> int *error);
>   * sev_issue_cmd_external_user - issue SEV command by other driver with a 
> file
>   * handle.
>   *
> - * The function can be used by other drivers to issue a SEV command on
> - * behalf by userspace. The caller must pass a valid SEV file descriptor
> - * so that we know that caller has access to SEV device.
> + * This function can be used by other drivers to issue a SEV command on
> + * behalf of userspace. The caller must pass a valid SEV file descriptor
> + * so that we know that it has access to SEV device.
>   *
>   * @filep - SEV device file pointer
>   * @cmd - command to issue
>

Re: [Part2 PATCH v5.1 12.1/31] crypto: ccp: Add Secure Encrypted Virtualization (SEV) command support

[Borislav Petkov]

On Fri, Oct 06, 2017 at 08:05:59PM -0500, Brijesh Singh wrote:
> AMD's new Secure Encrypted Virtualization (SEV) feature allows the
> memory contents of virtual machines to be transparently encrypted with a
> key unique to the VM. The programming and management of the encryption
> keys are handled by the AMD Secure Processor (AMD-SP) which exposes the
> commands for these tasks. The complete spec is available at:
> 
> http://support.amd.com/TechDocs/55766_SEV-KM%20API_Specification.pdf
> 
> Extend the AMD-SP driver to provide the following support:
> 
>  - an in-kernel API to communicate with the SEV firmware. The API can be
>used by the hypervisor to create encryption context for a SEV guest.
> 
>  - a userspace IOCTL to manage the platform certificates.
> 
> Cc: Paolo Bonzini 
> Cc: "Radim Krčmář" 
> Cc: Borislav Petkov 
> Cc: Herbert Xu 
> Cc: Gary Hook 
> Cc: Tom Lendacky 
> Cc: linux-crypto@vger.kernel.org
> Cc: k...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org
> Improvements-by: Borislav Petkov 
> Signed-off-by: Brijesh Singh 
> ---
> 
> Based on Boris feedback split this patch in 9 logical patches, they are
> numbers from 12.1 to 12.9.
> 
>  drivers/crypto/ccp/psp-dev.c | 244 
> +++
>  drivers/crypto/ccp/psp-dev.h |  17 +++
>  include/linux/psp-sev.h  | 159 
>  3 files changed, 420 insertions(+)

A bunch of fixes ontop:

* sev_fops_registered is superfluous if you can use psp->has_sev_fops

* s/sev_handle_cmd/sev_do_cmd/g - it really is sev_do_cmd(). "handle" is
something else.

* Flip misc dev reg logic in sev_ops_init()

* PSP_P2CMSG needs arg eval

* text streamlining

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e9b776c3acb2..f0e0fc1fb512 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -31,7 +31,6 @@
 #define DEVICE_NAME"sev"
 
 static DEFINE_MUTEX(sev_cmd_mutex);
-static bool sev_fops_registered;
 
 static struct psp_device *psp_alloc_struct(struct sp_device *sp)
 {
@@ -121,7 +120,7 @@ static int sev_cmd_buffer_len(int cmd)
return 0;
 }
 
-static int sev_handle_cmd(int cmd, void *data, int *psp_ret)
+static int sev_do_cmd(int cmd, void *data, int *psp_ret)
 {
unsigned int phys_lsb, phys_msb;
struct psp_device *psp;
@@ -182,26 +181,26 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
return -ENOTTY;
 }
 
-const struct file_operations sev_fops = {
+static const struct file_operations sev_fops = {
.owner  = THIS_MODULE,
.unlocked_ioctl = sev_ioctl,
 };
 
 int sev_platform_init(struct sev_data_init *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_INIT, data, error);
+   return sev_do_cmd(SEV_CMD_INIT, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_init);
 
 int sev_platform_shutdown(int *error)
 {
-   return sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, error);
+   return sev_do_cmd(SEV_CMD_SHUTDOWN, 0, error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_shutdown);
 
 int sev_platform_status(struct sev_data_status *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+   return sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_platform_status);
 
@@ -211,58 +210,58 @@ int sev_issue_cmd_external_user(struct file *filep, 
unsigned int cmd,
if (!filep || filep->f_op != _fops)
return -EBADF;
 
-   return sev_handle_cmd(cmd, data, error);
+   return sev_do_cmd(cmd, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_issue_cmd_external_user);
 
 int sev_guest_deactivate(struct sev_data_deactivate *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_DEACTIVATE, data, error);
+   return sev_do_cmd(SEV_CMD_DEACTIVATE, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_deactivate);
 
 int sev_guest_activate(struct sev_data_activate *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_ACTIVATE, data, error);
+   return sev_do_cmd(SEV_CMD_ACTIVATE, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_activate);
 
 int sev_guest_decommission(struct sev_data_decommission *data, int *error)
 {
-   return sev_handle_cmd(SEV_CMD_DECOMMISSION, data, error);
+   return sev_do_cmd(SEV_CMD_DECOMMISSION, data, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_decommission);
 
 int sev_guest_df_flush(int *error)
 {
-   return sev_handle_cmd(SEV_CMD_DF_FLUSH, 0, error);
+   return sev_do_cmd(SEV_CMD_DF_FLUSH, 0, error);
 }
 EXPORT_SYMBOL_GPL(sev_guest_df_flush);
 
 static int sev_ops_init(struct psp_device *psp)
 {
struct miscdevice *misc = >sev_misc;
-   int ret = 0;
+   int ret;
 
/*
-* SEV feature support can be detected on the multiple devices but the
-* SEV FW commands must