date:20171012

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-12 Thread Brijesh Singh



On 10/12/17 9:24 PM, Brijesh Singh wrote:
>
> On 10/12/17 2:53 PM, Borislav Petkov wrote:
> ...
>
>> Ok, a couple of things here:
>>
>> * Move the checks first and the allocations second so that you allocate
>> memory only after all checks have been passed and you don't allocate
>> pointlessly.
>
> I assume you mean performing the SEV state check before allocating the
> memory for the CSR blob, right ? In my patches, I typically perform all
> the SW specific checks and allocation before invoking the HW routines.
> Handling the PSP commands will take longer compare to kmalloc() or
> access_ok() etc. If its not a big deal then I would prefer to keep that
> way.
>
>
>> * That:
>>
>> if (state == SEV_STATE_WORKING) {
>> ret = -EBUSY;
>> goto e_free_blob;
>> } else if (state == SEV_STATE_UNINIT) {
>> ret = sev_firmware_init(>error);
>> if (ret)
>> goto e_free_blob;
>> do_shutdown = 1;
>> }
>>
>> is a repeating pattern. Perhaps it should be called
>> sev_firmware_reinit() and called by other functions.
>
>> * The rest is simplifications and streamlining.
>>
>> ---
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index e3ee68afd068..d41f5448a25b 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd 
>> *argp)
>>  int ret, state;
>>  void *blob;
>>  
>> -if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
>> -   sizeof(struct sev_user_data_pek_csr)))
>> +if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
>> +return -EFAULT;
>> +
>> +if (!input.address)
>> +return -EINVAL;
>> +


As per the spec, its perfectly legal to pass input.address = 0x0 and
input.length = 0x0. If userspace wants to query CSR length then it will
fill all the fields with. In response, FW will return error
(LENGTH_TO_SMALL) and input.length will be filled with the expected
length. Several command work very similar (e.g PEK_CSR,
PEK_CERT_EXPORT). A typical usage from userspace will be:

- query the length of the blob (call command with all fields set to zero)
- SEV FW will response with expected length
- userspace allocate the blob and retries the command. 


>> +/* allocate a physically contiguous buffer to store the CSR blob */
>> +if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
>> +input.length > SEV_FW_BLOB_MAX_SIZE)
>>  return -EFAULT;
>>  
>>  data = kzalloc(sizeof(*data), GFP_KERNEL);
>>  if (!data)
>>  return -ENOMEM;
>>  
>> -/* allocate a temporary physical contigous buffer to store the CSR blob 
>> */
>> -blob = NULL;
>> -if (input.address) {
>> -if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
>> -input.length > SEV_FW_BLOB_MAX_SIZE) {
>> -ret = -EFAULT;
>> -goto e_free;
>> -}
>> -
>> -blob = kmalloc(input.length, GFP_KERNEL);
>> -if (!blob) {
>> -ret = -ENOMEM;
>> -goto e_free;
>> -}
>> -
>> -data->address = __psp_pa(blob);
>> -data->len = input.length;
>> +blob = kmalloc(input.length, GFP_KERNEL);
>> +if (!blob) {
>> +ret = -ENOMEM;
>> +goto e_free;
>>  }
>>  
>> +data->address = __psp_pa(blob);
>> +data->len = input.length;
>> +
>>  ret = sev_platform_get_state(, >error);
>>  if (ret)
>>  goto e_free_blob;
>> @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd 
>> *argp)
>>  do_shutdown = 1;
>>  }
>>  
>> -ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
>> +ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
>>  
>>  input.length = data->len;
>>  
>>  /* copy blob to userspace */
>> -if (blob &&
>> -copy_to_user((void __user *)(uintptr_t)input.address,
>> -blob, input.length)) {
>> +if (copy_to_user((void __user *)input.address, blob, input.length)) {
>>  ret = -EFAULT;
>>  goto e_shutdown;
>>  }
>>  
>> -if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
>> - sizeof(struct sev_user_data_pek_csr)))
>> +if (copy_to_user((void __user *)argp->data, , sizeof(input)))
>>  ret = -EFAULT;
>>  
>>  e_shutdown:
>>  if (do_shutdown)
>> -sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +
>>  e_free_blob:
>>  kfree(blob);
>>  e_free:
>> @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
>> ioctl, unsigned long arg)
>>  ret = sev_ioctl_pdh_gen();
>>  break;
>>  
>> -case SEV_PEK_CSR: {
>>

Re: crypto API - async semantics

2017-10-12 Thread Herbert Xu

On Thu, Oct 12, 2017 at 09:39:34AM +, Horia Geantă wrote:
>
> Taking ascii art from crypto API docs:
> 
> DATA ---.
> v
>  .init() -> .update() -> .final()  ! .update() might not be called
>  ^| |at all in this scenario.
>  '' '---> HASH
> 
> My question was referring to the case where multiple update() operations
> are issued in parallel for the same request object.
> For e.g. let's say a crypto API client wants to hash a 4GB file, and
> does this by issuing update() repeatedly and performing
> wait_for_completion() at the very end.

That is most certainly not legal.  Is dm-crypt doing that?

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: crypto API - async semantics

2017-10-12 Thread Horia Geantă

On 10/12/2017 9:44 AM, Herbert Xu wrote:
> On Wed, Oct 11, 2017 at 12:36:11PM +, Horia Geantă wrote:
>> Hi Herbert,
>>
>> I am evaluating whether ahash implementation in caam crypto driver
>> behaves correctly.
>> One thing I've noticed is that for each ahash tfm there is support for
>> at most two in-flight requests, and I would like to know whether this is
>> an issue or not.
> 
> What do you mean by in-flight requests? If you're talking about
> requests that have not been completed (i.e., in progress) then
> that's fine.  Any new requests coming in can just be queued.
> 
> If you're talking about requests that have not been finalised
> then that would be broken.  The API allows an arbitrary number
> of requests to ben present in an non-finalised state.  For example,
> given an IPsec SA, we may be processing any number of packets for
> it at once, with each coming from a different CPU.
> 
Taking ascii art from crypto API docs:

DATA ---.
v
 .init() -> .update() -> .final()  ! .update() might not be called
 ^| |at all in this scenario.
 '' '---> HASH

My question was referring to the case where multiple update() operations
are issued in parallel for the same request object.
For e.g. let's say a crypto API client wants to hash a 4GB file, and
does this by issuing update() repeatedly and performing
wait_for_completion() at the very end.

>> In this context, could you please clarify whether your previous answer:
>> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg04029.html
>>> If any async operation returns EINPROGRESS, the client must not
>>> proceed until that operation has completed.
>> refers only to ahash operations OR to any crypto API async operation?
>> IIUC this would be equivalent to saying that crypto API allows only for
>> one in-flight request per tfm, correct?
> 
> No that comment is referring to the request object.  The whole point
> of having a request object is to allow arbitrary number of hash
> operations in parallel given a single tfm.  The tfm is simply a
> representation of the key.  All hash state must be stored in the
> request.
> 
Got it, thanks.

Horia

Re: crypto API - async semantics

2017-10-12 Thread Horia Geantă

On 10/12/2017 12:49 PM, Herbert Xu wrote:
> On Thu, Oct 12, 2017 at 09:39:34AM +, Horia Geantă wrote:
>>
>> Taking ascii art from crypto API docs:
>>
>> DATA ---.
>> v
>>  .init() -> .update() -> .final()  ! .update() might not be called
>>  ^| |at all in this scenario.
>>  '' '---> HASH
>>
>> My question was referring to the case where multiple update() operations
>> are issued in parallel for the same request object.
>> For e.g. let's say a crypto API client wants to hash a 4GB file, and
>> does this by issuing update() repeatedly and performing
>> wait_for_completion() at the very end.
> 
> That is most certainly not legal.  Is dm-crypt doing that?
> 
dm-crypt is issuing requests (aead / skcipher) in parallel, however a
new request object is allocated each time, which seems legit.

My confusion was due to mixing parallelism at different levels:
-at request object level - issuing multiple requests for the same
request object (ahash update() case)
-at tfm object level - issuing multiple requests for different request
objects (dm-crypt case)

As you pointed out, the first is not legal, while the latter is
perfectly fine.

Thanks,
Horia

Re: [PATCH v5] crypto: s5p-sss: Add HASH support for Exynos

2017-10-12 Thread Vladimir Zapolskiy

Hello Kamil,

thank you for the change, please find below a number of minor
review comments.

On 10/09/2017 02:12 PM, Kamil Konieczny wrote:
> Add support for MD5, SHA1, SHA256 hash algorithms for Exynos HW.
> It uses the crypto framework asynchronous hash api.
> It is based on omap-sham.c driver.
> S5P has some HW differencies and is not implemented.
> 

[snip]

>  
>  /* Feed control registers */
>  #define SSS_REG_FCINTSTAT   0x
> +#define SSS_FCINTSTAT_HPARTINT   BIT(7)
> +#define SSS_FCINTSTAT_HDONEINT   BIT(5)

  ^
Please use the same style of whitespaces as it is found around.

Generally I do agree that the tabs are better here, please feel free
to send a preceding change, which changes the spacing to tabs, otherwise
use space symbols in this change.

>  #define SSS_FCINTSTAT_BRDMAINT  BIT(3)
>  #define SSS_FCINTSTAT_BTDMAINT  BIT(2)
>  #define SSS_FCINTSTAT_HRDMAINT  BIT(1)
>  #define SSS_FCINTSTAT_PKDMAINT  BIT(0)
>  
>  #define SSS_REG_FCINTENSET  0x0004
> +#define SSS_FCINTENSET_HPARTINTENSET BIT(7)
> +#define SSS_FCINTENSET_HDONEINTENSET BIT(5)

Same as above.

>  #define SSS_FCINTENSET_BRDMAINTENSETBIT(3)
>  #define SSS_FCINTENSET_BTDMAINTENSETBIT(2)
>  #define SSS_FCINTENSET_HRDMAINTENSETBIT(1)
>  #define SSS_FCINTENSET_PKDMAINTENSETBIT(0)
>  
>  #define SSS_REG_FCINTENCLR  0x0008
> +#define SSS_FCINTENCLR_HPARTINTENCLR BIT(7)
> +#define SSS_FCINTENCLR_HDONEINTENCLR BIT(5)

Same as above.

>  #define SSS_FCINTENCLR_BRDMAINTENCLRBIT(3)
>  #define SSS_FCINTENCLR_BTDMAINTENCLRBIT(2)
>  #define SSS_FCINTENCLR_HRDMAINTENCLRBIT(1)
>  #define SSS_FCINTENCLR_PKDMAINTENCLRBIT(0)
>  
>  #define SSS_REG_FCINTPEND   0x000C
> +#define SSS_FCINTPEND_HPARTINTP  BIT(7)
> +#define SSS_FCINTPEND_HDONEINTP  BIT(5)

Same as above.

>  #define SSS_FCINTPEND_BRDMAINTP BIT(3)
>  #define SSS_FCINTPEND_BTDMAINTP BIT(2)
>  #define SSS_FCINTPEND_HRDMAINTP BIT(1)
> @@ -72,6 +88,7 @@
>  #define SSS_HASHIN_INDEPENDENT  _SBF(0, 0x00)
>  #define SSS_HASHIN_CIPHER_INPUT _SBF(0, 0x01)
>  #define SSS_HASHIN_CIPHER_OUTPUT_SBF(0, 0x02)
> +#define SSS_HASHIN_MASK  _SBF(0, 0x03)

Same as above.

[snip]

>  struct s5p_aes_reqctx {
> @@ -195,6 +284,19 @@ struct s5p_aes_ctx {
>   *   protects against concurrent access to these fields.
>   * @lock:Lock for protecting both access to device hardware registers
>   *   and fields related to current request (including the busy 
> field).
> + * @res: Resources for hash.
> + * @io_hash_base: Per-variant offset for HASH block IO memory.
> + * @hash_lock:   Lock for protecting hash_req, hash_queue and hash_flags
> + *   variable.
> + * @hash_tasklet: New HASH request scheduling job.
> + * @xmit_buf:Buffer for current HASH request transfer into SSS block.
> + * @hash_flags:  Flags for current HASH op.
> + * @hash_queue:  Async hash queue.
> + * @hash_req:Current request sending to SSS HASH block.
> + * @hash_sg_iter: Scatterlist transferred through DMA into SSS HASH block.
> + * @hash_sg_cnt: Counter for hash_sg_iter.
> + *
> + * @use_hash:true if HASH algs enabled

You may want to reorder the lines to get the same order as in the struct.

>   */
>  struct s5p_aes_dev {
>   struct device   *dev;
> @@ -215,16 +317,88 @@ struct s5p_aes_dev {
>   struct crypto_queue queue;
>   boolbusy;
>   spinlock_t  lock;
> +
> + struct resource *res;
> + void __iomem*io_hash_base;
> +
> + spinlock_t  hash_lock; /* protect hash_ vars */
> + unsigned long   hash_flags;
> + struct crypto_queue hash_queue;
> + struct tasklet_struct   hash_tasklet;
> +
> + u8  xmit_buf[BUFLEN];
> + struct ahash_request*hash_req;
> + struct scatterlist  *hash_sg_iter;
> + int hash_sg_cnt;

Please change the type to 'unsigned int'.

> +
> + booluse_hash;
>  };
>  
> -static struct s5p_aes_dev *s5p_dev;
> +enum hash_op {
> + HASH_OP_UPDATE = 1,
> + HASH_OP_FINAL = 2
> +};

If this type is not going to be extended, then I'd rather suggest to
use a boolean correspondent field in the struct s5p_hash_reqctx instead
of a new introduced type.

> +
> +/**
> + * struct s5p_hash_reqctx - HASH request context
> + * @dev: Associated device
> + * @op:  Current request operation (OP_UPDATE or OP_FINAL)

See a comment above.

> + * @digcnt:  Number of bytes processed by HW (without buffer[] ones)
> + * @digest:  Digest message or IV for partial result
> + *

Crypto Fixes for 4.14

2017-10-12 Thread Herbert Xu

Hi Linus: 

This push fixes the following issues:

- Crashes in skcipher/shash from zero-length input.
- Fix softirq GFP_KERNEL allocation in shash_setkey_unaligned.
- Error path bug in xts create function.
- Compiler warning regressions in axis and stm32.


Please pull from

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git linus


Arnd Bergmann (2):
  crypto: axis - hide an unused variable
  crypto: stm32 - Try to fix hash padding

Christophe Jaillet (1):
  crypto: xts - Fix an error handling path in 'create()'

Herbert Xu (2):
  crypto: skcipher - Fix crash on zero-length input
  crypto: shash - Fix zero-length shash ahash digest crash

Jia-Ju Bai (1):
  crypto: shash - Fix a sleep-in-atomic bug in shash_setkey_unaligned

 crypto/shash.c   |   10 ++
 crypto/skcipher.c|   17 +++--
 crypto/xts.c |6 --
 drivers/crypto/axis/artpec6_crypto.c |4 ++--
 drivers/crypto/stm32/stm32-hash.c|   15 +--
 5 files changed, 32 insertions(+), 20 deletions(-)

Thanks,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[PATCH] staging/ccree: Declare compiled out functions static inline

2017-10-12 Thread Rishabh Hardas

Sparse was giving out a warning for symbols
'cc_set_ree_fips_status' and 'fips_handler'
that they were not declared and need to be
made static. This patch makes both the symbols
static inline, to remove the warnings.

Signed-off-by: Rishabh Hardas 
---
 drivers/staging/ccree/ssi_fips.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/ccree/ssi_fips.h b/drivers/staging/ccree/ssi_fips.h
index 369ddf9..63bcca7 100644
--- a/drivers/staging/ccree/ssi_fips.h
+++ b/drivers/staging/ccree/ssi_fips.h
@@ -40,8 +40,8 @@ static inline int ssi_fips_init(struct ssi_drvdata *p_drvdata)
 }
 
 static inline void ssi_fips_fini(struct ssi_drvdata *drvdata) {}
-void cc_set_ree_fips_status(struct ssi_drvdata *drvdata, bool ok) {}
-void fips_handler(struct ssi_drvdata *drvdata) {}
+static inline void cc_set_ree_fips_status(struct ssi_drvdata *drvdata, bool 
ok) {}
+static inline void fips_handler(struct ssi_drvdata *drvdata) {}
 
 #endif /* CONFIG_CRYPTO_FIPS */
 
-- 
1.9.1

Re: crypto API - async semantics

2017-10-12 Thread Herbert Xu

On Wed, Oct 11, 2017 at 12:36:11PM +, Horia Geantă wrote:
> Hi Herbert,
> 
> I am evaluating whether ahash implementation in caam crypto driver
> behaves correctly.
> One thing I've noticed is that for each ahash tfm there is support for
> at most two in-flight requests, and I would like to know whether this is
> an issue or not.

What do you mean by in-flight requests? If you're talking about
requests that have not been completed (i.e., in progress) then
that's fine.  Any new requests coming in can just be queued.

If you're talking about requests that have not been finalised
then that would be broken.  The API allows an arbitrary number
of requests to ben present in an non-finalised state.  For example,
given an IPsec SA, we may be processing any number of packets for
it at once, with each coming from a different CPU.

> In this context, could you please clarify whether your previous answer:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg04029.html
> > If any async operation returns EINPROGRESS, the client must not
> > proceed until that operation has completed.
> refers only to ahash operations OR to any crypto API async operation?
> IIUC this would be equivalent to saying that crypto API allows only for
> one in-flight request per tfm, correct?

No that comment is referring to the request object.  The whole point
of having a request object is to allow arbitrary number of hash
operations in parallel given a single tfm.  The tfm is simply a
representation of the key.  All hash state must be stored in the
request.

Cheers,
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Fostering linux community collaboration on hardware accelerators

2017-10-12 Thread Douglas Miller

Not sure if you're already plugged-in to this, but the OpenMP group is 
(has been) working on Accelerator support.


http://www.openmp.org/updates/openmp-accelerator-support-gpus/

Maybe you are talking about a different aspect of accelerator support, 
but it seems prudent to involve OpenMP as much as makes sense.



On 10/12/2017 12:22 AM, Andrew Donnellan wrote:

On 10/10/17 22:28, Jonathan Cameron wrote:

Hi All,

Please forward this email to anyone you think may be interested.


Have forwarded this to a number of relevant IBMers.

On behalf of Huawei, I am looking into options to foster a wider 
community
around the various ongoing projects related to Accelerator support 
within

Linux.  The particular area of interest to Huawei is that of harnessing
accelerators from userspace, but in a collaborative way with the kernel
still able to make efficient use of them, where appropriate.

We are keen to foster a wider community than one just focused on
our own current technology.  This is a field with no clear answers, 
so the

widest possible range of input is needed!

The address list of this email is drawn from people we have had 
discussions

with or who have been suggested in response to Kenneth Lee's wrapdrive
presentation at Linaro Connect and earlier presentations on the more 
general

issue. A few relevant lists added to hopefully catch anyone we missed.
My apologies to anyone who got swept up in this and isn't interested!

Here we are defining accelerators fairly broadly - suggestions for a 
better

term are also welcome.

The infrastructure may be appropriate for:
* Traditional offload engines - cryptography, compression and similar
* Upcoming AI accelerators
* ODP type requirements for access to elements of networking
* Systems utilizing SVM including CCIX and other cache coherent buses
* Many things we haven't thought of yet...

As I see it, there are several aspects to this:

1) Kernel drivers for accelerators themselves.
    * Traditional drivers such as crypto etc
- These already have their own communities. The main
   focus of such work will always be through them.
 - What a more general community could add here would be an
   overview of the shared infrastructure of such devices.
  This is particularly true around VFIO based (or similar)
  userspace interfaces with a non trivial userspace component.
    * How to support new types of accelerator?

2) The need for lightweight access paths from userspace that 'play 
well' and
    share resources etc with standard in-kernel drivers.  This is the 
area
    that Kenneth Lee and Huawei have been focusing on with their 
wrapdrive
    effort. We know there are other similar efforts going on in other 
companies.
    * This may involve interacting with existing kernel communities 
such as

  those around VFIO and mdev.
    * Resource management when we may have many consumers - not all 
hardware

  has appropriate features to deal with this.

3) Usecases for accelerators. e.g.
    * kTLS
    * Storage encryption
    * ODP - networking dataplane
    * AI toolkits

Discussions we want to get started include:
* A wider range of hardware than we are currently considering. What 
makes
   sense to target / what hardware do people have they would like to 
support?
* Upstream paths - potential blockers and how to overcome them. The 
standard
   kernel drivers should be fairly straightforward, but once we start 
looking at

   systems with a heavier userspace component, things will get more
   controversial!
* Fostering stronger userspace communities to allow these these 
accelerators

   to be easily harnessed.

So as ever with a linux community focusing on a particular topic, the
obvious solution is a mailing list. There are a number of options on how
do this.

1) Ask one of the industry bodies to host? Who?

2) Put together a compelling argument for 
linux-accelerat...@vger.kernel.org

as probably the most generic location for such a list.


Happy to offer linux-accelerat...@lists.ozlabs.org, which I can get 
set up immediately (and if we want patchwork, patchwork.ozlabs.org is 
available as always, no matter where the list is hosted).



More open questions are
1) Scope?
  * Would anyone ever use such an overarching list?
  * Are we better off with the usual adhoc list of 'interested 
parties' + lkml?
  * Do we actually need to define the full scope - are we better with 
a vague

    definition?


I think a list with a broad and vaguely defined scope is a good idea - 
it would certainly be helpful to us to be able to follow what other 
contributors are doing that could be relevant to our CAPI and OpenCAPI 
work.




2) Is there an existing community we can use to discuss these issues?
    (beyond the obvious firehose of LKML).

3) Who else to approach for input on these general questions?

In parallel to this there are elements such as git / patchwork etc but
they can all be done as they are needed.

Thanks

--

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

2017-10-12 Thread Borislav Petkov

On Wed, Oct 11, 2017 at 11:50:30AM -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 
> ---
> Make it as a second patch in the series (changes from 12.1 -> 12.2)
> 
> Changes since v5.1:
>  * text streamlining (from Boris)
>  * rename sev_handle_cmd -> sev_do_cmd (from Boris)
>  * PSP_P2CMSG needs arg eval (from Boris)
>  * use #ifdef instead of #if defined() (from Boris)
> 
>  drivers/crypto/ccp/psp-dev.c | 251 
> +++
>  drivers/crypto/ccp/psp-dev.h |  16 +++
>  include/linux/psp-sev.h  | 159 +++
>  3 files changed, 426 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index b5789f878560..175cb3c3b8ef 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -23,9 +23,16 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  #include "sp-dev.h"
>  #include "psp-dev.h"
>  
> +#define DEVICE_NAME  "sev"
> +
> +static DEFINE_MUTEX(sev_cmd_mutex);
> +static bool sev_fops_registered;

Well, if you're going to have a global var, why not pull up the misc
device instead?

And mind you, I've moved out this assignments:

+   psp->sev_misc = psp_misc_dev;
+   init_waitqueue_head(>sev_int_queue);
+   dev_info(dev, "registered SEV device\n");

outside of the if-conditional as I'm assuming you want to do this for
each psp device for which sev_ops_init() is called.

Or am I wrong here?

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index 175cb3c3b8ef..d50aaa1ca75b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -31,7 +31,7 @@
 #define DEVICE_NAME"sev"
 
 static DEFINE_MUTEX(sev_cmd_mutex);
-static bool sev_fops_registered;
+static struct miscdevice *psp_misc_dev;
 
 static struct psp_device *psp_alloc_struct(struct sp_device *sp)
 {
@@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush);
 static int sev_ops_init(struct psp_device *psp)
 {
struct device *dev = psp->dev;
-   struct miscdevice *misc;
int ret;
 
/*
@@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp)
 * sev_do_cmd() finds the right master device to which to issue the
 * command to the firmware.
 */
-   if (!sev_fops_registered) {
-
-   misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
-   if (!misc)
+   if (!psp_misc_dev) {
+   psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), 
GFP_KERNEL);
+   if (!psp_misc_dev)
return -ENOMEM;
 
-   misc->minor = MISC_DYNAMIC_MINOR;
-   misc->name = DEVICE_NAME;
-   misc->fops = _fops;
+   psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
+   psp_misc_dev->name = DEVICE_NAME;
+   psp_misc_dev->fops = _fops;
 
-   ret = misc_register(misc);
+   ret = misc_register(psp_misc_dev);
if (ret)
return ret;
-
-   sev_fops_registered = true;
-   psp->sev_misc = misc;
-   init_waitqueue_head(>sev_int_queue);
-   dev_info(dev, "registered SEV device\n");
}
 
+   psp->sev_misc = psp_misc_dev;
+   init_waitqueue_head(>sev_int_queue);
+   dev_info(dev, "registered SEV device\n");
+
return 0;
 }
 
@@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp)
 
 static void sev_exit(struct psp_device *psp)
 {
-   if (psp->sev_misc)
-   misc_deregister(psp->sev_misc);
+   if (psp_misc_dev)
+   misc_deregister(psp_misc_dev);
 }
 
 int psp_dev_init(struct sp_device *sp)

-- 
Regards/Gruss,
Boris.

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

Re: [Part2 PATCH v5.2 12.3/31] crypto: ccp: Implement SEV_FACTORY_RESET ioctl command

2017-10-12 Thread Borislav Petkov

On Wed, Oct 11, 2017 at 11:55:21AM -0500, Brijesh Singh wrote:
> The SEV_FACTORY_RESET command can be used by the platform owner to
> reset the non-volatile SEV related data. The command is defined in
> SEV spec section 5.4
> 
> 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 
> ---
> 
> Changes since v5.1:
>  * rename sev_handle_cmd -> sev_do_cmd (from Boris)
>  * skip copy_to_user when invalid cmd id is passed (from Boris)
>  * use SEV_MAX instead of SEV_CMD_MAX to check for invalid command
> 
>  drivers/crypto/ccp/psp-dev.c | 29 -
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 175cb3c3b8ef..a9c885a39910 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -179,7 +179,34 @@ static int sev_do_cmd(int cmd, void *data, int *psp_ret)
>  
>  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long 
> arg)
>  {
> - return -ENOTTY;
> + void __user *argp = (void __user *)arg;
> + struct sev_issue_cmd input;
> + int ret = -EFAULT;
> +
> + if (ioctl != SEV_ISSUE_CMD)
> + return -EINVAL;
> +
> + if (copy_from_user(, argp, sizeof(struct sev_issue_cmd)))
> + return -EFAULT;
> +
> + if (input.cmd > SEV_MAX)
> + return -EINVAL;
> +
> + switch (input.cmd) {
> +
> + case SEV_FACTORY_RESET: {
> + ret = sev_do_cmd(SEV_CMD_FACTORY_RESET, 0, );
> + break;
> + }

Those curly braces are still here. Just use my diff I sent you by saving
the mail to a file and doing

$ patch -p1 --dry-run -i /tmp/boris.mail

ontop of this patch. If it applies ok, remove the --dry-run.

Then you can do changes ontop. This way you won't miss changes I've sent
you.

Thx.

-- 
Regards/Gruss,
Boris.

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

Re: [Part2 PATCH v5.2 12.1/31] crypto: ccp: Define SEV userspace ioctl and command id

2017-10-12 Thread Brijesh Singh




On 10/12/2017 08:27 AM, Borislav Petkov wrote:
...




+/**
+ * struct sev_user_data_status - PLATFORM_STATUS command parameters
+ *
+ * @major: major API version
+ * @minor: minor API version
+ * @state: platform state
+ * @owner: self-owned or externally owned
+ * @config: platform config flags
+ * @build: firmware build id for API version
+ * @guest_count: number of active guests
+ */
+struct sev_user_data_status {
+   __u8 api_major; /* Out */
+   __u8 api_minor; /* Out */
+   __u8 state; /* Out */
+   __u8 owner; /* Out */
+   __u32 config;   /* Out */
+   __u8 build; /* Out */
+   __u32 guest_count;  /* Out */
+} __packed;
+


After yesterday's discussion about the sev_data_status layout, that
struct is not needed anymore, right?



Yes, we no longer need that structure.

-Brijesh

Re: [PATCH 0/3] crypto: marvell - Remove the old CESA driver

2017-10-12 Thread Gregory CLEMENT

Hi Boris,
 
 On mer., oct. 11 2017, Boris Brezillon  
wrote:

> Hello,
>
> It's been several releases since we added a new driver to support the
> CESA IP (the new driver was introduced in 4.2). It seems most major
> bugs have been discovered and fixed and now is probably a good time to
> kill the old driver and force remaining users to switch to the new one.
>
> This series first add a platform_device_id table to the marvell CESA
> driver so that it can work on non-DT platforms. Then we patch all
> defconfigs to select the new driver instead of the old. And finally we
> remove the old driver.
> These patches have to be applied in this order to make things
> bisectable, so, if this series is accepted it will have to go through
> a single tree (either ARM or crypto).

For the whole series
Acked-by: Gregory CLEMENT 

And I am OK for this series going through crypto because I don't expect
any changes in the mvebu files so there won't be any merge conflict. But
I am also fine to take the series in my tree, it is up to Herbert Xu.

Thanks,

Gregory


>
> Regards,
>
> Boris
>
> Boris Brezillon (3):
>   crypto: marvell - Add a platform_device_id table
>   ARM: configs: Stop selecting the old CESA driver
>   crypto: marvell - Remove the old mv_cesa driver
>
>  arch/arm/configs/dove_defconfig |2 +-
>  arch/arm/configs/multi_v5_defconfig |2 +-
>  arch/arm/configs/orion5x_defconfig  |2 +-
>  drivers/crypto/Kconfig  |   22 +-
>  drivers/crypto/Makefile |1 -
>  drivers/crypto/marvell/cesa.c   |   13 +-
>  drivers/crypto/mv_cesa.c| 1216 
> ---
>  drivers/crypto/mv_cesa.h|  150 -
>  8 files changed, 12 insertions(+), 1396 deletions(-)
>  delete mode 100644 drivers/crypto/mv_cesa.c
>  delete mode 100644 drivers/crypto/mv_cesa.h
>
> -- 
> 2.11.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH] chacha20-ssse3/avx2: satisfy stack validation 2.0

2017-10-12 Thread Herbert Xu

On Sun, Oct 08, 2017 at 10:50:53PM +0200, Jason A. Donenfeld wrote:
> The new stack validator in objdump doesn't like directly assigning r11
> to rsp, warning with something like:
> 
> warning: objtool: chacha20_4block_xor_ssse3()+0xa: unsupported stack pointer 
> realignment
> warning: objtool: chacha20_8block_xor_avx2()+0x6: unsupported stack pointer 
> realignment
> 
> This fixes things up to use code similar to gcc's DRAP register, so that
> objdump remains happy.
> 
> Signed-off-by: Jason A. Donenfeld 
> Fixes: baa41469a7b9 ("objtool: Implement stack validation 2.0")

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 01/13] crypto: crypto4xx: wire up hmac_mc to hmac_muting

2017-10-12 Thread Herbert Xu

On Wed, Oct 04, 2017 at 01:00:05AM +0200, Christian Lamparter wrote:
> The hmac_mc parameter of set_dynamic_sa_command_1()
> was defined but not used. On closer inspection it
> turns out, it was never wired up.
> 
> Signed-off-by: Christian Lamparter 

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: ecdh_helper - return unsigned value for crypto_ecdh_key_len()

2017-10-12 Thread Herbert Xu

On Fri, Sep 29, 2017 at 12:13:08PM +0300, Tudor Ambarus wrote:
> ECDH_KPP_SECRET_MIN_SIZE and params->key_size are both returning
> unsigned values.
> 
> Signed-off-by: Tudor Ambarus 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 1/3] crypto: dh_helper - return unsigned int for dh_data_size()

2017-10-12 Thread Herbert Xu

On Fri, Sep 29, 2017 at 12:21:04PM +0300, Tudor Ambarus wrote:
> p->key_size, p->p_size, p->g_size are all of unsigned int type.
> 
> Signed-off-by: Tudor Ambarus 

Patch applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH 00/18] crypto: talitos - fixes and performance improvement

2017-10-12 Thread Herbert Xu

On Fri, Oct 06, 2017 at 03:04:31PM +0200, Christophe Leroy wrote:
> This serie fixes and improves the talitos crypto driver.
> 
> First 6 patchs are fixes of failures reported by the new tests in the
> kernel crypto test manager.
> 
> The 8 following patches are cleanups and simplifications.
> 
> The last 4 ones are performance improvement. The main improvement is
> in the one before the last, it divides by 2 the time needed for a md5
> hash on the SEC1.
> 
> Christophe Leroy (18):
>   crypto: talitos - fix AEAD test failures
>   crypto: talitos - fix memory corruption on SEC2
>   crypto: talitos - fix setkey to check key weakness
>   crypto: talitos - fix AEAD for sha224 on non sha224 capable chips
>   crypto: talitos - fix use of sg_link_tbl_len
>   crypto: talitos - fix ctr-aes-talitos
>   crypto: talitos - zeroize the descriptor with memset()
>   crypto: talitos - declare local functions static
>   crypto: talitos - use devm_kmalloc()
>   crypto: talitos - use of_property_read_u32()
>   crypto: talitos - use devm_ioremap()
>   crypto: talitos - don't check the number of channels at each interrupt
>   crypto: talitos - remove to_talitos_ptr_len()
>   crypto: talitos - simplify tests in ipsec_esp()
>   crypto: talitos - DMA map key in setkey()
>   crypto: talitos - do hw_context DMA mapping outside the requests
>   crypto: talitos - chain in buffered data for ahash on SEC1
>   crypto: talitos - avoid useless copy

All applied.  Thanks.
-- 
Email: Herbert Xu 
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: Fostering linux community collaboration on hardware accelerators

2017-10-12 Thread Francois Ozog

On 12 October 2017 at 16:57, Jonathan Cameron
 wrote:
> On Thu, 12 Oct 2017 08:31:36 -0500
> Douglas Miller  wrote:
>
>> Not sure if you're already plugged-in to this, but the OpenMP group is
>> (has been) working on Accelerator support.
>>
>> http://www.openmp.org/updates/openmp-accelerator-support-gpus/
>>
>> Maybe you are talking about a different aspect of accelerator support,
>> but it seems prudent to involve OpenMP as much as makes sense.
>
> That's certainly interesting and sits in the area of 'standard'
> userspace code but it is (I think) really addressing only one aspect
> of the wider support problem.
>
> I do like the emphasis on balancing between CPU and accelerator,
> that is certainly an open question even a the lowest levels in
> areas such as cryptography acceleration where you either run
> out of hardware resources on your accelerator or you actually
> have a usage pattern that would be quicker on the CPU due
> to inherent overheads in (current) non cpu crypto engines.
>
> Thanks for the pointer.  I can see we are going to need some location
> for resources like this to be gathered together.
>
> Jonathan
>
>>
>>
>> On 10/12/2017 12:22 AM, Andrew Donnellan wrote:
>> > On 10/10/17 22:28, Jonathan Cameron wrote:
>> >> Hi All,
>> >>
>> >> Please forward this email to anyone you think may be interested.
>> >
>> > Have forwarded this to a number of relevant IBMers.
>> >
>> >> On behalf of Huawei, I am looking into options to foster a wider
>> >> community
>> >> around the various ongoing projects related to Accelerator support
>> >> within
>> >> Linux.  The particular area of interest to Huawei is that of harnessing
>> >> accelerators from userspace, but in a collaborative way with the kernel
>> >> still able to make efficient use of them, where appropriate.
>> >>
>> >> We are keen to foster a wider community than one just focused on
>> >> our own current technology.  This is a field with no clear answers,
>> >> so the
>> >> widest possible range of input is needed!
>> >>
>> >> The address list of this email is drawn from people we have had
>> >> discussions
>> >> with or who have been suggested in response to Kenneth Lee's wrapdrive
>> >> presentation at Linaro Connect and earlier presentations on the more
>> >> general
>> >> issue. A few relevant lists added to hopefully catch anyone we missed.
>> >> My apologies to anyone who got swept up in this and isn't interested!
>> >>
>> >> Here we are defining accelerators fairly broadly - suggestions for a
>> >> better
>> >> term are also welcome.
>> >>
>> >> The infrastructure may be appropriate for:
>> >> * Traditional offload engines - cryptography, compression and similar
>> >> * Upcoming AI accelerators
>> >> * ODP type requirements for access to elements of networking
>> >> * Systems utilizing SVM including CCIX and other cache coherent buses
>> >> * Many things we haven't thought of yet...
>> >>
>> >> As I see it, there are several aspects to this:
>> >>
>> >> 1) Kernel drivers for accelerators themselves.
>> >> * Traditional drivers such as crypto etc
>> >> - These already have their own communities. The main
>> >>focus of such work will always be through them.
>> >>  - What a more general community could add here would be an
>> >>overview of the shared infrastructure of such devices.
>> >>   This is particularly true around VFIO based (or similar)
>> >>   userspace interfaces with a non trivial userspace component.
>> >> * How to support new types of accelerator?
>> >>
>> >> 2) The need for lightweight access paths from userspace that 'play
>> >> well' and
>> >> share resources etc with standard in-kernel drivers.  This is the
>> >> area
>> >> that Kenneth Lee and Huawei have been focusing on with their
>> >> wrapdrive
>> >> effort. We know there are other similar efforts going on in other
>> >> companies.
>> >> * This may involve interacting with existing kernel communities
>> >> such as
>> >>   those around VFIO and mdev.
>> >> * Resource management when we may have many consumers - not all
>> >> hardware
>> >>   has appropriate features to deal with this.
>> >>
>> >> 3) Usecases for accelerators. e.g.
>> >> * kTLS
>> >> * Storage encryption
>> >> * ODP - networking dataplane
>> >> * AI toolkits
>> >>
>> >> Discussions we want to get started include:
>> >> * A wider range of hardware than we are currently considering. What
>> >> makes
>> >>sense to target / what hardware do people have they would like to
>> >> support?
>> >> * Upstream paths - potential blockers and how to overcome them. The
>> >> standard
>> >>kernel drivers should be fairly straightforward, but once we start
>> >> looking at
>> >>systems with a heavier userspace component, things will get more
>> >>controversial!
>> >> * Fostering stronger userspace communities to allow these these
>> >> accelerators

[PATCH] crypto: cavium: clean up clang warning on unread variable offset

2017-10-12 Thread Colin King

From: Colin Ian King 

The variable offset is being assigned and not being used; it should
be passed as the 2nd argument to call to function nitrox_write_csr
but has been omitted. Fix this.

Cleans up clang warning: Value stored to 'offset' is never read

Fixes: 14fa93cdcd9b ("crypto: cavium - Add support for CNN55XX adapters.")
Signed-off-by: Colin Ian King 
---
 drivers/crypto/cavium/nitrox/nitrox_hal.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/cavium/nitrox/nitrox_hal.c 
b/drivers/crypto/cavium/nitrox/nitrox_hal.c
index f0655f82fa7d..73275ce6a668 100644
--- a/drivers/crypto/cavium/nitrox/nitrox_hal.c
+++ b/drivers/crypto/cavium/nitrox/nitrox_hal.c
@@ -126,7 +126,7 @@ void nitrox_config_pkt_input_rings(struct nitrox_device 
*ndev)
 * size and interrupt threshold.
 */
offset = NPS_PKT_IN_INSTR_BADDRX(i);
-   nitrox_write_csr(ndev, NPS_PKT_IN_INSTR_BADDRX(i), cmdq->dma);
+   nitrox_write_csr(ndev, offset, cmdq->dma);
 
/* configure ring size */
offset = NPS_PKT_IN_INSTR_RSIZEX(i);
-- 
2.14.1

[PATCH] crypto: ccp: remove unused variable qim

2017-10-12 Thread Colin King

From: Colin Ian King 

Variable qim is assigned but never read, it is redundant and can
be removed.

Cleans up clang warning: Value stored to 'qim' is never read

Fixes: 4b394a232df7 ("crypto: ccp - Let a v5 CCP provide the same function as 
v3")
Signed-off-by: Colin Ian King 
---
 drivers/crypto/ccp/ccp-dev-v5.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/crypto/ccp/ccp-dev-v5.c b/drivers/crypto/ccp/ccp-dev-v5.c
index 65604fc65e8f..44a4d2779b15 100644
--- a/drivers/crypto/ccp/ccp-dev-v5.c
+++ b/drivers/crypto/ccp/ccp-dev-v5.c
@@ -788,13 +788,12 @@ static int ccp5_init(struct ccp_device *ccp)
struct ccp_cmd_queue *cmd_q;
struct dma_pool *dma_pool;
char dma_pool_name[MAX_DMAPOOL_NAME_LEN];
-   unsigned int qmr, qim, i;
+   unsigned int qmr, i;
u64 status;
u32 status_lo, status_hi;
int ret;
 
/* Find available queues */
-   qim = 0;
qmr = ioread32(ccp->io_regs + Q_MASK_REG);
for (i = 0; i < MAX_HW_QUEUES; i++) {
 
-- 
2.14.1

[PATCH] crypto: qat: remove unused and redundant pointer vf_info

2017-10-12 Thread Colin King

From: Colin Ian King 

The pointer vf_info is being assigned but never read, it is redundant
and therefore can be removed.

Cleans up clang warning: Value stored to 'vf_info' is never read

Fixes: ed8ccaef52fa ("crypto: qat - Add support for SRIOV")
Signed-off-by: Colin Ian King 
---
 drivers/crypto/qat/qat_common/adf_dev_mgr.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_dev_mgr.c 
b/drivers/crypto/qat/qat_common/adf_dev_mgr.c
index 8afac52677a6..2d06409bd3c4 100644
--- a/drivers/crypto/qat/qat_common/adf_dev_mgr.c
+++ b/drivers/crypto/qat/qat_common/adf_dev_mgr.c
@@ -228,11 +228,8 @@ int adf_devmgr_add_dev(struct adf_accel_dev *accel_dev,
list_add_tail(>list, _table);
} else if (accel_dev->is_vf && pf) {
/* VF on host */
-   struct adf_accel_vf_info *vf_info;
struct vf_id_map *map;
 
-   vf_info = pf->pf.vf_info + adf_get_vf_id(accel_dev);
-
map = adf_find_vf(adf_get_vf_num(accel_dev));
if (map) {
struct vf_id_map *next;
-- 
2.14.1

Re: Fostering linux community collaboration on hardware accelerators

2017-10-12 Thread Douglas Miller


On 10/12/2017 10:48 AM, Francois Ozog wrote:

On 12 October 2017 at 16:57, Jonathan Cameron
 wrote:

On Thu, 12 Oct 2017 08:31:36 -0500
Douglas Miller  wrote:


Not sure if you're already plugged-in to this, but the OpenMP group is
(has been) working on Accelerator support.

http://www.openmp.org/updates/openmp-accelerator-support-gpus/

Maybe you are talking about a different aspect of accelerator support,
but it seems prudent to involve OpenMP as much as makes sense.

That's certainly interesting and sits in the area of 'standard'
userspace code but it is (I think) really addressing only one aspect
of the wider support problem.

I do like the emphasis on balancing between CPU and accelerator,
that is certainly an open question even a the lowest levels in
areas such as cryptography acceleration where you either run
out of hardware resources on your accelerator or you actually
have a usage pattern that would be quicker on the CPU due
to inherent overheads in (current) non cpu crypto engines.

Thanks for the pointer.  I can see we are going to need some location
for resources like this to be gathered together.

Jonathan



On 10/12/2017 12:22 AM, Andrew Donnellan wrote:

On 10/10/17 22:28, Jonathan Cameron wrote:

Hi All,

Please forward this email to anyone you think may be interested.

Have forwarded this to a number of relevant IBMers.


On behalf of Huawei, I am looking into options to foster a wider
community
around the various ongoing projects related to Accelerator support
within
Linux.  The particular area of interest to Huawei is that of harnessing
accelerators from userspace, but in a collaborative way with the kernel
still able to make efficient use of them, where appropriate.

We are keen to foster a wider community than one just focused on
our own current technology.  This is a field with no clear answers,
so the
widest possible range of input is needed!

The address list of this email is drawn from people we have had
discussions
with or who have been suggested in response to Kenneth Lee's wrapdrive
presentation at Linaro Connect and earlier presentations on the more
general
issue. A few relevant lists added to hopefully catch anyone we missed.
My apologies to anyone who got swept up in this and isn't interested!

Here we are defining accelerators fairly broadly - suggestions for a
better
term are also welcome.

The infrastructure may be appropriate for:
* Traditional offload engines - cryptography, compression and similar
* Upcoming AI accelerators
* ODP type requirements for access to elements of networking
* Systems utilizing SVM including CCIX and other cache coherent buses
* Many things we haven't thought of yet...

As I see it, there are several aspects to this:

1) Kernel drivers for accelerators themselves.
 * Traditional drivers such as crypto etc
 - These already have their own communities. The main
focus of such work will always be through them.
  - What a more general community could add here would be an
overview of the shared infrastructure of such devices.
   This is particularly true around VFIO based (or similar)
   userspace interfaces with a non trivial userspace component.
 * How to support new types of accelerator?

2) The need for lightweight access paths from userspace that 'play
well' and
 share resources etc with standard in-kernel drivers.  This is the
area
 that Kenneth Lee and Huawei have been focusing on with their
wrapdrive
 effort. We know there are other similar efforts going on in other
companies.
 * This may involve interacting with existing kernel communities
such as
   those around VFIO and mdev.
 * Resource management when we may have many consumers - not all
hardware
   has appropriate features to deal with this.

3) Usecases for accelerators. e.g.
 * kTLS
 * Storage encryption
 * ODP - networking dataplane
 * AI toolkits

Discussions we want to get started include:
* A wider range of hardware than we are currently considering. What
makes
sense to target / what hardware do people have they would like to
support?
* Upstream paths - potential blockers and how to overcome them. The
standard
kernel drivers should be fairly straightforward, but once we start
looking at
systems with a heavier userspace component, things will get more
controversial!
* Fostering stronger userspace communities to allow these these
accelerators
to be easily harnessed.

So as ever with a linux community focusing on a particular topic, the
obvious solution is a mailing list. There are a number of options on how
do this.

1) Ask one of the industry bodies to host? Who?

2) Put together a compelling argument for
linux-accelerat...@vger.kernel.org
as probably the most generic location for such a list.

Happy to offer linux-accelerat...@lists.ozlabs.org, which I can get
set up immediately (and if we want patchwork,

Re: [PATCH v3 0/4] crypto: Add driver for JZ4780 PRNG

2017-10-12 Thread PrasannaKumar Muralidharan

Hi Herbert,

On 12 October 2017 at 20:00, Herbert Xu  wrote:
> On Mon, Sep 18, 2017 at 07:32:37PM +0530, PrasannaKumar Muralidharan wrote:
>> This patch series adds support of pseudo random number generator found
>> in Ingenic's JZ4780 and X1000 SoC.
>>
>> Create cgublock node which has CGU and RNG node as its children. The
>> cgublock node uses "simple-bus" compatible which helps in exposing CGU
>> and RNG nodes without changing CGU driver. Add 'syscon' compatible in
>> CGU node in jz4780.dtsi. The jz4780-rng driver uses regmap exposed via
>> syscon interface to access the RNG registers. CGU driver is not
>> modified in this patch set as registers used by CGU driver and this
>> driver are different.
>>
>> PrasannaKumar Muralidharan (4):
>>   crypto: jz4780-rng: Add JZ4780 PRNG devicetree binding documentation
>>   crypto: jz4780-rng: Add Ingenic JZ4780 hardware PRNG driver
>>   crypto: jz4780-rng: Add RNG node to jz4780.dtsi
>>   crypto: jz4780-rng: Enable PRNG support in CI20 defconfig
>
> Please indicate which patches are intended to go through the crypto
> trees.

>From https://patchwork.linux-mips.org/patch/17162/ I expect the same.
Either all patches go via crypto tree or via mips tree.
The dtsi changes is not yet acked by MIPS / JZ4780 maintainer. Let's
wait for it.

Thanks,
PrasannaKumar

Re: [Nouveau] [PATCH 03/10] driver:gpu: return -ENOMEM on allocation failure.

2017-10-12 Thread Daniel Vetter

On Wed, Sep 13, 2017 at 01:02:12PM +0530, Allen Pais wrote:
> Signed-off-by: Allen Pais 

Applied to drm-misc-next, thanks.
-Daniel

> ---
>  drivers/gpu/drm/gma500/mid_bios.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/gma500/mid_bios.c 
> b/drivers/gpu/drm/gma500/mid_bios.c
> index d75ecb3..1fa1633 100644
> --- a/drivers/gpu/drm/gma500/mid_bios.c
> +++ b/drivers/gpu/drm/gma500/mid_bios.c
> @@ -237,7 +237,7 @@ static int mid_get_vbt_data_r10(struct drm_psb_private 
> *dev_priv, u32 addr)
>  
>   gct = kmalloc(sizeof(*gct) * vbt.panel_count, GFP_KERNEL);
>   if (!gct)
> - return -1;
> + return -ENOMEM;
>  
>   gct_virtual = ioremap(addr + sizeof(vbt),
>   sizeof(*gct) * vbt.panel_count);
> -- 
> 2.7.4
> 
> ___
> Nouveau mailing list
> nouv...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command

2017-10-12 Thread Borislav Petkov

On Fri, Oct 06, 2017 at 08:06:03PM -0500, Brijesh Singh wrote:
> The SEV_PEK_GEN command is used to generate a new Platform Endorsement
> Key (PEK). The command is defined in SEV spec section 5.6.
> 
> 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
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/psp-dev.c | 68 
> 
>  1 file changed, 68 insertions(+)

Just small fixups. Worth noting is this:

if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);

I think we want to return the result of the *last* command
executed, which, in the do_shutdown=1 case is SEV_CMD_SHUTDOWN, not
SEV_CMD_PEK_GEN.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index d02f48e356e8..31045ea7e798 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -211,7 +211,7 @@ static int sev_platform_get_state(int *state, int *error)
if (!data)
return -ENOMEM;
 
-   ret = sev_handle_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
+   ret = sev_do_cmd(SEV_CMD_PLATFORM_STATUS, data, error);
if (!ret)
*state = data->state;
 
@@ -228,7 +228,7 @@ static int sev_firmware_init(int *error)
if (!data)
return -ENOMEM;
 
-   rc = sev_handle_cmd(SEV_CMD_INIT, data, error);
+   rc = sev_do_cmd(SEV_CMD_INIT, data, error);
 
kfree(data);
return rc;
@@ -240,8 +240,8 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
int ret, state;
 
/*
-* PEK_GEN command can be issued only when firmware is in INIT state.
-* If firmware is in UNINIT state then we transition it in INIT state
+* The PEK_GEN command can be issued only when the firmware is in INIT
+* state. If it is in UNINIT state then we transition it in INIT state
 * and issue the command.
 */
ret = sev_platform_get_state(, >error);
@@ -258,10 +258,10 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_GEN, 0, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_GEN, 0, >error);
 
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
 
return ret;
 }
@@ -291,10 +291,10 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_ioctl_do_platform_status();
break;
 
-   case SEV_PEK_GEN: {
+   case SEV_PEK_GEN:
ret = sev_ioctl_pek_gen();
break;
-   }
+
default:
ret = -EINVAL;
goto out;

-- 
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.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command

2017-10-12 Thread Borislav Petkov

On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote:
> The SEV_PDH_GEN command is used to re-generate the Platform
> Diffie-Hellman (PDH) key. The command is defined in SEV spec section
> 5.9.
> 
> 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
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/psp-dev.c | 32 
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 03d7bd03ad58..28efb7a9245a 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
>   return ret;
>  }
>  
> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
> +{
> + int ret, state, do_shutdown = 0;
> +
> + /*
> +  * PDH_GEN command can be issued when platform is in INIT or WORKING
> +  * state. If we are in UNINIT state then transition in INIT state
> +  * before issuing the command.
> +  */
> + ret = sev_platform_get_state(, >error);
> + if (ret)
> + return ret;
> +

Why isn't this function doing:

if (state == SEV_STATE_WORKING) {
return -EBUSY;

like the PEK_GEN one?

Because if so, you can convert it and the PEK_GEN one into a single
function doing the work and wrappers handing in the command to avoid the
code duplication.

> + if (state == SEV_STATE_UNINIT) {
> + ret = sev_firmware_init(>error);
> + if (ret)
> + return ret;
> + do_shutdown = 1;
> + }
> +
> + ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, >error);
> +
> + if (do_shutdown)
> + sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +
> + return ret;
> +}
> +
>  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long 
> arg)
>  {
>   void __user *argp = (void __user *)arg;
> @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int 
> ioctl, unsigned long arg)
>   ret = sev_ioctl_pek_gen();
>   break;
>   }
> + case SEV_PDH_GEN: {
> + ret = sev_ioctl_pdh_gen();
> + break;
> + }

And those curly braces can go, as before.

-- 
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.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-12 Thread Borislav Petkov

On Fri, Oct 06, 2017 at 08:06:05PM -0500, Brijesh Singh wrote:
> The SEV_PEK_CSR command can be used to generate a PEK certificate
> signing request. The command is defined in SEV spec section 5.7.
> 
> 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
> Signed-off-by: Brijesh Singh 
> ---
>  drivers/crypto/ccp/psp-dev.c | 85 
> 
>  1 file changed, 85 insertions(+)

Ok, a couple of things here:

* Move the checks first and the allocations second so that you allocate
memory only after all checks have been passed and you don't allocate
pointlessly.

* That:

if (state == SEV_STATE_WORKING) {
ret = -EBUSY;
goto e_free_blob;
} else if (state == SEV_STATE_UNINIT) {
ret = sev_firmware_init(>error);
if (ret)
goto e_free_blob;
do_shutdown = 1;
}

is a repeating pattern. Perhaps it should be called
sev_firmware_reinit() and called by other functions.

* The rest is simplifications and streamlining.

---
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index e3ee68afd068..d41f5448a25b 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
int ret, state;
void *blob;
 
-   if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
-  sizeof(struct sev_user_data_pek_csr)))
+   if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
+   return -EFAULT;
+
+   if (!input.address)
+   return -EINVAL;
+
+   /* allocate a physically contiguous buffer to store the CSR blob */
+   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
+   input.length > SEV_FW_BLOB_MAX_SIZE)
return -EFAULT;
 
data = kzalloc(sizeof(*data), GFP_KERNEL);
if (!data)
return -ENOMEM;
 
-   /* allocate a temporary physical contigous buffer to store the CSR blob 
*/
-   blob = NULL;
-   if (input.address) {
-   if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
-   input.length > SEV_FW_BLOB_MAX_SIZE) {
-   ret = -EFAULT;
-   goto e_free;
-   }
-
-   blob = kmalloc(input.length, GFP_KERNEL);
-   if (!blob) {
-   ret = -ENOMEM;
-   goto e_free;
-   }
-
-   data->address = __psp_pa(blob);
-   data->len = input.length;
+   blob = kmalloc(input.length, GFP_KERNEL);
+   if (!blob) {
+   ret = -ENOMEM;
+   goto e_free;
}
 
+   data->address = __psp_pa(blob);
+   data->len = input.length;
+
ret = sev_platform_get_state(, >error);
if (ret)
goto e_free_blob;
@@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
do_shutdown = 1;
}
 
-   ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
+   ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
 
input.length = data->len;
 
/* copy blob to userspace */
-   if (blob &&
-   copy_to_user((void __user *)(uintptr_t)input.address,
-   blob, input.length)) {
+   if (copy_to_user((void __user *)input.address, blob, input.length)) {
ret = -EFAULT;
goto e_shutdown;
}
 
-   if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
-sizeof(struct sev_user_data_pek_csr)))
+   if (copy_to_user((void __user *)argp->data, , sizeof(input)))
ret = -EFAULT;
 
 e_shutdown:
if (do_shutdown)
-   sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+   ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
+
 e_free_blob:
kfree(blob);
 e_free:
@@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
ioctl, unsigned long arg)
ret = sev_ioctl_pdh_gen();
break;
 
-   case SEV_PEK_CSR: {
+   case SEV_PEK_CSR:
ret = sev_ioctl_pek_csr();
break;
-   }
+
default:
ret = -EINVAL;
goto out;

-- 
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.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command

2017-10-12 Thread Brijesh Singh



On 10/12/17 1:48 PM, Borislav Petkov wrote:
...
> On Fri, Oct 06, 2017 at 08:06:04PM -0500, Brijesh Singh wrote:
>> The SEV_PDH_GEN command is used to re-generate the Platform
>> Diffie-Hellman (PDH) key. The command is defined in SEV spec section
>> 5.9.
>>
>> 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
>> Signed-off-by: Brijesh Singh 
>> ---
>>  drivers/crypto/ccp/psp-dev.c | 32 
>>  1 file changed, 32 insertions(+)
>>
>> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
>> index 03d7bd03ad58..28efb7a9245a 100644
>> --- a/drivers/crypto/ccp/psp-dev.c
>> +++ b/drivers/crypto/ccp/psp-dev.c
>> @@ -271,6 +271,34 @@ static int sev_ioctl_pek_gen(struct sev_issue_cmd *argp)
>>  return ret;
>>  }
>>  
>> +static int sev_ioctl_pdh_gen(struct sev_issue_cmd *argp)
>> +{
>> +int ret, state, do_shutdown = 0;
>> +
>> +/*
>> + * PDH_GEN command can be issued when platform is in INIT or WORKING
>> + * state. If we are in UNINIT state then transition in INIT state
>> + * before issuing the command.
>> + */
>> +ret = sev_platform_get_state(, >error);
>> +if (ret)
>> +return ret;
>> +
> Why isn't this function doing:
>
> if (state == SEV_STATE_WORKING) {
> return -EBUSY;
>
> like the PEK_GEN one?

We need to follow the platform state machine logic defined in SEV spec
section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING
state because the command actually re-generate the identity of the
platform itself (in other words re-generate the Platform Endorsement
Key). Whereas, the PDH_GEN command is used for re-generating Platform
Diffie-Hellman Key which can be changed while the guest is running.

> Because if so, you can convert it and the PEK_GEN one into a single
> function doing the work and wrappers handing in the command to avoid the
> code duplication.
>
>> +if (state == SEV_STATE_UNINIT) {
>> +ret = sev_firmware_init(>error);
>> +if (ret)
>> +return ret;
>> +do_shutdown = 1;
>> +}
>> +
>> +ret = sev_handle_cmd(SEV_CMD_PDH_GEN, 0, >error);
>> +
>> +if (do_shutdown)
>> +sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
>> +
>> +return ret;
>> +}
>> +
>>  static long sev_ioctl(struct file *file, unsigned int ioctl, unsigned long 
>> arg)
>>  {
>>  void __user *argp = (void __user *)arg;
>> @@ -300,6 +328,10 @@ static long sev_ioctl(struct file *file, unsigned int 
>> ioctl, unsigned long arg)
>>  ret = sev_ioctl_pek_gen();
>>  break;
>>  }
>> +case SEV_PDH_GEN: {
>> +ret = sev_ioctl_pdh_gen();
>> +break;
>> +}
> And those curly braces can go, as before.
>

Re: [Part2 PATCH v5.1 12.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command

2017-10-12 Thread Borislav Petkov

On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
> Lets  consider this scenario
> 1- platform is in uninit state, we transition it to INIT
> 2- PEK_GEN command failed
> 3- since we have transitioned the platform in INIT state hence we must
> call the shutdown otherwise we will leave the system in wrong state. The
> shutdown command will most probably succeed and we will look the ret value

Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
the shutdown command fails?

You probably should carve out the whole shutdown order in separate
functions. I mean, the sequences do repeat in a couple of functions so
you could do:

ioctl:

case :

init_platform()
do_main_cmd()
shutdown_platform()
break;

and this way you have everything nicely separated and retvals properly
tracked...

Hmmm?

-- 
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.6/31] crypto: ccp: Implement SEV_PDH_GEN ioctl command

2017-10-12 Thread Borislav Petkov

On Thu, Oct 12, 2017 at 03:21:04PM -0500, Brijesh Singh wrote:
> We need to follow the platform state machine logic defined in SEV spec
> section 5.1.2. The PEK_GEN can not be issued when platform is in WORKING
> state because the command actually re-generate the identity of the
> platform itself (in other words re-generate the Platform Endorsement
> Key). Whereas, the PDH_GEN command is used for re-generating Platform
> Diffie-Hellman Key which can be changed while the guest is running.

I see.

So the proposition to carve out and split the platform *init commands
might come in handy here too...

-- 
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.5/31] crypto: ccp: Implement SEV_PEK_GEN ioctl command

2017-10-12 Thread Brijesh Singh



On 10/12/17 3:21 PM, Borislav Petkov wrote:
> On Thu, Oct 12, 2017 at 03:11:07PM -0500, Brijesh Singh wrote:
>> Lets  consider this scenario
>> 1- platform is in uninit state, we transition it to INIT
>> 2- PEK_GEN command failed
>> 3- since we have transitioned the platform in INIT state hence we must
>> call the shutdown otherwise we will leave the system in wrong state. The
>> shutdown command will most probably succeed and we will look the ret value
> Sure but what do you do if the main command, i.e., PEK_GEN succeeds but
> the shutdown command fails?
>
> You probably should carve out the whole shutdown order in separate
> functions. I mean, the sequences do repeat in a couple of functions so
> you could do:
>
> ioctl:
>
>   case :
>
>   init_platform()
>   do_main_cmd()
>   shutdown_platform()
>   break;
>
> and this way you have everything nicely separated and retvals properly
> tracked...
>
> Hmmm?

Some commands are allowed in INIT and WORKING, some in UINIT only,  some
WORKING, and others in all the state. We need to follow the platform
state machine. I will see what I can do. thanks

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

2017-10-12 Thread Brijesh Singh



On 10/12/17 9:08 AM, Borislav Petkov wrote:
...

> Well, if you're going to have a global var, why not pull up the misc
> device instead?
>
> And mind you, I've moved out this assignments:
>
> +   psp->sev_misc = psp_misc_dev;
> +   init_waitqueue_head(>sev_int_queue);
> +   dev_info(dev, "registered SEV device\n");
>
> outside of the if-conditional as I'm assuming you want to do this for
> each psp device for which sev_ops_init() is called.
>
> Or am I wrong here?

I am OK with your patch except  one minor issue highlighted below.

> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index 175cb3c3b8ef..d50aaa1ca75b 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -31,7 +31,7 @@
>  #define DEVICE_NAME  "sev"
>  
>  static DEFINE_MUTEX(sev_cmd_mutex);
> -static bool sev_fops_registered;
> +static struct miscdevice *psp_misc_dev;
>  
>  static struct psp_device *psp_alloc_struct(struct sp_device *sp)
>  {
> @@ -242,7 +242,6 @@ EXPORT_SYMBOL_GPL(sev_guest_df_flush);
>  static int sev_ops_init(struct psp_device *psp)
>  {
>   struct device *dev = psp->dev;
> - struct miscdevice *misc;
>   int ret;
>  
>   /*
> @@ -252,26 +251,24 @@ static int sev_ops_init(struct psp_device *psp)
>* sev_do_cmd() finds the right master device to which to issue the
>* command to the firmware.
>*/
> - if (!sev_fops_registered) {
> -
> - misc = devm_kzalloc(dev, sizeof(*misc), GFP_KERNEL);
> - if (!misc)
> + if (!psp_misc_dev) {
> + psp_misc_dev = devm_kzalloc(dev, sizeof(struct miscdevice), 
> GFP_KERNEL);
> + if (!psp_misc_dev)
>   return -ENOMEM;
>  
> - misc->minor = MISC_DYNAMIC_MINOR;
> - misc->name = DEVICE_NAME;
> - misc->fops = _fops;
> + psp_misc_dev->minor = MISC_DYNAMIC_MINOR;
> + psp_misc_dev->name = DEVICE_NAME;
> + psp_misc_dev->fops = _fops;
>  
> - ret = misc_register(misc);
> + ret = misc_register(psp_misc_dev);
>   if (ret)
>   return ret;
> -
> - sev_fops_registered = true;
> - psp->sev_misc = misc;
> - init_waitqueue_head(>sev_int_queue);
> - dev_info(dev, "registered SEV device\n");
>   }
>  
> + psp->sev_misc = psp_misc_dev;
> + init_waitqueue_head(>sev_int_queue);
> + dev_info(dev, "registered SEV device\n");
> +
>   return 0;
>  }
>  
> @@ -288,8 +285,8 @@ static int sev_init(struct psp_device *psp)
>  
>  static void sev_exit(struct psp_device *psp)
>  {
> - if (psp->sev_misc)
> - misc_deregister(psp->sev_misc);
> + if (psp_misc_dev)
> + misc_deregister(psp_misc_dev);

The sev_exit() will be called for all the psp_device instance. we need
to set psp_misc_dev = NULL after deregistering the device.

if (psp_misc_dev) {
  misc_deregister(psp_misc_dev);
   psp_misc_dev = NULL;
}

>  }
>  
>  int psp_dev_init(struct sp_device *sp)
>

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

2017-10-12 Thread Borislav Petkov

On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote:
> The sev_exit() will be called for all the psp_device instance. we need
> to set psp_misc_dev = NULL after deregistering the device.
> 
> if (psp_misc_dev) {
>   misc_deregister(psp_misc_dev);
>    psp_misc_dev = NULL;

Right, except we assign that misc device to every psp device:

psp->sev_misc = psp_misc_dev;

so the question now is, how do you want this to work: the misc device
should be destroyed after the last psp device is destroyed or how?

Because after the first:

psp_misc_dev = NULL;

the respective psp->sev_misc will still keep references to that device
but the device itself will be already deregistered. And that sounds
strange.

-- 
Regards/Gruss,
Boris.

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

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

2017-10-12 Thread Brijesh Singh


On 10/12/17 4:41 PM, Borislav Petkov wrote:
> On Thu, Oct 12, 2017 at 04:11:18PM -0500, Brijesh Singh wrote:
>> The sev_exit() will be called for all the psp_device instance. we need
>> to set psp_misc_dev = NULL after deregistering the device.
>>
>> if (psp_misc_dev) {
>>   misc_deregister(psp_misc_dev);
>>    psp_misc_dev = NULL;
> Right, except we assign that misc device to every psp device:
>
>   psp->sev_misc = psp_misc_dev;
>
> so the question now is, how do you want this to work: the misc device
> should be destroyed after the last psp device is destroyed or how?

We don't know when the last psp device is getting destroyed. Since we
are  making the sev_misc_dev as a global variable then there is no
reason for 'sev_misc' field in the psp_device.

> Because after the first:
>
>   psp_misc_dev = NULL;
>
> the respective psp->sev_misc will still keep references to that device
> but the device itself will be already deregistered. And that sounds
> strange.

See my above comment, I think the simplest solution is remove psp->sev_misc

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

2017-10-12 Thread Borislav Petkov

On Thu, Oct 12, 2017 at 04:52:32PM -0500, Brijesh Singh wrote:
> See my above comment, I think the simplest solution is remove psp->sev_misc

Ok, so far so good.

But now you still need to track which is the last psp device and to call
misc_deregister() only when the last device exits. Because if you do
that for the first psp device as it is now, all the following devices
will see a deregistered misc device. And I don't think we want that.

You probably could do something with reference counting: Documentation/kref.txt
to track that and have the last device deregister the misc device.

Or have the "enclosing" sp-dev deregister the misc device when it is
exiting and when it is sure that there are no more psp devices...

-- 
Regards/Gruss,
Boris.

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

[PATCH] crypto: qat: qat_common: qat_uclo - mark expected switch fall-throughs

2017-10-12 Thread Gustavo A. R. Silva

In preparation to enabling -Wimplicit-fallthrough, mark switch cases
where we are expecting to fall through.

Signed-off-by: Gustavo A. R. Silva 
---
 drivers/crypto/qat/qat_common/qat_uclo.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/crypto/qat/qat_common/qat_uclo.c 
b/drivers/crypto/qat/qat_common/qat_uclo.c
index e2454d9..61ae091 100644
--- a/drivers/crypto/qat/qat_common/qat_uclo.c
+++ b/drivers/crypto/qat/qat_common/qat_uclo.c
@@ -791,6 +791,7 @@ static int qat_uclo_init_reg(struct 
icp_qat_fw_loader_handle *handle,
case ICP_GPA_ABS:
case ICP_GPB_ABS:
ctx_mask = 0;
+   /* fall through */
case ICP_GPA_REL:
case ICP_GPB_REL:
return qat_hal_init_gpr(handle, ae, ctx_mask, reg_type,
@@ -800,6 +801,7 @@ static int qat_uclo_init_reg(struct 
icp_qat_fw_loader_handle *handle,
case ICP_SR_RD_ABS:
case ICP_DR_RD_ABS:
ctx_mask = 0;
+   /* fall through */
case ICP_SR_REL:
case ICP_DR_REL:
case ICP_SR_RD_REL:
@@ -809,6 +811,7 @@ static int qat_uclo_init_reg(struct 
icp_qat_fw_loader_handle *handle,
case ICP_SR_WR_ABS:
case ICP_DR_WR_ABS:
ctx_mask = 0;
+   /* fall through */
case ICP_SR_WR_REL:
case ICP_DR_WR_REL:
return qat_hal_init_wr_xfer(handle, ae, ctx_mask, reg_type,
-- 
2.7.4

Re: [Part2 PATCH v5.1 12.7/31] crypto: ccp: Implement SEV_PEK_CSR ioctl command

2017-10-12 Thread Brijesh Singh



On 10/12/17 2:53 PM, Borislav Petkov wrote:
...

> Ok, a couple of things here:
>
> * Move the checks first and the allocations second so that you allocate
> memory only after all checks have been passed and you don't allocate
> pointlessly.


I assume you mean performing the SEV state check before allocating the
memory for the CSR blob, right ? In my patches, I typically perform all
the SW specific checks and allocation before invoking the HW routines.
Handling the PSP commands will take longer compare to kmalloc() or
access_ok() etc. If its not a big deal then I would prefer to keep that
way.


>
> * That:
>
> if (state == SEV_STATE_WORKING) {
> ret = -EBUSY;
> goto e_free_blob;
> } else if (state == SEV_STATE_UNINIT) {
> ret = sev_firmware_init(>error);
> if (ret)
> goto e_free_blob;
> do_shutdown = 1;
> }
>
> is a repeating pattern. Perhaps it should be called
> sev_firmware_reinit() and called by other functions.


> * The rest is simplifications and streamlining.
>
> ---
> diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
> index e3ee68afd068..d41f5448a25b 100644
> --- a/drivers/crypto/ccp/psp-dev.c
> +++ b/drivers/crypto/ccp/psp-dev.c
> @@ -302,33 +302,30 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
>   int ret, state;
>   void *blob;
>  
> - if (copy_from_user(, (void __user *)(uintptr_t)argp->data,
> -sizeof(struct sev_user_data_pek_csr)))
> + if (copy_from_user(, (void __user *)argp->data, sizeof(input)))
> + return -EFAULT;
> +
> + if (!input.address)
> + return -EINVAL;
> +
> + /* allocate a physically contiguous buffer to store the CSR blob */
> + if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
> + input.length > SEV_FW_BLOB_MAX_SIZE)
>   return -EFAULT;
>  
>   data = kzalloc(sizeof(*data), GFP_KERNEL);
>   if (!data)
>   return -ENOMEM;
>  
> - /* allocate a temporary physical contigous buffer to store the CSR blob 
> */
> - blob = NULL;
> - if (input.address) {
> - if (!access_ok(VERIFY_WRITE, input.address, input.length) ||
> - input.length > SEV_FW_BLOB_MAX_SIZE) {
> - ret = -EFAULT;
> - goto e_free;
> - }
> -
> - blob = kmalloc(input.length, GFP_KERNEL);
> - if (!blob) {
> - ret = -ENOMEM;
> - goto e_free;
> - }
> -
> - data->address = __psp_pa(blob);
> - data->len = input.length;
> + blob = kmalloc(input.length, GFP_KERNEL);
> + if (!blob) {
> + ret = -ENOMEM;
> + goto e_free;
>   }
>  
> + data->address = __psp_pa(blob);
> + data->len = input.length;
> +
>   ret = sev_platform_get_state(, >error);
>   if (ret)
>   goto e_free_blob;
> @@ -349,25 +346,23 @@ static int sev_ioctl_pek_csr(struct sev_issue_cmd *argp)
>   do_shutdown = 1;
>   }
>  
> - ret = sev_handle_cmd(SEV_CMD_PEK_CSR, data, >error);
> + ret = sev_do_cmd(SEV_CMD_PEK_CSR, data, >error);
>  
>   input.length = data->len;
>  
>   /* copy blob to userspace */
> - if (blob &&
> - copy_to_user((void __user *)(uintptr_t)input.address,
> - blob, input.length)) {
> + if (copy_to_user((void __user *)input.address, blob, input.length)) {
>   ret = -EFAULT;
>   goto e_shutdown;
>   }
>  
> - if (copy_to_user((void __user *)(uintptr_t)argp->data, ,
> -  sizeof(struct sev_user_data_pek_csr)))
> + if (copy_to_user((void __user *)argp->data, , sizeof(input)))
>   ret = -EFAULT;
>  
>  e_shutdown:
>   if (do_shutdown)
> - sev_handle_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> + ret = sev_do_cmd(SEV_CMD_SHUTDOWN, 0, NULL);
> +
>  e_free_blob:
>   kfree(blob);
>  e_free:
> @@ -408,10 +403,10 @@ static long sev_ioctl(struct file *file, unsigned int 
> ioctl, unsigned long arg)
>   ret = sev_ioctl_pdh_gen();
>   break;
>  
> - case SEV_PEK_CSR: {
> + case SEV_PEK_CSR:
>   ret = sev_ioctl_pek_csr();
>   break;
> - }
> +
>   default:
>   ret = -EINVAL;
>   goto out;
>

38 matches

Mail list logo