cpu load calculation
Hi there, We think we might have encountered a problem with the cpu load calculation in Linux (2.2.x). The current calculation is based on dividing the jiffies to user, system and idle jiffies according to the process the timer hardware interrupt interrupts. In our case the timer interrupt generates some kernel processing in the bottom half. After that finishes, the idle process takes over. The system therefore seems completely idle while in actuality if the kernel processing takes 50% of a jiffie the system is 50% loaded (with system processing). The problem obviously results from synchronization of processing with the timer interrupt used to calculate the load. - Did anybody encounter this problem? - Know any solutions? - Recommend any specific solution? Many thanks, Gilad. -- Gilad Ben-Yossef [EMAIL PROTECTED] http://benyossef.com :: +972(54)756701 "Anything that can go wrong, will go wrong, while interrupts are disabled. " -- Murphey's law of kernel programing. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 00/32] [RFC] nohz/cpuset: Start discussions on nohz CPUs
On Mon, Oct 29, 2012 at 10:27 PM, Steven Rostedt rost...@goodmis.org wrote: A while ago Frederic posted a series of patches to get an idea on how to implement nohz cpusets. snip By using isocpus and nohz cpuset, a task would be able to achieve true cpu isolation. This has been long asked for by those in the RT community. If a task requires uninterruptible CPU time, this would be able to give a task that, even without the full PREEMPT-RT patch set. This patch set is not for inclusion. It is just to get the topic at the forefront again. The design requires more work and more discussion. Three additional data points that might be of interest to the discussion: 1. AFAIK both Tilera and Cavium carry patch sets with similar functionality in their respective kernels, so the idea has some real world users already. 2. I tested a previous version of the same patch set (based on 3.3) together with some fixes* and got the same latency, in cycles, from a simple test program and a version of said program running bare metal with no OS. The same program running without this patch got 3 orders of magnitude higher latency. So, this certainly shows some great potential. 3. Even if you don't care about latency at all, on a massively multi-core (or hyperscale, as I've read some people call it now) systems, assigning a task to a single CPU can makes a lot of sense from a cache utilization perspective etc; if you that, this feature can give a performance boost to anything that is mostly CPU bound and perhaps for some workloads that are not so CPU bound as well. Specifically, many high performance computing type of workloads come to mind. So, this has the potential to be useful to both RT folks and HPC folks, I think. [*] A newer version patch set: http://www.spinics.net/lists/linux-mm/msg33860.html and disabling the part that sends IPI to update cputime for nohz/cpuset CPUs. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] context_tracking: New context tracking susbsystem
On Sat, Nov 3, 2012 at 6:09 PM, Frederic Weisbecker fweis...@gmail.com wrote: Create a new subsystem that probes on kernel boundaries to keep track of the transitions between level contexts with two basic initial contexts: user or kernel. This is an abstraction of some RCU code that use such tracking to implement its userspace extended quiescent state. We need to pull this up from RCU into this new level of indirection because this tracking is also going to be used to implement an on demand generic virtual cputime accounting. A necessary step to shutdown the tick while still accounting the cputime. ... diff --git a/arch/Kconfig b/arch/Kconfig index 366ec06..3855e06 100644 --- a/arch/Kconfig +++ b/arch/Kconfig @@ -300,15 +300,15 @@ config SECCOMP_FILTER See Documentation/prctl/seccomp_filter.txt for details. -config HAVE_RCU_USER_QS +config HAVE_CONTEXT_TRACKING bool help - Provide kernel entry/exit hooks necessary for userspace + Provide kernel/user boundaries probes necessary for userspace RCU extended quiescent state. Syscalls need to be wrapped inside A minor nit pick: if whole point of the patch is to turn an RCU specific mechanism to a generic one that RCU happens to use, then the text needs to reflect that. How about: Provide kernel/user boundaries probes necessary for subsystems that need it, such as userspace RCU extended quiescent state. - rcu_user_exit()-rcu_user_enter() through the slow path using - TIF_NOHZ flag. Exceptions handlers must be wrapped as well. Irqs - are already protected inside rcu_irq_enter/rcu_irq_exit() but - preemption or signal handling on irq exit still need to be protected. + user_exit()-user_enter() through the slow path using TIF_NOHZ flag. + Exceptions handlers must be wrapped as well. Irqs are already + protected inside rcu_irq_enter/rcu_irq_exit() but preemption or + signal handling on irq exit still need to be protected. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC Patch v1 00/31] Synopsys ARC Linux kernel Port
On Wed, Nov 7, 2012 at 11:47 AM, Vineet Gupta vineet.gup...@synopsys.com wrote: Hi, This patchset based off-of 3.7-rc3, introduces the Linux kernel port to ARC700 processor family (750D and 770D) from Synopsys. We've been using the port for 6 month now at Ezchip running on top of FPGA in 4 way SMP configuration (support for which will no doubt follow later) and have subjected it to various stress tests (hackbench and such) and it is working up quite well over all. I realize some code style or organization changes might be in order but at the same time wanted to attest the nature of its good and behavior performance on real hardware; so for what it's worth: Tested-by: Gilad Ben-Yossef gi...@benyossef.com Cheers, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH v1 14/31] ARC: syscall support
On Wed, Nov 7, 2012 at 4:21 PM, Arnd Bergmann a...@arndb.de wrote: On Wednesday 07 November 2012, Vineet Gupta wrote: + * Being uClibc based we need some of the deprecated syscalls: + * -Not emulated by uClibc at all + * unlink, mkdir,... (needed by Busybox, LTP etc) + * times (needed by LTP pan test harness) + * -Not emulated efficiently + * select: emulated using pselect (but extra code to chk usec 1sec) + * ... I'm pretty sure that this has been solved before, best get in contact with the maintainers of the openrisc/c6x/hexagon platforms, that probably all use uClibc without needing these. You have to remove the legacy calls here. So, I completely agree about not adding more deprecated system call or ABIs (thinking about the ptrace regset issues in another patch in the same patchset), but on the other hand I have to wonder if having a port in the tree that doesn't have a working C library or a debugger makes sense. I mean, it is not quite the same thing as saying: well, users of the old versions of the user tools will need to maintain out of tree patches. That makes sense - it puts the burden of maintenance on people clinging to new versions when newer one exists, but this is not what is happening with Arc. Right now, there are no working version of the tools for Arc, so everyone will need to use the out of tree patches. I wonder what is worse - having an in tree port that no one (can) use or adding some deprecated crap (sorry...), clearly marked for deletion the minute a version of the relevant user tools exists that can be used with the new mechanisms? Just my 2c as an Arc kernel port user, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC GIT PULL] nohz: Kconfig layout improvements
Hi, [ Sorry for dropping LKML on my previous email. It was caused by replying from my smartphone. Adding everyone back now ] On Thu, Apr 4, 2013 at 9:23 PM, Christoph Lameter c...@linux.com wrote: On Thu, 4 Apr 2013, Gilad Ben-Yossef wrote: The vmstat work item leaving a timer running on the CPU, perhaps? Of course there is always a timer running for VM work. Check /proc/timers Not compiled in. I've sent a patch previously to make vmstat disable the timer if no VM work has been detected since the last run. Maybe you can try it? It was against 3.4 though. Probably won't apply... Can you repost it? CC me on future postings. I wrote the vmstat handling. Here is the last version I posted over a year ago. You were CCed and provided very useful feedback: http://lkml.indiana.edu/hypermail/linux/kernel/1205.0/01291.html Based on your feedback I re-spun them but never gotten around to port them to recent kernel and post them. The latest (yet unpublished versions) are here: https://github.com/gby/linux/commit/04e041327036772383c14ebdc450f522d782f264 https://github.com/gby/linux/commit/f5b8ae815670a289af9ff22ad83da3295472e63c I'll try to resurrect them, but maybe they'll prove useful as a reference as they are in the meantime. Hope it helps, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mm: when handling percpu_pagelist_fraction, use on_each_cpu() to set percpu pageset fields.
On Fri, Apr 5, 2013 at 11:33 PM, Cody P Schafer c...@linux.vnet.ibm.com wrote: In free_hot_cold_page(), we rely on pcp-batch remaining stable. Updating it without being on the cpu owning the percpu pageset potentially destroys this stability. Change for_each_cpu() to on_each_cpu() to fix. Are you referring to this? - 1329 if (pcp-count = pcp-high) { 1330 free_pcppages_bulk(zone, pcp-batch, pcp); 1331 pcp-count -= pcp-batch; 1332 } I'm probably missing the obvious but won't it be simpler to do this in free_hot_cold_page() - 1329 if (pcp-count = pcp-high) { 1330 unsigned int batch = ACCESS_ONCE(pcp-batch); 1331 free_pcppages_bulk(zone, batch, pcp); 1332 pcp-count -= batch; 1333 } Now the batch value used is stable and you don't have to IPI every CPU in the system just to change a config knob... Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mm: when handling percpu_pagelist_fraction, use on_each_cpu() to set percpu pageset fields.
On Mon, Apr 8, 2013 at 8:28 PM, Cody P Schafer c...@linux.vnet.ibm.com wrote: On 04/08/2013 05:20 AM, Gilad Ben-Yossef wrote: On Fri, Apr 5, 2013 at 11:33 PM, Cody P Schafer c...@linux.vnet.ibm.com wrote: In free_hot_cold_page(), we rely on pcp-batch remaining stable. Updating it without being on the cpu owning the percpu pageset potentially destroys this stability. Change for_each_cpu() to on_each_cpu() to fix. Are you referring to this? - This was the case I noticed. 1329 if (pcp-count = pcp-high) { 1330 free_pcppages_bulk(zone, pcp-batch, pcp); 1331 pcp-count -= pcp-batch; 1332 } I'm probably missing the obvious but won't it be simpler to do this in free_hot_cold_page() - 1329 if (pcp-count = pcp-high) { 1330 unsigned int batch = ACCESS_ONCE(pcp-batch); 1331 free_pcppages_bulk(zone, batch, pcp); 1332 pcp-count -= batch; 1333 } Potentially, yes. Note that this was simply the one case I noticed, rather than certainly the only case. OK, so perhaps the right thing to do is to understand what are (some of) the other cases so that we may choose the right solution. I also wonder whether there could be unexpected interactions between -high and -batch not changing together atomically. For example, could adjusting this knob cause -batch to rise enough that it is greater than the previous -high? If the code above then runs with the previous -high, -count wouldn't be correct (checking this inside free_pcppages_bulk() might help on this one issue). You are right, but that can be treated in setup_pagelist_highmark() e.g.: 3993 static void setup_pagelist_highmark(struct per_cpu_pageset *p, 3994 unsigned long high) 3995 { 3996 struct per_cpu_pages *pcp; unsigned int batch; 3997 3998 pcp = p-pcp; /* We're about to mess with PCP in an non atomic fashion. Put an intermediate safe value of batch and make sure it is visible before any other change */ pcp-batch = 1UL; smb_mb(); 3999 pcp-high = high; 4000 batch = max(1UL, high/4); 4001 if ((high/4) (PAGE_SHIFT * 8)) 4002 batch = PAGE_SHIFT * 8; pcp-batch = batch; 4003 } Or we could use an RCU here, but that might be an overkill. Now the batch value used is stable and you don't have to IPI every CPU in the system just to change a config knob... Is this really considered an issue? I wouldn't have expected someone to adjust the config knob often enough (or even more than once) to cause problems. Of course as a It'd be nice thing, I completely agree. Well, interfering unconditionally with other CPUs either via IPIs or scheduling work on them is a major headache for people that run work on machines with 4k CPUs, especially the HPC or RT or combos from the finance and networking users. If this was the only little knob or trigger that does this, then maybe it wont be so bad, but the problem is there is a list of these little knobs and items that potentially cause cross machine interference, and the poor sys admin has to keep them all in his or her head: Now, is it ok to pull this knob now, or will it cause an IPI s**t storm? We can never get rid of them all, but I'd really prefer to keep them down to a minimum if at all possible. Here, it looks to me that it is possible and that the price is not great - that is, the resulting code is not too hairy or none maintainable. At least, that is how it looks to me. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/3] mm: when handling percpu_pagelist_fraction, use on_each_cpu() to set percpu pageset fields.
On Tue, Apr 9, 2013 at 9:03 AM, Gilad Ben-Yossef gi...@benyossef.com wrote: I also wonder whether there could be unexpected interactions between -high and -batch not changing together atomically. For example, could adjusting this knob cause -batch to rise enough that it is greater than the previous -high? If the code above then runs with the previous -high, -count wouldn't be correct (checking this inside free_pcppages_bulk() might help on this one issue). You are right, but that can be treated in setup_pagelist_highmark() e.g.: 3993 static void setup_pagelist_highmark(struct per_cpu_pageset *p, 3994 unsigned long high) 3995 { 3996 struct per_cpu_pages *pcp; unsigned int batch; 3997 3998 pcp = p-pcp; /* We're about to mess with PCP in an non atomic fashion. Put an intermediate safe value of batch and make sure it is visible before any other change */ pcp-batch = 1UL; smb_mb(); 3999 pcp-high = high; and i think I missed another needed barrier here: smp_mb(); 4000 batch = max(1UL, high/4); 4001 if ((high/4) (PAGE_SHIFT * 8)) 4002 batch = PAGE_SHIFT * 8; pcp-batch = batch; 4003 } -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 03/10] mm/page_alloc: insert memory barriers to allow async update of pcp batch and high
On Wed, Apr 10, 2013 at 2:28 AM, Cody P Schafer c...@linux.vnet.ibm.com wrote: In pageset_set_batch() and setup_pagelist_highmark(), ensure that batch is always set to a safe value (1) prior to updating high, and ensure that high is fully updated before setting the real value of batch. Suggested by Gilad Ben-Yossef gi...@benyossef.com in this thread: https://lkml.org/lkml/2013/4/9/23 Also reproduces his proposed comment. Signed-off-by: Cody P Schafer c...@linux.vnet.ibm.com --- mm/page_alloc.c | 19 +++ 1 file changed, 19 insertions(+) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index d259599..a07bd4c 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -4007,11 +4007,26 @@ static int __meminit zone_batchsize(struct zone *zone) #endif } +static void pageset_update_prep(struct per_cpu_pages *pcp) +{ + /* +* We're about to mess with PCP in an non atomic fashion. Put an +* intermediate safe value of batch and make sure it is visible before +* any other change +*/ + pcp-batch = 1; + smp_wmb(); +} + /* a companion to setup_pagelist_highmark() */ static void pageset_set_batch(struct per_cpu_pageset *p, unsigned long batch) { struct per_cpu_pages *pcp = p-pcp; + pageset_update_prep(pcp); + pcp-high = 6 * batch; + smp_wmb(); + pcp-batch = max(1UL, 1 * batch); } @@ -4039,7 +4054,11 @@ static void setup_pagelist_highmark(struct per_cpu_pageset *p, struct per_cpu_pages *pcp; pcp = p-pcp; + pageset_update_prep(pcp); + pcp-high = high; + smp_wmb(); + pcp-batch = max(1UL, high/4); if ((high/4) (PAGE_SHIFT * 8)) pcp-batch = PAGE_SHIFT * 8; -- 1.8.2 That is very good. However, now we've created a protocol for updating -high and -batch: 1. Call pageset_update_prep(pcp) 2. Update -high 3. smp_wmb() 4. Update -batch But that protocol is not documented anywhere and someone that reads the code two years from now will not be aware of it or why it is done like that. How about if we create: /* * pcp-high and pcp-batch values are related and dependent on one another: * -batch must never be higher then -high. * The following function updates them in a safe manner without a costly atomic transaction. */ static void pageset_update(struct per_cpu_pages *pcp, unsigned int high, unsigned int batch) { /* start with a fail safe value for batch */ pcp-batch = 1; smp_wmb(); /* Update high, then batch, in order */ pcp-high = high; smp_wmb(); pcp-batch = batch; } And use that at the update sites? then the protocol becomes explicit. Thanks, Gilad. -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 03/10] mm/page_alloc: insert memory barriers to allow async update of pcp batch and high
On Wed, Apr 10, 2013 at 9:19 AM, Gilad Ben-Yossef gi...@benyossef.com wrote: On Wed, Apr 10, 2013 at 2:28 AM, Cody P Schafer c...@linux.vnet.ibm.com wrote: In pageset_set_batch() and setup_pagelist_highmark(), ensure that batch is always set to a safe value (1) prior to updating high, and ensure that high is fully updated before setting the real value of batch. Suggested by Gilad Ben-Yossef gi...@benyossef.com in this thread: https://lkml.org/lkml/2013/4/9/23 Also reproduces his proposed comment. Signed-off-by: Cody P Schafer c...@linux.vnet.ibm.com --- mm/page_alloc.c | 19 +++ 1 file changed, 19 insertions(+) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index d259599..a07bd4c 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -4007,11 +4007,26 @@ static int __meminit zone_batchsize(struct zone *zone) #endif } +static void pageset_update_prep(struct per_cpu_pages *pcp) +{ + /* +* We're about to mess with PCP in an non atomic fashion. Put an +* intermediate safe value of batch and make sure it is visible before +* any other change +*/ + pcp-batch = 1; + smp_wmb(); +} + /* a companion to setup_pagelist_highmark() */ static void pageset_set_batch(struct per_cpu_pageset *p, unsigned long batch) { struct per_cpu_pages *pcp = p-pcp; + pageset_update_prep(pcp); + pcp-high = 6 * batch; + smp_wmb(); + pcp-batch = max(1UL, 1 * batch); } @@ -4039,7 +4054,11 @@ static void setup_pagelist_highmark(struct per_cpu_pageset *p, struct per_cpu_pages *pcp; pcp = p-pcp; + pageset_update_prep(pcp); + pcp-high = high; + smp_wmb(); + pcp-batch = max(1UL, high/4); if ((high/4) (PAGE_SHIFT * 8)) pcp-batch = PAGE_SHIFT * 8; -- 1.8.2 That is very good. However, now we've created a protocol for updating -high and -batch: 1. Call pageset_update_prep(pcp) 2. Update -high 3. smp_wmb() 4. Update -batch But that protocol is not documented anywhere and someone that reads the code two years from now will not be aware of it or why it is done like that. How about if we create: /* * pcp-high and pcp-batch values are related and dependent on one another: * -batch must never be higher then -high. * The following function updates them in a safe manner without a costly atomic transaction. */ static void pageset_update(struct per_cpu_pages *pcp, unsigned int high, unsigned int batch) { /* start with a fail safe value for batch */ pcp-batch = 1; smp_wmb(); /* Update high, then batch, in order */ pcp-high = high; smp_wmb(); pcp-batch = batch; } And use that at the update sites? then the protocol becomes explicit. Oh, and other then that it looks good to me, so assuming you do that - Reviewed-By: Gilad Ben-Yossef gi...@benyossef.com Many thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v2 2/2] mm: add sysctl to pick vmstat monitor cpu
Add a sysctl knob to enable admin to hand pick the scapegoat cpu that will perform the extra work of preiodically checking for new VM activity on CPUs that have switched off their vmstat_update work item schedling. Signed-off-by: Gilad Ben-Yossef gi...@benyossef.com CC: Christoph Lameter c...@linux.com CC: Paul E. McKenney paul...@linux.vnet.ibm.com CC: linux-kernel@vger.kernel.org CC: linux...@kvack.org --- include/linux/vmstat.h |1 + kernel/sysctl.c|7 mm/vmstat.c| 72 3 files changed, 74 insertions(+), 6 deletions(-) diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h index a30ab79..470f1d0 100644 --- a/include/linux/vmstat.h +++ b/include/linux/vmstat.h @@ -9,6 +9,7 @@ #include linux/atomic.h extern int sysctl_stat_interval; +extern int sysctl_vmstat_monitor_cpu; #ifdef CONFIG_VM_EVENT_COUNTERS /* diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 9edcf45..58c889e 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -1361,6 +1361,13 @@ static struct ctl_table vm_table[] = { .mode = 0644, .proc_handler = proc_dointvec_jiffies, }, + { + .procname = stat_monitor_cpu, + .data = sysctl_vmstat_monitor_cpu, + .maxlen = sizeof(sysctl_vmstat_monitor_cpu), + .mode = 0644, + .proc_handler = proc_dointvec, + }, #endif #ifdef CONFIG_MMU { diff --git a/mm/vmstat.c b/mm/vmstat.c index 6143c70..767412e 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -1187,7 +1187,7 @@ static const struct file_operations proc_vmstat_file_operations = { static DEFINE_PER_CPU(struct delayed_work, vmstat_work); int sysctl_stat_interval __read_mostly = HZ; static struct cpumask vmstat_cpus; -static int vmstat_monitor_cpu __read_mostly = VMSTAT_NO_CPU; +int sysctl_vmstat_monitor_cpu __read_mostly = VMSTAT_NO_CPU; static inline bool need_vmstat(int cpu) { @@ -1232,12 +1232,13 @@ static void vmstat_update(struct work_struct *w) { int cpu, this_cpu = smp_processor_id(); - if (unlikely(this_cpu == vmstat_monitor_cpu)) + if (unlikely(this_cpu == sysctl_vmstat_monitor_cpu)) for_each_cpu_not(cpu, vmstat_cpus) if (need_vmstat(cpu)) start_cpu_timer(cpu); - if (likely(refresh_cpu_vm_stats(this_cpu) || (this_cpu == vmstat_monitor_cpu))) + if (likely(refresh_cpu_vm_stats(this_cpu) || + (this_cpu == sysctl_vmstat_monitor_cpu))) schedule_delayed_work(__get_cpu_var(vmstat_work), round_jiffies_relative(sysctl_stat_interval)); else @@ -1266,9 +1267,9 @@ static int __cpuinit vmstat_cpuup_callback(struct notifier_block *nfb, if (cpumask_test_cpu(cpu, vmstat_cpus)) { cancel_delayed_work_sync(per_cpu(vmstat_work, cpu)); per_cpu(vmstat_work, cpu).work.func = NULL; - if(cpu == vmstat_monitor_cpu) { + if (cpu == sysctl_vmstat_monitor_cpu) { int this_cpu = smp_processor_id(); - vmstat_monitor_cpu = this_cpu; + sysctl_vmstat_monitor_cpu = this_cpu; if (!cpumask_test_cpu(this_cpu, vmstat_cpus)) start_cpu_timer(this_cpu); } @@ -1299,7 +1300,7 @@ static int __init setup_vmstat(void) register_cpu_notifier(vmstat_notifier); - vmstat_monitor_cpu = smp_processor_id(); + sysctl_vmstat_monitor_cpu = smp_processor_id(); for_each_online_cpu(cpu) setup_cpu_timer(cpu); @@ -1474,5 +1475,64 @@ fail: return -ENOMEM; } +#ifdef CONFIG_SYSCTL +/* + * proc handler for /proc/sys/mm/stat_monitor_cpu + * + * Note that there is a harmless race condition here: + * If you concurrently try to change the monitor CPU to + * a new valid one and an invalid (offline) one at the + * same time, you can get a success indication for the + * valid one, a failure for the invalid one, but end up + * with the old value. It's easily fixable but hardly + * worth the added complexity. + */ + +int proc_monitor_cpu(struct ctl_table *table, int write, + void __user *buffer, size_t *lenp, loff_t *ppos) +{ + int ret; + int tmp; + + /* +* We need to make sure the chosen and old monitor cpus don't +* go offline on us during the transition. +*/ + get_online_cpus(); + + tmp = sysctl_vmstat_monitor_cpu; + + ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos); + + if (ret || !write) + goto out; + + /* +* An offline CPU is a bad choice for monitoring duty. +* Abort
[PATCH v2 1/2] mm: make vmstat_update periodic run conditional
vmstat_update runs every second from the work queue to update statistics and drain per cpu pages back into the global page allocator. This is useful in most circumstances but is wasteful if the CPU doesn't actually make any VM activity. This can happen in the situtation that the CPU is idle or running a CPU bound long term task (e.g. CPU isolation), in which case the periodic vmstate_update timer needlessly interrupts the CPU. This patch tries to make vmstat_update schedule itself for the next round only if there was any work for it to do in the previous run. The assumption is that if for a whole second we didn't see any VM activity it is reasnoable to assume that the CPU is not using the VM because it is idle or runs a long term single CPU bound task. A scapegoat CPU is picked to serve to periodically monitor CPUs that have their vmstat_update work stopped and re-schedule them if VM activity is detected. The scapegoat CPU never stops its vmstat_update work item instance. Signed-off-by: Gilad Ben-Yossef gi...@benyossef.com CC: Christoph Lameter c...@linux.com CC: Paul E. McKenney paul...@linux.vnet.ibm.com CC: linux-kernel@vger.kernel.org CC: linux...@kvack.org --- include/linux/vmstat.h |2 +- mm/vmstat.c| 92 --- 2 files changed, 79 insertions(+), 15 deletions(-) diff --git a/include/linux/vmstat.h b/include/linux/vmstat.h index c586679..a30ab79 100644 --- a/include/linux/vmstat.h +++ b/include/linux/vmstat.h @@ -198,7 +198,7 @@ extern void __inc_zone_state(struct zone *, enum zone_stat_item); extern void dec_zone_state(struct zone *, enum zone_stat_item); extern void __dec_zone_state(struct zone *, enum zone_stat_item); -void refresh_cpu_vm_stats(int); +bool refresh_cpu_vm_stats(int); void refresh_zone_stat_thresholds(void); void drain_zonestat(struct zone *zone, struct per_cpu_pageset *); diff --git a/mm/vmstat.c b/mm/vmstat.c index f42745e..6143c70 100644 --- a/mm/vmstat.c +++ b/mm/vmstat.c @@ -14,6 +14,7 @@ #include linux/module.h #include linux/slab.h #include linux/cpu.h +#include linux/cpumask.h #include linux/vmstat.h #include linux/sched.h #include linux/math64.h @@ -432,11 +433,12 @@ EXPORT_SYMBOL(dec_zone_page_state); * with the global counters. These could cause remote node cache line * bouncing and will have to be only done when necessary. */ -void refresh_cpu_vm_stats(int cpu) +bool refresh_cpu_vm_stats(int cpu) { struct zone *zone; int i; int global_diff[NR_VM_ZONE_STAT_ITEMS] = { 0, }; + bool vm_activity = false; for_each_populated_zone(zone) { struct per_cpu_pageset *p; @@ -483,14 +485,21 @@ void refresh_cpu_vm_stats(int cpu) if (p-expire) continue; - if (p-pcp.count) + if (p-pcp.count) { + vm_activity = true; drain_zone_pages(zone, p-pcp); + } #endif } for (i = 0; i NR_VM_ZONE_STAT_ITEMS; i++) - if (global_diff[i]) + if (global_diff[i]) { atomic_long_add(global_diff[i], vm_stat[i]); + vm_activity = true; + } + + return vm_activity; + } /* @@ -1172,24 +1181,69 @@ static const struct file_operations proc_vmstat_file_operations = { #endif /* CONFIG_PROC_FS */ #ifdef CONFIG_SMP + +#define VMSTAT_NO_CPU (-1) + static DEFINE_PER_CPU(struct delayed_work, vmstat_work); int sysctl_stat_interval __read_mostly = HZ; +static struct cpumask vmstat_cpus; +static int vmstat_monitor_cpu __read_mostly = VMSTAT_NO_CPU; -static void vmstat_update(struct work_struct *w) +static inline bool need_vmstat(int cpu) { - refresh_cpu_vm_stats(smp_processor_id()); - schedule_delayed_work(__get_cpu_var(vmstat_work), - round_jiffies_relative(sysctl_stat_interval)); + struct zone *zone; + int i; + + for_each_populated_zone(zone) { + struct per_cpu_pageset *p; + + p = per_cpu_ptr(zone-pageset, cpu); + + for (i = 0; i NR_VM_ZONE_STAT_ITEMS; i++) + if (p-vm_stat_diff[i]) + return true; + + if (zone_to_nid(zone) != numa_node_id() p-pcp.count) + return true; + } + + return false; } -static void __cpuinit start_cpu_timer(int cpu) +static void vmstat_update(struct work_struct *w); + +static void start_cpu_timer(int cpu) { struct delayed_work *work = per_cpu(vmstat_work, cpu); - INIT_DEFERRABLE_WORK(work, vmstat_update); + cpumask_set_cpu(cpu, vmstat_cpus); schedule_delayed_work_on(cpu, work, __round_jiffies_relative(HZ, cpu)); } +static void __cpuinit setup_cpu_timer(int cpu) +{ + struct delayed_work *work = per_cpu(vmstat_work, cpu); + + INIT_DEFERRABLE_WORK(work, vmstat_update
Re: [RFC] Restrict kernel spawning of threads to a specified set of cpus.
Hi, On Thu, Sep 5, 2013 at 11:07 PM, Christoph Lameter c...@linux.com wrote: I am not sure how to call this kernel option but we need something like that. I see drivers and the kernel spawning processes on the nohz cores. The name kthread is not really catching the purpose. os_cpus=? highlatency_cpus=? First off, thank you for doing this. It is very useful :-) Currently if one wishes to run a single task on an isolated CPU with as little interference as possible, one needs to pass rcu_nocbs, isolcpus, nohz_full parameters and now kthread parameter, all pretty much with the same values I know some people won't like this, but can we perhaps fold all these into a single parameter, perhaps even the existing isolcpus? Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC vmstat: On demand vmstat threads
On Wed, Sep 4, 2013 at 7:48 PM, Christoph Lameter c...@linux.com wrote: vmstat threads are used for folding counter differentials into the zone, per node and global counters at certain time intervals. They currently run at defined intervals on all processors which will cause some holdoff for processors that need minimal intrusion by the OS. This patch creates a vmstat sheperd task that monitors the per cpu differentials on all processors. If there are differentials on a processor then a vmstat thread local to the processors with the differentials is created. That process will then start folding the diffs in regular intervals. Should the vmstat process find that there is no work to be done then it will terminate itself and make the sheperd task monitor the differentials again. I wasn't happy with the results of my own attempt to accomplish the same and I like this much better. So, for what it's worth - Reviewed-by: Gilad Ben-Yossef gi...@benyossef.com Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Restrict kernel spawning of threads to a specified set of cpus.
Hi, On Tue, Sep 10, 2013 at 9:47 AM, Mike Galbraith bitbuc...@online.de wrote: On Tue, 2013-09-10 at 09:05 +0300, Gilad Ben-Yossef wrote: Hi, On Thu, Sep 5, 2013 at 11:07 PM, Christoph Lameter c...@linux.com wrote: I am not sure how to call this kernel option but we need something like that. I see drivers and the kernel spawning processes on the nohz cores. The name kthread is not really catching the purpose. os_cpus=? highlatency_cpus=? First off, thank you for doing this. It is very useful :-) Currently if one wishes to run a single task on an isolated CPU with as little interference as possible, one needs to pass rcu_nocbs, isolcpus, nohz_full parameters and now kthread parameter, all pretty much with the same values I know some people won't like this, but can we perhaps fold all these into a single parameter, perhaps even the existing isolcpus? isolcpus is supposed to go away, as cpusets can isolate CPUs, and can turn off load balancing. And I'm all for that. I think cpusets is a much more elegant solution. But... AFAIK currently cpusets cannot migrate timers that were registered on a cpu prior to it being isolated via cpuset, designate RCU off loaded CPUs or sets cpus as full nohz capable, or - it seems from this patch, keep off certain kernel thread off a cpu. This is no fault of cpusets, but it still means there are work loads that it can't support at this time. So long as we must have a kernel boot option, I prefer to have one and not four of them. Think of it this way - when we put all these capabilities into cpusets, we'll have just one kernel option to kill and not four. Does that makes sense? Gilad -Mike -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Restrict kernel spawning of threads to a specified set of cpus.
On Tue, Sep 10, 2013 at 10:26 AM, Mike Galbraith bitbuc...@online.de wrote: Hammering on the wrong spot makes removing isolcpus take longer, and adds up to more hammering in the long run, no? Hearing you mention isolcpus, I just thought I should mention that it wants to go away, so might not be the optimal spot for isolation related tinkering. OK, so I'll bite - isolcpu currently has special magic to do its thing but AFAIK part of the reason isolcpu works better (for some definition of better, for some work loads) is simply because it blocks migration earlier than you get with cpusets. What if we re-did the implementation of isolcpu as creating an cpuset with migration off as early as possible in the boot process, prior to spawning init? So basically, isolcpus becomes just a way to configure a cpuset early? Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Restrict kernel spawning of threads to a specified set of cpus.
On Thu, Sep 12, 2013 at 5:16 PM, Frederic Weisbecker fweis...@gmail.com wrote: On Thu, Sep 12, 2013 at 02:10:56PM +, Christoph Lameter wrote: On Thu, 12 Sep 2013, Frederic Weisbecker wrote: Why not do this from userspace instead? Because the cpumasks are hardcoded in the kernel code. Ok but you can change the affinity of a kthread from userspace, as long as you define a cpu set that is among that kthread's cpus allowed. There is also the problem of kernel threads registering timers. We don't have a good way to migrate those yet, I believe. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC] Restrict kernel spawning of threads to a specified set of cpus.
On Thu, Sep 12, 2013 at 9:35 PM, Frederic Weisbecker fweis...@gmail.com wrote: Just offline the CPUs you want to isolate, affine your kthreads and re-online the CPUs. If you're lucky enough to have 1024 CPUs, a winter night should be enough ;-) Great, I have 4,096 CPUs. I guess I have to wait for the winter solstice :-) Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC vmstat: On demand vmstat threads
On Wed, Sep 18, 2013 at 5:06 PM, Andrew Morton a...@linux-foundation.org wrote: On Tue, 10 Sep 2013 21:13:34 + Christoph Lameter c...@linux.com wrote: With this patch it is possible then to have periods longer than 2 seconds without any OS event on a cpu (hardware thread). It would be useful (actually essential) to have a description of why anyone cares about this. A good and detailed description, please. Let me have a stab at this: The existing vmstat_update mechanism depends on a deferrable timer firing every second by default which registers a work queue item that runs on the local CPU, with the result that we have 1 interrupt and one additional schedulable task on each CPU aprox. every second. If your workload indeed causes VM activity or you are running multiple tasks per CPU, you probably have bigger issues to deal with. However, many existing workloads dedicate a CPU for a single CPU bound task. This is done by high performance computing folks, by high frequency financial applications folks, by networking folks (Intel DPDK, EZchip NPS) and with the advent of systems with more and more CPUs over time, this will(?) become more and more common to do since when you have enough CPUs you care less about efficiently sharing your CPU with other tasks and more about efficiently monopolizing a CPU per task. The difference of having this timer firing and workqueue kernel thread scheduled per second can be enormous. An artificial test I made measuring the worst case time to do a simple i++ in an endless loop on a bare metal system and under Linux on an isolated CPU (cpusets or isolcpus - take your pick) with dynticks and with and without this patch, have Linux match the bare metal performance (~700 cycles) with this patch and loose by couple of orders of magnitude (~200k cycles) without it[*] - and all this for something that just calculates statistics. For networking applications, for example, this is the difference between dropping packets or sustaining line rate. Statistics are important and useful, but if there is a way to not cause statistics gathering produce such a huge performance difference would be great. This is what we are trying to do here. Does it makes sense? [*] To be honest it required one more patch, but this one or something like is needed to get that one working, so... Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC vmstat: On demand vmstat threads
On Tue, Sep 10, 2013 at 4:13 PM, Christoph Lameter c...@linux.com wrote: Note: This patch is based on the vmstat patches in Andrew's tree to be merged for the 3.12 kernel. Sorry for being dumb but this patch doesn't apply for me on either mmotm nor Linus master. What did I miss? Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Kconfig.debug: Add FRAME_POINTER anti-dependency for ARC
Hi, On Tue, Aug 27, 2013 at 11:31 AM, Vineet Gupta vineet.gup...@synopsys.com wrote: Frame pointer on ARC doesn't serve the conventional purpose of stack unwinding due to the typical way ABI designates it's usage. More out of curiosity to understand the platform better than actual review - can you explain a little what you meant by this statement? Is this statement because of the use of write back mode with ld/st to or not related? Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] mm: make vmstat_update periodic run conditional
On Thu, Jun 20, 2013 at 5:05 PM, Christoph Lameter c...@linux.com wrote: On Wed, 19 Jun 2013, Gilad Ben-Yossef wrote: +static void vmstat_update(struct work_struct *w) +{ + int cpu, this_cpu = smp_processor_id(); + + if (unlikely(this_cpu == vmstat_monitor_cpu)) + for_each_cpu_not(cpu, vmstat_cpus) + if (need_vmstat(cpu)) + start_cpu_timer(cpu); + + if (likely(refresh_cpu_vm_stats(this_cpu) || (this_cpu == vmstat_monitor_cpu))) + schedule_delayed_work(__get_cpu_var(vmstat_work), + round_jiffies_relative(sysctl_stat_interval)); + else + cpumask_clear_cpu(this_cpu, vmstat_cpus); The clearing of vmstat_cpus could be avoided if this processor is not running tickless. Frequent updates to vmstat_cpus could become an issue. I like the idea of tying the vmstat disabling to the tickless logic but I seem to have run into a bit of a chicken and egg problem here: vmstat_update runs from the vmstat work queue item by the workqueue kernel thread. If this code is running, it means there are at least two schedulable tasks: 1. The workqueue kernel thread, because it is running. 2. At least one more task, otherwise were were in idle and the workqueue kernel thread would not execute this work item. Unfortunately, having two schedulable tasks means we're not running tickless, so the check will never trigger - or have I've missed something obvious? Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] smp.c: Quit unconditionally enabling irqs in on_each_cpu_mask().
On Thu, Aug 8, 2013 at 11:27 PM, David Daney dda...@caviumnetworks.com wrote: On 08/08/2013 12:25 PM, Christoph Lameter wrote: On Thu, 8 Aug 2013, David Daney wrote: I don't know of any bugs currently caused by this unconditional local_irq_enable(), but I want to use this function in MIPS/OCTEON early boot (when we have early_boot_irqs_disabled). This also makes this function have similar semantics to on_each_cpu() which is good in itself. smp_call_function_many() wants interrupts enabled. That's what the comments say, but it isn't actually true. The usage introduced by the patch is no different than the existing usage in on_each_cpu() 30 line up in the file. Regardless of the question of how smp_call_function_many() should be called, the IRQ disable/enable pair is actually there to make sure the provided function runs on the current CPU at the same conditions as it would get called via the IPI. I would at least consider putting a test there to make sure IRQs really are disabled when entering the function, otherwise the bugs stemming from incorrect use can be tricky to catch. Just my 0.2 BTC Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 1/2] mm: make vmstat_update periodic run conditional
On Thu, Aug 8, 2013 at 5:59 PM, Christoph Lameter c...@gentwo.org wrote: On Thu, 8 Aug 2013, Gilad Ben-Yossef wrote: vmstat_update runs from the vmstat work queue item by the workqueue kernel thread. If this code is running, it means there are at least two schedulable tasks: 1. The workqueue kernel thread, because it is running. 2. At least one more task, otherwise were were in idle and the workqueue kernel thread would not execute this work item. Unfortunately, having two schedulable tasks means we're not running tickless, so the check will never trigger - or have I've missed something obvious? The vmstat update is deferrable work. As such it is not required to run and can be pushed off. It will not be considered for the calculation of the next timer interupt. See __next_timer_interrupt(). Yes, I understand that. I was trying to say something else: If the code does not consider setting the vmstat_cpus bit in the mask unless we are running on a CPU in tickless state, than we will (almost) never set vmstat_cpus since we will (almost) never be tickless in a deferrable work - If there is no other task, we will be in idle and the deferreable work will not be scheduled since the timer will not fire. If there is one task originally, the work queue gets executed in the work queue kernel thread, so we have two tasks so tickless will disengae. If there is more than one task tickless is not engage. Bottom line - we will be in active tickless mode when running a deferreable work item only if we happen to have fire the timer that scheduled the work and the previously running task happened to block. This is rare enough that in practice we will almost never be in active tickless mode when running the vmstat_update function. I hope I manage to explain myself better this time. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: kernel: smp: WARNING at kernel/smp.c:673
Hi Sasha and Kim, I believe Kim is right and the WARN_ON_ONCE condition should be negated. Furthermore, I'm pretty sure I'm the one to blame for this bug. Good catch and thank you both. (Un)fortunately, I'm in a vacation in an island resort in Thailand at the moment and the beer is much better than the Internet connection. I'll do my best to prepare, test and send a patch, but if someone beats me to it before my return in 10 days I will be happy to review. Many thanks, Gilad On Aug 4, 2014 1:41 PM, Joonsoo Kim iamjoonsoo@lge.com wrote: On Sun, Aug 03, 2014 at 10:57:02PM -0400, Sasha Levin wrote: Hi all, While fuzzing with trinity inside a KVM tools guest running the latest -next kernel, I've stumbled on the following spew: [ 1226.701012] WARNING: CPU: 6 PID: 8624 at kernel/smp.c:673 on_each_cpu_cond+0x27f/0x2f0() [ 1226.703128] Modules linked in: [ 1226.703955] CPU: 6 PID: 8624 Comm: trinity-c26 Tainted: GW 3.16.0-rc7-next-20140801-sasha-00047-gd6ce559 #991 [ 1226.706792] 881b2e8a3000 23830039 881b2e8a3000 [ 1226.708179] 881ada5b3978 a1ee96b4 881ada5b39b8 [ 1226.709610] 9d1ce5ba 9d2c0ddf 001b 8807d7608000 [ 1226.710966] Call Trace: [ 1226.711417] dump_stack (lib/dump_stack.c:52) [ 1226.712311] warn_slowpath_common (kernel/panic.c:432) [ 1226.715425] warn_slowpath_null (kernel/panic.c:466) [ 1226.716436] on_each_cpu_cond (kernel/smp.c:673 (discriminator 3)) [ 1226.720821] __kmem_cache_shrink (mm/slub.c:3420) [ 1226.722711] slab_memory_callback (mm/slub.c:3464 mm/slub.c:3556) [ 1226.723925] notifier_call_chain (kernel/notifier.c:93) [ 1226.725244] __blocking_notifier_call_chain (kernel/notifier.c:319) [ 1226.726310] blocking_notifier_call_chain (kernel/notifier.c:329) [ 1226.727263] memory_notify (drivers/base/memory.c:177) [ 1226.728210] __offline_pages.constprop.23 (include/linux/notifier.h:179 mm/memory_hotplug.c:1682) [ 1226.731896] offline_pages (mm/memory_hotplug.c:1787) [ 1226.733095] memory_subsys_offline (drivers/base/memory.c:267 drivers/base/memory.c:304) [ 1226.735208] device_offline (drivers/base/core.c:1428) [ 1226.736966] online_store (drivers/base/core.c:451 (discriminator 2)) [ 1226.737720] dev_attr_store (drivers/base/core.c:138) [ 1226.739986] sysfs_kf_write (fs/sysfs/file.c:116) [ 1226.742105] kernfs_fop_write (fs/kernfs/file.c:308) [ 1226.743058] do_loop_readv_writev (fs/read_write.c:708) [ 1226.745095] do_readv_writev (fs/read_write.c:840) [ 1226.752769] vfs_writev (fs/read_write.c:879) [ 1226.753755] SyS_writev (fs/read_write.c:911 fs/read_write.c:903) [ 1226.754660] tracesys (arch/x86/kernel/entry_64.S:541) No time to dig too much into this today, will work on it tomorrow. I think that that WARN_ON_ONCE() in kernel/smp.c:673 is wrong. smp_call_function_single() returns 0 if success so it should be WARN_ON_ONCE(ret) rather than WARN_ON_ONCE(!ret). Thanks. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 5/6] nohz: support PR_DATAPLANE_STRICT mode
From: Andy Lutomirski [mailto:l...@amacapital.net] Sent: Saturday, May 09, 2015 10:29 AM To: Chris Metcalf Cc: Srivatsa S. Bhat; Paul E. McKenney; Frederic Weisbecker; Ingo Molnar; Rik van Riel; linux-...@vger.kernel.org; Andrew Morton; linux- ker...@vger.kernel.org; Thomas Gleixner; Tejun Heo; Peter Zijlstra; Steven Rostedt; Christoph Lameter; Gilad Ben Yossef; Linux API Subject: Re: [PATCH 5/6] nohz: support PR_DATAPLANE_STRICT mode On May 8, 2015 11:44 PM, Chris Metcalf cmetc...@ezchip.com wrote: With QUIESCE mode, the task is in principle guaranteed not to be interrupted by the kernel, but only if it behaves. In particular, if it enters the kernel via system call, page fault, or any of a number of other synchronous traps, it may be unexpectedly exposed to long latencies. Add a simple flag that puts the process into a state where any such kernel entry is fatal. To allow the state to be entered and exited, we add an internal bit to current-dataplane_flags that is set when prctl() sets the flags. That way, when we are exiting the kernel after calling prctl() to forbid future kernel exits, we don't get immediately killed. Is there any reason this can't already be addressed in userspace using /proc/interrupts or perf_events? ISTM the real goal here is to detect when we screw up and fail to avoid an interrupt, and killing the task seems like overkill to me. Also, can we please stop further torturing the exit paths? So, I don't know if it is a practical suggestion or not, but would it better/easier to mark a pending signal on kernel entry for this case? The upsides I see is that the user gets her notification (killing the task or just logging the event in a signal handler) and hopefully since return to userspace with a pending signal is already handled we don't need new code in the exit path? Gilad N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a��� 0��h���i
RE: [PATCH 0/6] support dataplane mode for nohz_full
From: Mike Galbraith [mailto:umgwanakikb...@gmail.com] Sent: Saturday, May 09, 2015 10:20 AM To: Ingo Molnar Cc: Andrew Morton; Chris Metcalf; Steven Rostedt; Gilad Ben Yossef; Ingo Molnar; Peter Zijlstra; Rik van Riel; Tejun Heo; Frederic Weisbecker; Thomas Gleixner; Paul E. McKenney; Christoph Lameter; Srivatsa S. Bhat; linux-...@vger.kernel.org; linux-...@vger.kernel.org; linux- ker...@vger.kernel.org Subject: Re: [PATCH 0/6] support dataplane mode for nohz_full On Sat, 2015-05-09 at 09:05 +0200, Ingo Molnar wrote: * Andrew Morton a...@linux-foundation.org wrote: On Fri, 8 May 2015 19:11:10 -0400 Chris Metcalf cmetc...@ezchip.com wrote: On 5/8/2015 5:22 PM, Steven Rostedt wrote: On Fri, 8 May 2015 14:18:24 -0700 Andrew Morton a...@linux-foundation.org wrote: On Fri, 8 May 2015 13:58:41 -0400 Chris Metcalf cmetc...@ezchip.com wrote: A prctl() option (PR_SET_DATAPLANE) is added Dumb question: what does the term dataplane mean in this context? I can't see the relationship between those words and what this patch does. I was thinking the same thing. I haven't gotten around to searching DATAPLANE yet. I would assume we want a name that is more meaningful for what is happening. The text in the commit message and the 0/6 cover letter do try to explain the concept. The terminology comes, I think, from networking line cards, where the dataplane is the part of the application that handles all the fast path processing of network packets, and the control plane is the part that handles routing updates, etc., generally slow-path stuff. I've probably just been using the terms so long they seem normal to me. That said, what would be clearer? NO_HZ_STRICT as a superset of NO_HZ_FULL? Or move away from the NO_HZ terminology a bit; after all, we're talking about no interrupts of any kind, and maybe NO_HZ is too limited in scope? So, NO_INTERRUPTS? USERSPACE_ONLY? Or look to vendors who ship bare-metal runtimes and call it BARE_METAL? Borrow the Tilera marketing name and call it ZERO_OVERHEAD? Maybe BARE_METAL seems most plausible -- after DATAPLANE, to me, of course :-) 'baremetal' has uses in virtualization speak, so I think that would be confusing. I like NO_INTERRUPTS. Simple, direct. NO_HZ_PURE? Hm, coke light, coke zero... OS_LIGHT and OS_ZERO? LOL... you forgot OS_CLASSIC for backwards compatibility :-) How about TASK_SOLO? Yes, you are trying to achieve the least amount of interference but the bigger context is about monopolizing a single CPU for yourself. Anyway it is worth pointing out that while NO_HZ_FULL is very useful in conjunction with this turning the tick off is useful also if you have multiple tasks runnable (e.g. if you know you only need to context switch in 100 ms, why keep a periodic interrupt running?) even though we don't support it *right now*. It might be a good idea not to entangle these concepts too much. Gilad Gilad Ben-Yossef Chief Software Architect EZchip Technologies Ltd. 37 Israel Pollak Ave, Kiryat Gat 82025 ,Israel Tel: +972-4-959- ext. 576, Fax: +972-8-681-1483 Mobile: +972-52-826-0388, US Mobile: +1-973-826-0388 Email: gil...@ezchip.com, Web: http://www.ezchip.com N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a��� 0��h���i
Re: [PATCH 2/2] tracing: Fix for non-continuous cpu ids
On Tue, Jun 2, 2015 at 5:26 PM, Steven Rostedt rost...@goodmis.org wrote: On Tue, 2 Jun 2015 16:26:29 +0300 Gil Fruchter g...@ezchip.com wrote: Currently exception occures due to access beyond buffer_iter range while using index of cpu bigger than num_possible_cpus(). Below there is an example for such exception when we use cpus 0,1,16,17. I'm curious, what broken boxes have non-continuous cpu ids? The NPS-400 is a single SoC with 256 cores based on Synopsys Arc, each one is capable to work as up to 16 SMT hardware threads or logical cores. I gave a talk about it two years ago at Plumbers/LinuxCon. Most of the Linux support is in the process being up-streamed via the Arc architecture maintainer (the patch set is at https://github.com/EZchip/linux) but there are some none architecture specific patches that we just happen to run into like this one. As to the why, well, the CPU ids has a mapping to the topology - which cluster (NUMA node), core and SMT thread you are looking at. The thing is, the number of SMT threads per core is configurable, and you can hot plug down a core, reconfigure it for a different number of SMT thread and hot them back in. So the empty slots are possible logical cores that don't happen to be active at the time, but could hotplug up later on. I hope this makes some sense. Since the NPS-400 is not generally available yet I wouldn't worry too much about stable kernel for now... Thanks, Gilad Ben-Yossef -- Gilad Ben-Yossef Chief Coffee Drinker gi...@benyossef.com Israel Cell: +972-52-8260388 US Cell: +1-973-8260388 http://benyossef.com If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog? -- Jean-Baptiste Queru -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 06/14] task_isolation: provide strict mode configurable signal
Hi Andy, Thank for the feedback. > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Wednesday, October 21, 2015 9:53 PM > To: Gilad Ben Yossef > Cc: Chris Metcalf; Steven Rostedt; Ingo Molnar; Peter Zijlstra; Andrew > Morton; Rik van Riel; Tejun Heo; Frederic Weisbecker; Thomas Gleixner; Paul > E. McKenney; Christoph Lameter; Viresh Kumar; Catalin Marinas; Will Deacon; > linux-...@vger.kernel.org; Linux API; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v8 06/14] task_isolation: provide strict mode > configurable signal > > >> >> On Tue, 20 Oct 2015 16:36:04 -0400 > >> >> Chris Metcalf <cmetc...@ezchip.com> wrote: > >> >> > >> >>> Allow userspace to override the default SIGKILL delivered > >> >>> when a task_isolation process in STRICT mode does a syscall > >> >>> or otherwise synchronously enters the kernel. > >> >>> > > > >> > > >> > It doesn't map SIGKILL to some other signal unconditionally. It just > >> > allows > >> > the "hey, you broke the STRICT contract and entered the kernel" signal > >> > to be something besides the default SIGKILL. > >> > > >> > > > > > >> > >> I still dislike this thing. It seems like a debugging feature being > >> implemented using signals instead of existing APIs. I *still* don't > >> see why perf can't be used to accomplish your goal. > >> > > > > It is not (just) a debugging feature. There are workloads were not > performing an action is much preferred to being late. > > > > Consider the following artificial but representative scenario: a task > > running > in strict isolation is controlling a radiotherapy alpha emitter. > > The code runs in a tight event loop, reading an MMIO register with location > data, making some calculation and in response writing an > > MMIO register that triggers the alpha emitter. As a safety measure, each > trigger is for a specific very short time frame - the alpha emitter > > auto stops. > > > > The code has a strict assumption that no more than X cycles pass between > reading the value and the response and the system is built in > > such a way that as long as the code has mastery of the CPU the assumption > holds true. If something breaks this assumption (unplanned > > context switch to kernel), what you want to do is just stop place > > rather than fire the alpha emitter X nanoseconds too late. > > > > This feature lets you say: if the "contract" of isolation is broken, > > notify/kill > me at once. > > That's a fair point. It's risky, though, for quite a few reasons. > > 1. If someone builds an alpha emitter like this, they did it wrong. > The kernel should write a trigger *and* a timestamp to the hardware > and the hardware should trigger at the specified time if the time is > in the future and throw an error if it's in the past. If you need to > check that you made the deadline, check the actual desired condition > (did you meat the deadline?) not a proxy (did the signal fire?). > As I wrote above it is an *artificial* scenario. Yes, hardware and systems can be designed better, but they are not always are and in these kind of systems, you really do want to have double or triple checks. Knowing such systems, even IF the hardware was designed as you specified (and I agree it should!) you would still add the software protection. > 2. This strict mode thing isn't exhaustive. It's missing, at least, > coverage for NMI, MCE, and SMI. Sure, you can think that you've > disabled all NMI sources, you can try to remember to set the > appropriate boot flag that panics on MCE (and hope that you don't get > screwed by broadcast MCE on Intel systems before it got fixed > (Skylake? Is the fix even available in a released chip?), and, for > SMI, good luck... You are right - it isn't exhaustive. It is one piece in a bigger puzzle. Many of the other bits are platform specific and some of them have been dealt with on the platform that care about these things. Yes, we don't have dark magic to detect SMIs. Is that a reason to penalize platforms where there is no such thing as SMI? > 3. You haven't dealt with IPIs. The TLB flush code in particular > seems like it will break all your assumptions. > But we have - in the general context. Consider this patch set from 2012 - https://lwn.net/Articles/479510/ Not finished for sure. But what we have is now useful enough that it is used in the real world for different workloads on different platforms, from packet processing, through HPC to high frequency trading. >
RE: [PATCH v8 08/14] nohz_full: allow disabling the 1Hz minimum tick at boot
> From: Frederic Weisbecker [mailto:fweis...@gmail.com] > > This option also allows easy testing of nohz_full and task-isolation > > modes to determine what functionality needs to be implemented, > > and what possibly-spurious timer interrupts are scheduled when > > the basic 1Hz tick has been turned off. > > > > Signed-off-by: Chris Metcalf> > There have been proposals to disable/tune the 1 Hz tick via debugfs which > I Nacked because once you give such an opportunity to the users, they > will use that hack and never fix the real underlying issue. > > For the same reasons, I'm sorry but I have to Nack this proposal as well. > > If this is for development or testing purpose, > scheduler_max_tick_deferment() is > easily commented out. The problem with the latter is that it is much easier get back to one of the poor^H^H^H^H brave souls that are willing to risk their kittens testing this stuff for us saying: "can you please boot without this boot option and let me know if that behavior you were complaining about still happens?" rather than "can you please go to this and that line in the source file and un-comment it and re-compile and see if it still happens?" I hope this makes more sense. Thinking about it, it's probably a good idea to taint the kernel when this option is set as well. Thanks, Gilad -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 06/14] task_isolation: provide strict mode configurable signal
> From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Wednesday, October 21, 2015 4:43 AM > To: Chris Metcalf > Subject: Re: [PATCH v8 06/14] task_isolation: provide strict mode > configurable signal > > On Tue, Oct 20, 2015 at 6:30 PM, Chris Metcalf> wrote: > > On 10/20/2015 8:56 PM, Steven Rostedt wrote: > >> > >> On Tue, 20 Oct 2015 16:36:04 -0400 > >> Chris Metcalf wrote: > >> > >>> Allow userspace to override the default SIGKILL delivered > >>> when a task_isolation process in STRICT mode does a syscall > >>> or otherwise synchronously enters the kernel. > >>> > > > > It doesn't map SIGKILL to some other signal unconditionally. It just allows > > the "hey, you broke the STRICT contract and entered the kernel" signal > > to be something besides the default SIGKILL. > > > > > I still dislike this thing. It seems like a debugging feature being > implemented using signals instead of existing APIs. I *still* don't > see why perf can't be used to accomplish your goal. > It is not (just) a debugging feature. There are workloads were not performing an action is much preferred to being late. Consider the following artificial but representative scenario: a task running in strict isolation is controlling a radiotherapy alpha emitter. The code runs in a tight event loop, reading an MMIO register with location data, making some calculation and in response writing an MMIO register that triggers the alpha emitter. As a safety measure, each trigger is for a specific very short time frame - the alpha emitter auto stops. The code has a strict assumption that no more than X cycles pass between reading the value and the response and the system is built in such a way that as long as the code has mastery of the CPU the assumption holds true. If something breaks this assumption (unplanned context switch to kernel), what you want to do is just stop place rather than fire the alpha emitter X nanoseconds too late. This feature lets you say: if the "contract" of isolation is broken, notify/kill me at once. For code where isolation is important, the correctness of a calculation is dependent on timing. It's like you would accept the kernel to kill a task if it read from an unmapped virtual address rather than returning garbage data. With an isolated task, the right data acted on later than you think is garbage just the same. I hope this sheds some light on the issue. Thanks, Gilad
[PATCH 0/2] scatterlist: clean up
Ran into these two clean up / optimization opportunities while chasing an unrelated bug. Gilad Ben-Yossef (2): scatterlist: reorder compound boolean expression scatterlist: do not disable IRQs in sg_copy_buffer lib/scatterlist.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) -- 2.1.4
[PATCH 2/2] scatterlist: do not disable IRQs in sg_copy_buffer
Commit 50bed2e2862a ("sg: disable interrupts inside sg_copy_buffer") introduced disabling interrupts in sg_copy_buffer() since atomic uses of miter required it due to use of kmap_atomic(). However, as commit 8290e2d2dcbf ("scatterlist: atomic sg_mapping_iter() no longer needs disabled IRQs") acknowledges disabling interrupts is no longer needed for calls to kmap_atomic() and therefore unneeded for miter ops either, so remove it from sg_copy_buffer(). Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- lib/scatterlist.c | 4 1 file changed, 4 deletions(-) diff --git a/lib/scatterlist.c b/lib/scatterlist.c index 393920f..c6cf822 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -651,7 +651,6 @@ size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf, { unsigned int offset = 0; struct sg_mapping_iter miter; - unsigned long flags; unsigned int sg_flags = SG_MITER_ATOMIC; if (to_buffer) @@ -664,8 +663,6 @@ size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf, if (!sg_miter_skip(, skip)) return false; - local_irq_save(flags); - while ((offset < buflen) && sg_miter_next()) { unsigned int len; @@ -681,7 +678,6 @@ size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf, sg_miter_stop(); - local_irq_restore(flags); return offset; } EXPORT_SYMBOL(sg_copy_buffer); -- 2.1.4
[PATCH 1/2] scatterlist: reorder compound boolean expression
Test the cheaper boolean expression with no side effects first. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- lib/scatterlist.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/scatterlist.c b/lib/scatterlist.c index 004fc70..393920f 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -666,7 +666,7 @@ size_t sg_copy_buffer(struct scatterlist *sgl, unsigned int nents, void *buf, local_irq_save(flags); - while (sg_miter_next() && offset < buflen) { + while ((offset < buflen) && sg_miter_next()) { unsigned int len; len = min(miter.length, buflen - offset); -- 2.1.4
Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt
On Tue, Feb 7, 2017 at 12:35 PM, Binoy Jayan <binoy.ja...@linaro.org> wrote: > === > dm-crypt optimization for larger block sizes > === > > Currently, the iv generation algorithms are implemented in dm-crypt.c. The > goal > is to move these algorithms from the dm layer to the kernel crypto layer by > implementing them as template ciphers so they can be used in relation with > algorithms like aes, and with multiple modes like cbc, ecb etc. As part of > this > patchset, the iv-generation code is moved from the dm layer to the crypto > layer > and adapt the dm-layer to send a whole 'bio' (as defined in the block layer) > at a time. Each bio contains the in memory representation of physically > contiguous disk blocks. Since the bio itself may not be contiguous in main > memory, the dm layer sets up a chained scatterlist of these blocks split into > physically contiguous segments in memory so that DMA can be performed. ... > Binoy Jayan (1): > crypto: Add IV generation algorithms > > drivers/md/dm-crypt.c | 1894 > ++-- > include/crypto/geniv.h | 47 ++ > 2 files changed, 1402 insertions(+), 539 deletions(-) > create mode 100644 include/crypto/geniv.h Ran Bonnie++ on it last night (Luks mode, plain64, Qemu Virt platform Arm64) and it works just fine. Tested-by: Gilad Ben-Yossef <gi...@benyossef.com> Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [RFC PATCH v3] crypto: Add IV generation algorithms
On Thu, Jan 19, 2017 at 6:42 AM, Binoy Jayan <binoy.ja...@linaro.org> wrote: > Hi Gilad, > > On 18 January 2017 at 20:51, Gilad Ben-Yossef <gi...@benyossef.com> wrote: >> I have some review comments and a bug report - > > Thank you very much for testing this on ARM and for the comments. My pleasure. Thanks for writing the code :-) > >>> + rinfo.iv_sector = ctx->cc_sector; >>> + rinfo.nents = nents; >>> + rinfo.iv = iv; >>> + >>> + skcipher_request_set_crypt(req, dmreq->sg_in, dmreq->sg_out, >> >> Also, where do the scatterlist src2 and dst2 that you use >> sg_set_page() get sg_init_table() called on? >> I couldn't figure it out... > > Thank you pointing this out. I missed out to add sg_init_table(src2, 1) > and sg_init_table(dst2, 1), but sg_set_page is used in geniv_iter_block. > This is probably the reason for the panic on ARM platform. However it > ran fine under qemu-x86. May be I should setup an arm platform too > for testing. I tried adding sg_init_table() where I thought it was appropriate and it didn't resolve the issue. For what it's worth, my guess is that the difference between our setups is not related to Arm but to other options or the storage I'm using. Are you using cryptd? Thanks, Gilad > > Regards, > Binoy -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [RFC PATCH v3] crypto: Add IV generation algorithms
Hi Binoy, On Wed, Jan 18, 2017 at 11:40 AM, Binoy Jayan <binoy.ja...@linaro.org> wrote: > Currently, the iv generation algorithms are implemented in dm-crypt.c. > The goal is to move these algorithms from the dm layer to the kernel > crypto layer by implementing them as template ciphers so they can be > implemented in hardware for performance. As part of this patchset, the > iv-generation code is moved from the dm layer to the crypto layer and > adapt the dm-layer to send a whole 'bio' (as defined in the block layer) > at a time. Each bio contains an in memory representation of physically > contiguous disk blocks. The dm layer sets up a chained scatterlist of > these blocks split into physically contiguous segments in memory so that > DMA can be performed. Also, the key management code is moved from dm layer > to the cryto layer since the key selection for encrypting neighboring > sectors depend on the keycount. > > Synchronous crypto requests to encrypt/decrypt a sector are processed > sequentially. Asynchronous requests if processed in parallel, are freed > in the async callback. The dm layer allocates space for iv. The hardware > implementations can choose to make use of this space to generate their IVs > sequentially or allocate it on their own. > Interface to the crypto layer - include/crypto/geniv.h > > Signed-off-by: Binoy Jayan <binoy.ja...@linaro.org> > --- I have some review comments and a bug report - > */ > -static int crypt_convert(struct crypt_config *cc, > -struct convert_context *ctx) > + > +static int crypt_convert_bio(struct crypt_config *cc, > +struct convert_context *ctx) > { > + unsigned int cryptlen, n1, n2, nents, i = 0, bytes = 0; > + struct skcipher_request *req; > + struct dm_crypt_request *dmreq; > + struct geniv_req_info rinfo; > + struct bio_vec bv_in, bv_out; > int r; > + u8 *iv; > > atomic_set(>cc_pending, 1); > + crypt_alloc_req(cc, ctx); > + > + req = ctx->req; > + dmreq = dmreq_of_req(cc, req); > + iv = iv_of_dmreq(cc, dmreq); > > - while (ctx->iter_in.bi_size && ctx->iter_out.bi_size) { > + n1 = bio_segments(ctx->bio_in); > + n2 = bio_segments(ctx->bio_in); I'm pretty sure this needs to be n2 = bio_segments(ctx->bio_out); > + nents = n1 > n2 ? n1 : n2; > + nents = nents > MAX_SG_LIST ? MAX_SG_LIST : nents; > + cryptlen = ctx->iter_in.bi_size; > > - crypt_alloc_req(cc, ctx); > + DMDEBUG("dm-crypt:%s: segments:[in=%u, out=%u] bi_size=%u\n", > + bio_data_dir(ctx->bio_in) == WRITE ? "write" : "read", > + n1, n2, cryptlen); > > > - /* There was an error while processing the request. */ > - default: > - atomic_dec(>cc_pending); > - return r; > - } > + sg_set_page(>sg_in[i], bv_in.bv_page, bv_in.bv_len, > + bv_in.bv_offset); > + sg_set_page(>sg_out[i], bv_out.bv_page, bv_out.bv_len, > + bv_out.bv_offset); > + > + bio_advance_iter(ctx->bio_in, >iter_in, bv_in.bv_len); > + bio_advance_iter(ctx->bio_out, >iter_out, bv_out.bv_len); > + > + bytes += bv_in.bv_len; > + i++; > } > > - return 0; > + DMDEBUG("dm-crypt: Processed %u of %u bytes\n", bytes, cryptlen); > + > + rinfo.is_write = bio_data_dir(ctx->bio_in) == WRITE; Please consider wrapping the above boolean expression in parenthesis. > + rinfo.iv_sector = ctx->cc_sector; > + rinfo.nents = nents; > + rinfo.iv = iv; > + > + skcipher_request_set_crypt(req, dmreq->sg_in, dmreq->sg_out, Also, where do the scatterlist src2 and dst2 that you use sg_set_page() get sg_init_table() called on? I couldn't figure it out... Last but not least, when performing the following sequence on Arm64 (on latest Qemu Virt platform) - 1. cryptsetup luksFormat fs3.img 2. cryptsetup open --type luks fs3.img croot 3. mke2fs /dev/mapper/croot [ fs3.img is a 16MB file for loopback ] The attached kernel panic happens. The same does not occur without the patch. Let me know if you need any additional information to recreate it. I've tried to debug it a little but did not came up with anything useful aside from above review notes. Thanks! -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your d
Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt
On Wed, Mar 1, 2017 at 3:21 PM, Ondrej Mosnacek <omosn...@redhat.com> wrote: > 2017-03-01 13:42 GMT+01:00 Gilad Ben-Yossef <gi...@benyossef.com>: > > Wouldn't adopting a bulk request API (something like what I tried to > do here [1]) that allows users to supply multiple messages, each with > their own IV, fulfill this purpose? That way, we wouldn't need to > introduce any new modes into Crypto API and the drivers/accelerators > would only need to provide bulk implementations of common modes > (xts(aes), cbc(aes), ...) to provide better performance for dm-crypt > (and possibly other users, too). > > I'm not sure how exactly these crypto accelerators work, but wouldn't > it help if the drivers simply get more messages (in our case sectors) > in a single call? I wonder, would (efficiently) supporting such a > scheme require changes in the HW itself or could it be achieved just > by modifying driver code (let's say specifically for your CryptoCell > accelerator)? > > [1] https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg23007.html > >From a general perspective - that is things are expect to be true not just for CryptoCell but for most HW crypto engines, you want two things - for the HW engine to be able to burst work for a long time and than rest for a long time vs. a stop and go scheme (engine utilization) and for the average IO transaction to be relatively long (bus utilization) So, a big cluster size i.e. Milan's proposal) works great - you get both. Submitting a series of sequential small clusters where the HW can calculate the IV (e.g. Binoy's proposal) works great if the HW supports it - you get both. A batched series of small clusters + IV is less favorable - if your HW engines has lots of parallel context processing (this is expensive for HW) you might enjoy engine utilization but the bus utilization will be low - lots of small transactions. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt
On Tue, Feb 28, 2017 at 11:05 PM, Milan Broz <gmazyl...@gmail.com> wrote: > > On 02/22/2017 07:12 AM, Binoy Jayan wrote: > > > > I was wondering if this is near to be ready for submission (apart from > > the testmgr.c > > changes) or I need to make some changes to make it similar to the IPSec > > offload? > > I just tried this and except it registers the IV for every new device again, > it works... > (After a while you have many duplicate entries in /proc/crypto.) > > But I would like to see some summary why such a big patch is needed in the > first place. > (During an internal discussions seems that people are already lost in mails > and > patches here, so Ondra promised me to send some summary mail soon here.) > > IIRC the first initial problem was dmcrypt performance on some embedded > crypto processors that are not able to cope with small crypto requests > effectively. > > > Do you have some real performance numbers that proves that such a patch is > adequate? > > I would really like to see the performance issue fixed but I am really not > sure > this approach works for everyone. It would be better to avoid repeating this > exercise later. > IIRC Ondra's "bulk" mode, despite rejected, shows that there is a potential > to speedup things even for crypt drivers that do not support own IV > generators. > AFAIK the problem that we are trying to solve is that if the IV is generated outside the crypto API domain than you are forced to have an invocation of the crypto API per each block because you need to provide the IV for each block. By putting the IV generation responsibility in the Crypto API we open the way to do a single invocation of the crypto API for a whole sequence of blocks. For software implementation of XTS this doesn't matter much - but for hardware based XTS providers that can do the IV generation themselves it's the difference between a sequence of small invocation, with all the overhead that that entails and a single big invocatio - and this can be big. This lead some vendors to ship hacked up versions of dm-crypt to match the specific crypto hardware they were using, or so I've heard at least - didn't see the code myself. I believe Binoy is trying to address this in a generic upstream worthy way instead. Anyway, you are only supposed to see s difference when using a hardware based XTS provider algo that supports IV generation. > I like the patch is now contained inside dmcrypt, but it still exposes IVs > that > are designed just for old, insecure, compatibility-only containers. > > I really do not think every compatible crap must be accessible through crypto > API. > (I wrote the dmcrypt lrw and tcw compatibility IVs and I would never do that > this way > if I know it is accessible outside of dmcrypt internals...) > Even the ESSIV is something that was born to fix predictive IVs (CBC > watermarking > attacks) for disk encryption only, no reason to expose it outside of disk > encryption. > The point is that you have more than one implementation of these "compatible crap" - the software implementation that you wrote and potentially multiple hardware implementations and putting this in the crypto API domain is the way to abstract this so you use the one that works best of your platform. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt
On Wed, Mar 1, 2017 at 11:29 AM, Milan Broz <gmazyl...@gmail.com> wrote: > > On 03/01/2017 09:30 AM, Gilad Ben-Yossef wrote: > > On Tue, Feb 28, 2017 at 11:05 PM, Milan Broz <gmazyl...@gmail.com> wrote: > >> > >> On 02/22/2017 07:12 AM, Binoy Jayan wrote: > >>> > >>> I was wondering if this is near to be ready for submission (apart from > >>> the testmgr.c > >>> changes) or I need to make some changes to make it similar to the IPSec > >>> offload? > >> > >> I just tried this and except it registers the IV for every new device > >> again, it works... > >> (After a while you have many duplicate entries in /proc/crypto.) > >> > >> But I would like to see some summary why such a big patch is needed in the > >> first place. > >> (During an internal discussions seems that people are already lost in > >> mails and > >> patches here, so Ondra promised me to send some summary mail soon here.) > >> > >> IIRC the first initial problem was dmcrypt performance on some embedded > >> crypto processors that are not able to cope with small crypto requests > >> effectively. > >> > >> > >> Do you have some real performance numbers that proves that such a patch is > >> adequate? > >> > >> I would really like to see the performance issue fixed but I am really not > >> sure > >> this approach works for everyone. It would be better to avoid repeating > >> this exercise later. > >> IIRC Ondra's "bulk" mode, despite rejected, shows that there is a potential > >> to speedup things even for crypt drivers that do not support own IV > >> generators. > >> > > > > AFAIK the problem that we are trying to solve is that if the IV is > > generated outside the crypto API > > domain than you are forced to have an invocation of the crypto API per > > each block because you > > need to provide the IV for each block. > > > > By putting the IV generation responsibility in the Crypto API we open > > the way to do a single invocation > > of the crypto API for a whole sequence of blocks. > > Sure, but this is theory. Does it really work on some hw already? > Do you have performance measurements or comparison? I'm working on up streaming a driver for Arm CryptoCell that supports this and working offline to get Binoy a board to test this with. Alas, shipping crypto HW has its fair share of regulatory challenges... :-) I can certainly understand if you don't wont to take the patch until we have results with dm-crypt itself but the difference between 8 separate invocation of the engine for 512 bytes of XTS and a single invocation for 4KB are pretty big. >From what I know of HW engines I'd be surprised if this is in any way unique to CryptoCell. > > For software implementation of XTS this doesn't matter much - but for > > hardware based XTS providers > > It is not only embedded crypto, we have some more reports in the past > that 512B sectors are not ideal even for other systems. > (IIRC it was also with AES-NI that represents really big group of users). I never said anything about embedded :-) It really is an observation about overhead of context switches between dm-crypt and whatever/wherever you handle crypto - be it an off CPU hardware engine or a bunch of parallel kernel threads running on other cores. You really want to burst as much as possible. > > > This lead some vendors to ship hacked up versions of dm-crypt to match > > the specific crypto hardware > > they were using, or so I've heard at least - didn't see the code myself. > > I saw few version of that. There was a very hacky way to provide > request-based dmcrypt > (see old "Introduce the request handling for dm-crypt" thread on dm-devel). > This is not the acceptable way but definitely it points to the same problem. > > > I believe Binoy is trying to address this in a generic upstream worthy > > way instead. > > IIRC the problem is performance, if we can solve it by some generic way, > good, but for now it seems to be a big change and just hope it helps later... > I see what you're saying. We need number to back this up. > > Anyway, you are only supposed to see s difference when using a > > hardware based XTS provider algo > > that supports IV generation. > > > >> I like the patch is now contained inside dmcrypt, but it still exposes IVs > >> that > >> are designed just for old, insecure, compatibility-only containers. > >> > >> I really do not think every compatible crap must b
Re: [RFC PATCH v2] crypto: Add IV generation algorithms
Hi Binoy, On Tue, Dec 13, 2016 at 02:19:09PM +0530, Binoy Jayan wrote: > Currently, the iv generation algorithms are implemented in dm-crypt.c. > The goal is to move these algorithms from the dm layer to the kernel > crypto layer by implementing them as template ciphers so they can be > implemented in hardware for performance. As part of this patchset, the > iv-generation code is moved from the dm layer to the crypto layer and > adapt the dm-layer to send a whole 'bio' (as defined in the block layer) > at a time. Each bio contains the in memory representation of physically > contiguous disk blocks. The dm layer sets up a chained scatterlist of > these blocks split into physically contiguous segments in memory so that > DMA can be performed. The iv generation algorithms implemented in geniv.c > include plain, plain64, essiv, benbi, null, lmk and tcw. > Good idea. I wanted to test the patch but alas it does not apply cleanly. You seem to have a blank line at the end of files and other small transgressions that makes checkpatch grumpy. Also... > > Not-signed-off-by: Binoy Jayan <binoy.ja...@linaro.org> What is Not-signed-off-by ? :-) Thanks, Gilad Ben-Yossef
[PATCH v3 RESEND] dm: switch dm-verity to async hash crypto API
Use of the synchronous digest API limits dm-verity to using pure CPU based algorithm providers and rules out the use of off CPU algorithm providers which are normally asynchronous by nature, potentially freeing CPU cycles. This can reduce performance per Watt in situations such as during boot time when a lot of concurrent file accesses are made to the protected volume. Move DM_VERITY to the asynchronous hash API. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Tested-by: Milan Broz <gmazyl...@gmail.com> CC: Eric Biggers <ebigge...@gmail.com> CC: Ondrej Mosnáček <omosnacek+linux-cry...@gmail.com> --- Resending since I did not get any feedback beyond Milan confirmation that the latest version passes his simple sanity test. The patch was tested by me on an Armv7 based dual core Zynq ZC706 development board with SHA256-asm, SHA256-neon synchronous providers with no visible degradation of performance, with cryptd based asynchronous versions of the same and with an off tree Arm CryptoCell asynchronous provider. Changes from v2: - Use completion to potentially wait also on crypto_ahash_init() as it may finish asynchronously as well in some drivers, such as cryptd, as discovered by Milan Broz. - Use sg_init_one() where apropriate as pointed out by Milan Broz. Changes from v1: - Use a 0 mask to allocate crypto alg indicating we welcome async algo providers, as suggested by Ondrej Mosnáček. - Fix use of un-initialized completion when using async provider for IO blocks hashing - Pass flag indicating we are OK with crypto provider backlog - Re-factor checking for need to wait into a common function drivers/md/dm-verity-fec.c| 4 +- drivers/md/dm-verity-target.c | 202 +- drivers/md/dm-verity.h| 23 +++-- 3 files changed, 158 insertions(+), 71 deletions(-) drivers/md/dm-verity-fec.c| 4 +- drivers/md/dm-verity-target.c | 201 +- drivers/md/dm-verity.h| 23 +++-- 3 files changed, 157 insertions(+), 71 deletions(-) diff --git a/drivers/md/dm-verity-fec.c b/drivers/md/dm-verity-fec.c index 0f0eb8a..dab98fe 100644 --- a/drivers/md/dm-verity-fec.c +++ b/drivers/md/dm-verity-fec.c @@ -188,7 +188,7 @@ static int fec_decode_bufs(struct dm_verity *v, struct dm_verity_fec_io *fio, static int fec_is_erasure(struct dm_verity *v, struct dm_verity_io *io, u8 *want_digest, u8 *data) { - if (unlikely(verity_hash(v, verity_io_hash_desc(v, io), + if (unlikely(verity_hash(v, verity_io_hash_req(v, io), data, 1 << v->data_dev_block_bits, verity_io_real_digest(v, io return 0; @@ -397,7 +397,7 @@ static int fec_decode_rsb(struct dm_verity *v, struct dm_verity_io *io, } /* Always re-validate the corrected block against the expected hash */ - r = verity_hash(v, verity_io_hash_desc(v, io), fio->output, + r = verity_hash(v, verity_io_hash_req(v, io), fio->output, 1 << v->data_dev_block_bits, verity_io_real_digest(v, io)); if (unlikely(r < 0)) diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c index 7335d8a..97de961 100644 --- a/drivers/md/dm-verity-target.c +++ b/drivers/md/dm-verity-target.c @@ -93,81 +93,123 @@ static sector_t verity_position_at_level(struct dm_verity *v, sector_t block, } /* - * Wrapper for crypto_shash_init, which handles verity salting. + * Callback function for asynchrnous crypto API completion notification */ -static int verity_hash_init(struct dm_verity *v, struct shash_desc *desc) +static void verity_op_done(struct crypto_async_request *base, int err) { - int r; + struct verity_result *res = (struct verity_result *)base->data; - desc->tfm = v->tfm; - desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP; + if (err == -EINPROGRESS) + return; - r = crypto_shash_init(desc); + res->err = err; + complete(>completion); +} - if (unlikely(r < 0)) { - DMERR("crypto_shash_init failed: %d", r); - return r; - } +/* + * Wait for async crypto API callback + */ +static inline int verity_complete_op(struct verity_result *res, int ret) +{ + switch (ret) { + case 0: + break; - if (likely(v->version >= 1)) { - r = crypto_shash_update(desc, v->salt, v->salt_size); + case -EINPROGRESS: + case -EBUSY: + ret = wait_for_completion_interruptible(>completion); + if (!ret) + ret = res->err; + reinit_completion(>completion); + break; - if (unlikely(r < 0)) { - DMERR("crypto_shash_upd
Re: [PATCH 0/4] staging: add ccree crypto driver
On Tue, Apr 18, 2017 at 6:39 PM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote: >> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware >> accelerators. It is supported by a long lived series of out of tree >> drivers, which I am now in the process of unifying and upstreaming. >> This is the first drop, supporting the new CryptoCell 712 REE. >> >> The code still needs some cleanup before maturing to a proper >> upstream driver, which I am in the process of doing. However, >> as discussion of some of the capabilities of the hardware and >> its application to some dm-crypt and dm-verity features recently >> took place I though it is better to do this in the open via the >> staging tree. >> >> A Git repository based off of Linux 4.11-rc7 is available at >> https://github.com/gby/linux.git branch ccree. >> >> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> >> CC: Binoy Jayan <binoy.ja...@linaro.org> >> CC: Ofir Drang <ofir.dr...@arm.com> >> >> Gilad Ben-Yossef (4): >> staging: add ccree crypto driver >> staging: ccree: add TODO list >> staging: ccree: add devicetree bindings >> MAINTAINERS: add Gilad BY as maintainer for ccree > > We can't do much without a real patch, sorry. Digging in random git > trees doesn't work :( > > I can't take this as-is, I need patches. Got it. I'll break the driver to a series of patches and post them . Thanks for the feedback. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [PATCH 0/4] staging: add ccree crypto driver
On Tue, Apr 18, 2017 at 6:43 PM, Mark Rutland <mark.rutl...@arm.com> wrote: ... >> >> >> >> The code still needs some cleanup before maturing to a proper >> >> upstream driver, which I am in the process of doing. However, >> >> as discussion of some of the capabilities of the hardware and >> >> its application to some dm-crypt and dm-verity features recently >> >> took place I though it is better to do this in the open via the >> >> staging tree. >> >> >> >> A Git repository based off of Linux 4.11-rc7 is available at >> >> https://github.com/gby/linux.git branch ccree. >> >> ... >> >> .../devicetree/bindings/crypto/arm-cryptocell.txt | 23 + >> > >> > I'm interested in looking at the DT binding, but for some reason I've >> > only recevied the cover letter so far. >> > >> > Are my mail servers being particularly slow today, or has something gone >> > wrong with the Cc list for the remaining patches? >> > >> >> Thanks a bunch for the willingness to review this. >> >> My apologies for not communicating this clearly enough - Since it is >> an pre-existing driver it is rather big. >> I did not want to inflict a 600K patch on the mailing list so opted to >> provide a git repo instead, as stated in the >> email. > > Should this have been a [GIT PULL], then? > > I was confused by the [PATCH 0/4]. Yes, it should have. Sorry about that. > >> The patch in question is here: >> https://github.com/gby/linux/commit/12d92e0bc19aa9bbd891cdab765eaaeabe0b6967 >> >> Do let me know if you prefer that I will send at least the smaller >> patch file via email in the normal fashion. > > Sending patches is the usual way to have things reviewed. Linking to an > external site doesn't fit with the workflows of everyone. > > If you wish to have patches reviewed, please send them as patches in the > usual fashion. Thanks for the feedback. I will break the driver up and post patches in the normal fashion. > > I will note that on the patch you linked, the compatible string is far > too vague, per common conventions. Per your description above, that > should be something like "arm,cryptocell-712-ree" (and each variant > should have its own string). Got it. Will change. Thanks for the review even in this unconventional form :-) ! > > I don't have documentation to hand to attempt to review the rest. > > I would appreciate if you could ensure that the DT binding was reviewed > as part of the staging TODOs. That needs to follow the usual DT review > process of sending patches to the list. Until that has occurred, it > shouldn't be in Documentation/devicetree/. Of course, will do. Thanks, Gilad > > Thanks, > Mark. -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH v2 5/9] staging: ccree: add AEAD support
Add CryptoCell AEAD support Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |1 + drivers/staging/ccree/Makefile |2 +- drivers/staging/ccree/cc_crypto_ctx.h | 21 + drivers/staging/ccree/ssi_aead.c | 2826 drivers/staging/ccree/ssi_aead.h | 120 ++ drivers/staging/ccree/ssi_buffer_mgr.c | 899 ++ drivers/staging/ccree/ssi_buffer_mgr.h |4 + drivers/staging/ccree/ssi_driver.c | 11 + drivers/staging/ccree/ssi_driver.h |4 + 9 files changed, 3887 insertions(+), 1 deletion(-) create mode 100644 drivers/staging/ccree/ssi_aead.c create mode 100644 drivers/staging/ccree/ssi_aead.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 3fff040..2d11223 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -5,6 +5,7 @@ config CRYPTO_DEV_CCREE select CRYPTO_HASH select CRYPTO_BLKCIPHER select CRYPTO_DES + select CRYPTO_AEAD select CRYPTO_AUTHENC select CRYPTO_SHA1 select CRYPTO_MD5 diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index 89afe9a..b9285c0 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/cc_crypto_ctx.h b/drivers/staging/ccree/cc_crypto_ctx.h index f198779..743461f 100644 --- a/drivers/staging/ccree/cc_crypto_ctx.h +++ b/drivers/staging/ccree/cc_crypto_ctx.h @@ -263,6 +263,27 @@ struct drv_ctx_cipher { (CC_AES_KEY_SIZE_MAX/sizeof(uint32_t))]; }; +/* authentication and encryption with associated data class */ +struct drv_ctx_aead { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_AES */ + enum drv_cipher_mode mode; + enum drv_crypto_direction direction; + uint32_t key_size; /* numeric value in bytes */ + uint32_t nonce_size; /* nonce size (octets) */ + uint32_t header_size; /* finit additional data size (octets) */ + uint32_t text_size; /* finit text data size (octets) */ + uint32_t tag_size; /* mac size, element of {4, 6, 8, 10, 12, 14, 16} */ + /* block_state1/2 is the AES engine block state */ + uint8_t block_state[CC_AES_BLOCK_SIZE]; + uint8_t mac_state[CC_AES_BLOCK_SIZE]; /* MAC result */ + uint8_t nonce[CC_AES_BLOCK_SIZE]; /* nonce buffer */ + uint8_t key[CC_AES_KEY_SIZE_MAX]; + /* reserve to end of allocated context size */ + uint32_t reserved[CC_DRV_CTX_SIZE_WORDS - 8 - + 3 * (CC_AES_BLOCK_SIZE/sizeof(uint32_t)) - + CC_AES_KEY_SIZE_MAX/sizeof(uint32_t)]; +}; + /***/ /* MESSAGE BASED CONTEXTS **/ /***/ diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c new file mode 100644 index 000..1d2890e --- /dev/null +++ b/drivers/staging/ccree/ssi_aead.c @@ -0,0 +1,2826 @@ +/* + * Copyright (C) 2012-2016 ARM Limited or its affiliates. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "ssi_config.h" +#include "ssi_driver.h" +#include "ssi_buffer_mgr.h" +#include "ssi_aead.h" +#include "ssi_request_mgr.h" +#include "ssi_hash.h" +#include "ssi_sysfs.h" +#include "ssi_sram_mgr.h" + +#define template_aead template_u.aead + +#define MAX_AEAD_SETKEY_SEQ 12 +#define MAX_AEAD_PROCESS_SEQ 23 + +#define MAX_HMAC_DIGEST_SIZE (SHA256_DIGEST_SIZE) +#define MAX_HMAC_BLOCK_SIZE (SHA256_BLOCK_SIZE) + +#define AES_CCM_RFC4309_NONCE_SIZE 3 +#define MAX_NONCE_SIZE CTR_RFC36
[PATCH v2 9/9] MAINTAINERS: add Gilad BY as ccree maintainer
I work for Arm on maintaining the TrustZone CryptoCell driver. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 676c139..f21caa1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3066,6 +3066,13 @@ F: drivers/net/ieee802154/cc2520.c F: include/linux/spi/cc2520.h F: Documentation/devicetree/bindings/net/ieee802154/cc2520.txt +CCREE ARM TRUSTZONE CRYPTOCELL 700 REE DRIVER +M: Gilad Ben-Yossef <gi...@benyossef.com> +L: linux-cry...@vger.kernel.org +S: Supported +F: drivers/staging/ccree/ +W: https://developer.arm.com/products/system-ip/trustzone-cryptocell/cryptocell-700-family + CEC DRIVER M: Hans Verkuil <hans.verk...@cisco.com> L: linux-me...@vger.kernel.org -- 2.1.4
[PATCH v2 3/9] staging: ccree: add skcipher support
Add CryptoCell skcipher support Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |8 + drivers/staging/ccree/Makefile |2 +- drivers/staging/ccree/cc_crypto_ctx.h | 21 + drivers/staging/ccree/ssi_buffer_mgr.c | 147 drivers/staging/ccree/ssi_buffer_mgr.h | 16 + drivers/staging/ccree/ssi_cipher.c | 1440 drivers/staging/ccree/ssi_cipher.h | 88 ++ drivers/staging/ccree/ssi_driver.c | 14 + drivers/staging/ccree/ssi_driver.h | 30 + 9 files changed, 1765 insertions(+), 1 deletion(-) create mode 100644 drivers/staging/ccree/ssi_cipher.c create mode 100644 drivers/staging/ccree/ssi_cipher.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index a528a99..3fff040 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -3,11 +3,19 @@ config CRYPTO_DEV_CCREE depends on CRYPTO_HW && OF && HAS_DMA default n select CRYPTO_HASH + select CRYPTO_BLKCIPHER + select CRYPTO_DES + select CRYPTO_AUTHENC select CRYPTO_SHA1 select CRYPTO_MD5 select CRYPTO_SHA256 select CRYPTO_SHA512 select CRYPTO_HMAC + select CRYPTO_AES + select CRYPTO_CBC + select CRYPTO_ECB + select CRYPTO_CTR + select CRYPTO_XTS help Say 'Y' to enable a driver for the Arm TrustZone CryptoCell C7xx. Currently only the CryptoCell 712 REE is supported. diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index f94e225..21a80d5 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/cc_crypto_ctx.h b/drivers/staging/ccree/cc_crypto_ctx.h index fedf259..f198779 100644 --- a/drivers/staging/ccree/cc_crypto_ctx.h +++ b/drivers/staging/ccree/cc_crypto_ctx.h @@ -242,6 +242,27 @@ struct drv_ctx_hmac { CC_DIGEST_SIZE_MAX - CC_HMAC_BLOCK_SIZE_MAX]; }; +struct drv_ctx_cipher { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_AES */ + enum drv_cipher_mode mode; + enum drv_crypto_direction direction; + enum drv_crypto_key_type crypto_key_type; + enum drv_crypto_padding_type padding_type; + uint32_t key_size; /* numeric value in bytes */ + uint32_t data_unit_size; /* required for XTS */ + /* block_state is the AES engine block state. + * It is used by the host to pass IV or counter at initialization. + * It is used by SeP for intermediate block chaining state and for + * returning MAC algorithms results. */ + uint8_t block_state[CC_AES_BLOCK_SIZE]; + uint8_t key[CC_AES_KEY_SIZE_MAX]; + uint8_t xex_key[CC_AES_KEY_SIZE_MAX]; + /* reserve to end of allocated context size */ + uint32_t reserved[CC_DRV_CTX_SIZE_WORDS - 7 - + CC_AES_BLOCK_SIZE/sizeof(uint32_t) - 2 * + (CC_AES_KEY_SIZE_MAX/sizeof(uint32_t))]; +}; + /***/ /* MESSAGE BASED CONTEXTS **/ /***/ diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index 5144eaa..a0fafa9 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -28,6 +28,7 @@ #include "ssi_buffer_mgr.h" #include "cc_lli_defs.h" +#include "ssi_cipher.h" #include "ssi_hash.h" #define LLI_MAX_NUM_OF_DATA_ENTRIES 128 @@ -517,6 +518,152 @@ static inline int ssi_ahash_handle_curr_buf(struct device *dev, return 0; } +void ssi_buffer_mgr_unmap_blkcipher_request( + struct device *dev, + void *ctx, + unsigned int ivsize, + struct scatterlist *src, + struct scatterlist *dst) +{ + struct blkcipher_req_ctx *req_ctx = (struct blkcipher_req_ctx *)ctx; + + if (likely(req_ctx->gen_ctx.iv_dma_addr != 0)) { + SSI_LOG_DEBUG("Unmapped iv: iv_dma_addr=0x%llX iv_size=%u\n", + (unsigned long long)req_ctx->gen_ctx.iv_dma_addr, + ivsize); + SSI_RESTORE_DMA_ADDR_TO_48BIT(req_ctx->gen_ctx.iv_dma_addr); + dma_unmap_single(dev, req_ctx->gen_ctx.iv_dma_addr, +ivsize, +DMA_TO_DEVICE); + } + /* Release pool */ + if (req_ctx->dma
[PATCH v2 6/9] staging: ccree: add FIPS support
Add FIPS mode support to CryptoCell driver Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |9 + drivers/staging/ccree/Makefile |1 + drivers/staging/ccree/ssi_aead.c|6 + drivers/staging/ccree/ssi_cipher.c | 52 + drivers/staging/ccree/ssi_driver.c | 19 +- drivers/staging/ccree/ssi_driver.h |2 + drivers/staging/ccree/ssi_fips.c| 65 ++ drivers/staging/ccree/ssi_fips.h| 70 ++ drivers/staging/ccree/ssi_fips_data.h | 315 ++ drivers/staging/ccree/ssi_fips_ext.c| 96 ++ drivers/staging/ccree/ssi_fips_ll.c | 1681 +++ drivers/staging/ccree/ssi_fips_local.c | 369 +++ drivers/staging/ccree/ssi_fips_local.h | 77 ++ drivers/staging/ccree/ssi_hash.c| 21 +- drivers/staging/ccree/ssi_request_mgr.c |2 + 15 files changed, 2783 insertions(+), 2 deletions(-) create mode 100644 drivers/staging/ccree/ssi_fips.c create mode 100644 drivers/staging/ccree/ssi_fips.h create mode 100644 drivers/staging/ccree/ssi_fips_data.h create mode 100644 drivers/staging/ccree/ssi_fips_ext.c create mode 100644 drivers/staging/ccree/ssi_fips_ll.c create mode 100644 drivers/staging/ccree/ssi_fips_local.c create mode 100644 drivers/staging/ccree/ssi_fips_local.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 2d11223..ae62704 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -24,6 +24,15 @@ config CRYPTO_DEV_CCREE cryptographic operations on the system REE. If unsure say Y. +config CCREE_FIPS_SUPPORT + bool "Turn on CryptoCell 7XX REE FIPS mode support" + depends on CRYPTO_DEV_CCREE + default n + help + Say 'Y' to enable support for FIPS compliant mode by the + CCREE driver. + If unsure say N. + config CCREE_DISABLE_COHERENT_DMA_OPS bool "Disable Coherent DMA operations for the CCREE driver" depends on CRYPTO_DEV_CCREE diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index b9285c0..44f3e3e 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-$(CCREE_FIPS_SUPPORT) += ssi_fips.o ssi_fips_ll.o ssi_fips_ext.o ssi_fips_local.o diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index 1d2890e..3ab958b 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -36,6 +36,7 @@ #include "ssi_hash.h" #include "ssi_sysfs.h" #include "ssi_sram_mgr.h" +#include "ssi_fips_local.h" #define template_aead template_u.aead @@ -153,6 +154,8 @@ static int ssi_aead_init(struct crypto_aead *tfm) container_of(alg, struct ssi_crypto_alg, aead_alg); SSI_LOG_DEBUG("Initializing context @%p for %s\n", ctx, crypto_tfm_alg_name(&(tfm->base))); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); + /* Initialize modes in instance */ ctx->cipher_mode = ssi_alg->cipher_mode; ctx->flow_mode = ssi_alg->flow_mode; @@ -572,6 +575,7 @@ ssi_aead_setkey(struct crypto_aead *tfm, const u8 *key, unsigned int keylen) SSI_LOG_DEBUG("Setting key in context @%p for %s. key=%p keylen=%u\n", ctx, crypto_tfm_alg_name(crypto_aead_tfm(tfm)), key, keylen); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); /* STAT_PHASE_0: Init and sanity checks */ START_CYCLE_COUNT(); @@ -699,6 +703,7 @@ static int ssi_aead_setauthsize( { struct ssi_aead_ctx *ctx = crypto_aead_ctx(authenc); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); /* Unsupported auth. sizes */ if ((authsize == 0) || (authsize >crypto_aead_maxauthsize(authenc))) { @@ -2006,6 +2011,7 @@ static int ssi_aead_process(struct aead_request *req, enum drv_crypto_direction SSI_LOG_DEBUG("%s context=%p req=%p iv=%p src=%p src_ofs=%d dst=%p dst_ofs=%d cryptolen=%d\n", ((direct==DRV_CRYPTO_DIRECTION_ENCRYPT)?"Encrypt":"Decrypt"), ctx, req, req->iv, sg_virt(req->src), req->src->offset, sg_virt(req->dst), req->dst->offset, req->cryptlen); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); /* STAT_PHASE_0: Init and sanity checks */ START_CYCLE_COUNT(); diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 2e4ce90..e8a4071 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -31,6 +31,7 @@ #include "ssi_cipher.h" #include "ssi_r
[PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver
Arm TrustZone CryptoCell 700 is a family of cryptographic hardware accelerators. It is supported by a long lived series of out of tree drivers, which I am now in the process of unifying and upstreaming. This is the first drop, supporting the new CryptoCell 712 REE. The code still needs some cleanup before maturing to a proper upstream driver, which I am in the process of doing. However, as discussion of some of the capabilities of the hardware and its application to some dm-crypt and dm-verity features recently took place I though it is better to do this in the open via the staging tree. A Git repository based off of Linux 4.11-rc7 is also available at https://github.com/gby/linux.git branch ccree_v2 for those inclined. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> CC: Binoy Jayan <binoy.ja...@linaro.org> CC: Ofir Drang <ofir.dr...@arm.com> CC: Stuart Yoder <stuart.yo...@arm.com> Changes from v1: - Broke up patch set into smaller units for mailing list review as per Greg KH's indication. - Changed DT binding compatible tag as per Mark Rutland suggestion. - Moved DT binding document inside the staging directory and added DT binding review to TODO list as per Mark Rutland's request. Many thanks to all reviewers :-) Gilad Ben-Yossef (9): staging: ccree: introduce CryptoCell HW driver staging: ccree: add ahash support staging: ccree: add skcipher support staging: ccree: add IV generation support staging: ccree: add AEAD support staging: ccree: add FIPS support staging: ccree: add TODO list staging: ccree: add DT bindings for Arm CryptoCell MAINTAINERS: add Gilad BY as ccree maintainer MAINTAINERS|7 + drivers/staging/Kconfig|2 + drivers/staging/Makefile |2 +- .../devicetree/bindings/crypto/arm-cryptocell.txt | 27 + drivers/staging/ccree/Kconfig | 43 + drivers/staging/ccree/Makefile |3 + drivers/staging/ccree/TODO | 28 + drivers/staging/ccree/bsp.h| 21 + drivers/staging/ccree/cc_bitops.h | 62 + drivers/staging/ccree/cc_crypto_ctx.h | 299 +++ drivers/staging/ccree/cc_hal.h | 35 + drivers/staging/ccree/cc_hw_queue_defs.h | 603 + drivers/staging/ccree/cc_lli_defs.h| 57 + drivers/staging/ccree/cc_pal_log.h | 188 ++ drivers/staging/ccree/cc_pal_log_plat.h| 33 + drivers/staging/ccree/cc_pal_types.h | 97 + drivers/staging/ccree/cc_pal_types_plat.h | 29 + drivers/staging/ccree/cc_regs.h| 106 + drivers/staging/ccree/dx_crys_kernel.h | 180 ++ drivers/staging/ccree/dx_env.h | 224 ++ drivers/staging/ccree/dx_host.h| 155 ++ drivers/staging/ccree/dx_reg_base_host.h | 34 + drivers/staging/ccree/dx_reg_common.h | 26 + drivers/staging/ccree/hash_defs.h | 78 + drivers/staging/ccree/hw_queue_defs_plat.h | 43 + drivers/staging/ccree/ssi_aead.c | 2832 drivers/staging/ccree/ssi_aead.h | 120 + drivers/staging/ccree/ssi_buffer_mgr.c | 1876 + drivers/staging/ccree/ssi_buffer_mgr.h | 105 + drivers/staging/ccree/ssi_cipher.c | 1503 +++ drivers/staging/ccree/ssi_cipher.h | 89 + drivers/staging/ccree/ssi_config.h | 61 + drivers/staging/ccree/ssi_driver.c | 557 drivers/staging/ccree/ssi_driver.h | 228 ++ drivers/staging/ccree/ssi_fips.c | 65 + drivers/staging/ccree/ssi_fips.h | 70 + drivers/staging/ccree/ssi_fips_data.h | 315 +++ drivers/staging/ccree/ssi_fips_ext.c | 96 + drivers/staging/ccree/ssi_fips_ll.c| 1681 drivers/staging/ccree/ssi_fips_local.c | 369 +++ drivers/staging/ccree/ssi_fips_local.h | 77 + drivers/staging/ccree/ssi_hash.c | 2751 +++ drivers/staging/ccree/ssi_hash.h | 101 + drivers/staging/ccree/ssi_ivgen.c | 301 +++ drivers/staging/ccree/ssi_ivgen.h | 72 + drivers/staging/ccree/ssi_pm.c | 150 ++ drivers/staging/ccree/ssi_pm.h | 46 + drivers/staging/ccree/ssi_pm_ext.c | 60 + drivers/staging/ccree/ssi_pm_ext.h | 33 + drivers/staging/ccree/ssi_request_mgr.c| 713 + drivers/staging/ccree/ssi_request_mgr.h| 60 + drivers/staging/ccree/ssi_sram_mgr.c | 138 + drivers/staging/ccree/ssi_sram_mgr.h | 80
[PATCH v2 7/9] staging: ccree: add TODO list
Add TODO list for moving out of staging tree for ccree crypto driver Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/TODO | 28 1 file changed, 28 insertions(+) create mode 100644 drivers/staging/ccree/TODO diff --git a/drivers/staging/ccree/TODO b/drivers/staging/ccree/TODO new file mode 100644 index 000..3f1d61d --- /dev/null +++ b/drivers/staging/ccree/TODO @@ -0,0 +1,28 @@ + + +* +* * +* Arm Trust Zone CryptoCell REE Linux driver upstreaming TODO items* +* * +* + +ccree specific items +a.k.a stuff fixing for this driver to move out of staging +~ + +1. Move to using Crypto Engine to handle backlog queueing. +2. Remove synchronous algorithm support leftovers. +3. Separate platform specific code for FIPS and power management into separate platform modules. +4. Drop legacy kernel support code. +5. Move most (all?) #ifdef CONFIG into inline functions. +6. Remove all unused definitions. +7. Re-factor to accomediate newer/older HW revisions besides the 712. +8. Handle the many checkpatch errors. +9. Implement ahash import/export correctly. +10. Go through a proper review of DT bindings and sysfs ABI + +Kernel infrastructure items +a.k.a stuff we either neither need to fix in the kernel or understand what we're doing wrong + +1. ahash import/export context has a PAGE_SIZE/8 size limit. We need more. +2. Crypto Engine seems to be built for HW with hardware queue depth of 1, we have 600++. -- 2.1.4
[PATCH v2 8/9] staging: ccree: add DT bindings for Arm CryptoCell
This adds DT bindings for the Arm TrustZone CryptoCell cryptographic accelerator IP. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- .../devicetree/bindings/crypto/arm-cryptocell.txt | 27 ++ 1 file changed, 27 insertions(+) create mode 100644 drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt diff --git a/drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt new file mode 100644 index 000..2ea6517 --- /dev/null +++ b/drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt @@ -0,0 +1,27 @@ +Arm TrustZone CryptoCell cryptographic accelerators + +Required properties: +- compatible: must be "arm,cryptocell-712-ree". +- reg: shall contain base register location and length. + Typically length is 0x1. +- interrupts: shall contain the interrupt for the device. + +Optional properties: +- interrupt-parent: can designate the interrupt controller the + device interrupt is connected to, if needed. +- clocks: may contain the clock handling the device, if needed. +- power-domains: may contain a reference to the PM domain, if applicable. + + +Examples: + +Zynq FPGA device + + + arm_cc7x: arm_cc7x@8000 { + compatible = "arm,cryptocell-712-ree"; + interrupt-parent = <>; + interrupts = < 0 30 4 >; + reg = < 0x8000 0x1 >; + }; + -- 2.1.4
Re: [PATCH v2 0/9] staging: ccree: add Arm TrustZone CryptoCell REE driver
On Thu, Apr 20, 2017 at 4:30 PM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Thu, Apr 20, 2017 at 04:12:54PM +0300, Gilad Ben-Yossef wrote: >> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware >> accelerators. It is supported by a long lived series of out of tree >> drivers, which I am now in the process of unifying and upstreaming. >> This is the first drop, supporting the new CryptoCell 712 REE. >> >> The code still needs some cleanup before maturing to a proper >> upstream driver, which I am in the process of doing. However, >> as discussion of some of the capabilities of the hardware and >> its application to some dm-crypt and dm-verity features recently >> took place I though it is better to do this in the open via the >> staging tree. >> >> A Git repository based off of Linux 4.11-rc7 is also available at >> https://github.com/gby/linux.git branch ccree_v2 for those inclined. > > If you want this in staging, I'll be glad to take it, but note then you > can't work off of an external repo, as syncing the two is almost > impossible and more work than you want to go through. Once it's in the staging tree I don't need a separate repo. It was only useful so long as I did not have an upstream tree to point people to. > > So, as long as this builds properly, want me to queue these up in my > tree? Yes, please. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH v2 4/9] staging: ccree: add IV generation support
Add CryptoCell IV hardware generation support. This patch adds the needed support to drive the HW but does not expose the ability via the kernel crypto API yet. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Makefile | 2 +- drivers/staging/ccree/ssi_buffer_mgr.c | 2 + drivers/staging/ccree/ssi_cipher.c | 11 ++ drivers/staging/ccree/ssi_cipher.h | 1 + drivers/staging/ccree/ssi_driver.c | 9 + drivers/staging/ccree/ssi_driver.h | 7 + drivers/staging/ccree/ssi_ivgen.c | 301 drivers/staging/ccree/ssi_ivgen.h | 72 drivers/staging/ccree/ssi_pm.c | 2 + drivers/staging/ccree/ssi_request_mgr.c | 33 +++- 10 files changed, 438 insertions(+), 2 deletions(-) create mode 100644 drivers/staging/ccree/ssi_ivgen.c create mode 100644 drivers/staging/ccree/ssi_ivgen.h diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index 21a80d5..89afe9a 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index a0fafa9..6a9c964 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -534,6 +534,7 @@ void ssi_buffer_mgr_unmap_blkcipher_request( SSI_RESTORE_DMA_ADDR_TO_48BIT(req_ctx->gen_ctx.iv_dma_addr); dma_unmap_single(dev, req_ctx->gen_ctx.iv_dma_addr, ivsize, +req_ctx->is_giv ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE); } /* Release pool */ @@ -587,6 +588,7 @@ int ssi_buffer_mgr_map_blkcipher_request( req_ctx->gen_ctx.iv_dma_addr = dma_map_single(dev, (void *)info, ivsize, + req_ctx->is_giv ? DMA_BIDIRECTIONAL: DMA_TO_DEVICE); if (unlikely(dma_mapping_error(dev, req_ctx->gen_ctx.iv_dma_addr))) { diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 01467e8..2e4ce90 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -819,6 +819,13 @@ static int ssi_blkcipher_process( areq, desc, _len); + /* do we need to generate IV? */ + if (req_ctx->is_giv == true) { + ssi_req.ivgen_dma_addr[0] = req_ctx->gen_ctx.iv_dma_addr; + ssi_req.ivgen_dma_addr_len = 1; + /* set the IV size (8/16 B long)*/ + ssi_req.ivgen_size = ivsize; + } END_CYCLE_COUNT(ssi_req.op_type, STAT_PHASE_2); /* STAT_PHASE_3: Lock HW and push sequence */ @@ -901,6 +908,7 @@ static int ssi_sblkcipher_encrypt(struct blkcipher_desc *desc, unsigned int ivsize = crypto_blkcipher_ivsize(blk_tfm); req_ctx->backup_info = desc->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, dst, src, nbytes, desc->info, ivsize, NULL, DRV_CRYPTO_DIRECTION_ENCRYPT); } @@ -916,6 +924,7 @@ static int ssi_sblkcipher_decrypt(struct blkcipher_desc *desc, unsigned int ivsize = crypto_blkcipher_ivsize(blk_tfm); req_ctx->backup_info = desc->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, dst, src, nbytes, desc->info, ivsize, NULL, DRV_CRYPTO_DIRECTION_DECRYPT); } @@ -948,6 +957,7 @@ static int ssi_ablkcipher_encrypt(struct ablkcipher_request *req) unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); req_ctx->backup_info = req->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_ENCRYPT); } @@ -960,6 +970,7 @@ static int ssi_ablkcipher_decrypt(struct ablkcipher_request *req) unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); req_ctx->backup_info = req->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_DECRYPT); } diff --git a/drivers/staging/ccree/ssi_cipher.h b/drivers/staging/ccree/ssi_cipher.h index 511800f1..d1a98f9 100644 --- a/driv
[PATCH v2 2/9] staging: ccree: add ahash support
Add CryptoCell async. hash and HMAC support. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |6 + drivers/staging/ccree/Makefile |2 +- drivers/staging/ccree/cc_crypto_ctx.h | 22 + drivers/staging/ccree/hash_defs.h | 78 + drivers/staging/ccree/ssi_buffer_mgr.c | 311 +++- drivers/staging/ccree/ssi_buffer_mgr.h |6 + drivers/staging/ccree/ssi_driver.c | 11 +- drivers/staging/ccree/ssi_driver.h |4 +- drivers/staging/ccree/ssi_hash.c | 2732 drivers/staging/ccree/ssi_hash.h | 101 ++ drivers/staging/ccree/ssi_pm.c |4 + 11 files changed, 3263 insertions(+), 14 deletions(-) create mode 100644 drivers/staging/ccree/hash_defs.h create mode 100644 drivers/staging/ccree/ssi_hash.c create mode 100644 drivers/staging/ccree/ssi_hash.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 0f723d7..a528a99 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -2,6 +2,12 @@ config CRYPTO_DEV_CCREE tristate "Support for ARM TrustZone CryptoCell C7XX family of Crypto accelerators" depends on CRYPTO_HW && OF && HAS_DMA default n + select CRYPTO_HASH + select CRYPTO_SHA1 + select CRYPTO_MD5 + select CRYPTO_SHA256 + select CRYPTO_SHA512 + select CRYPTO_HMAC help Say 'Y' to enable a driver for the Arm TrustZone CryptoCell C7xx. Currently only the CryptoCell 712 REE is supported. diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index 972af69..f94e225 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/cc_crypto_ctx.h b/drivers/staging/ccree/cc_crypto_ctx.h index 8b8aea2..fedf259 100644 --- a/drivers/staging/ccree/cc_crypto_ctx.h +++ b/drivers/staging/ccree/cc_crypto_ctx.h @@ -220,6 +220,28 @@ struct drv_ctx_generic { } __attribute__((__may_alias__)); +struct drv_ctx_hash { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_HASH */ + enum drv_hash_mode mode; + uint8_t digest[CC_DIGEST_SIZE_MAX]; + /* reserve to end of allocated context size */ + uint8_t reserved[CC_CTX_SIZE - 2 * sizeof(uint32_t) - + CC_DIGEST_SIZE_MAX]; +}; + +/* drv_ctx_hmac should have the same structure as drv_ctx_hash except + k0, k0_size fields */ +struct drv_ctx_hmac { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_HMAC */ + enum drv_hash_mode mode; + uint8_t digest[CC_DIGEST_SIZE_MAX]; + uint32_t k0[CC_HMAC_BLOCK_SIZE_MAX/sizeof(uint32_t)]; + uint32_t k0_size; + /* reserve to end of allocated context size */ + uint8_t reserved[CC_CTX_SIZE - 3 * sizeof(uint32_t) - + CC_DIGEST_SIZE_MAX - CC_HMAC_BLOCK_SIZE_MAX]; +}; + /***/ /* MESSAGE BASED CONTEXTS **/ /***/ diff --git a/drivers/staging/ccree/hash_defs.h b/drivers/staging/ccree/hash_defs.h new file mode 100644 index 000..0cd6909 --- /dev/null +++ b/drivers/staging/ccree/hash_defs.h @@ -0,0 +1,78 @@ +/* + * Copyright (C) 2012-2016 ARM Limited or its affiliates. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + * You should have received a copy of the GNU General Public License along + * with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +#ifndef _HASH_DEFS_H__ +#define _HASH_DEFS_H__ + +#include "cc_crypto_ctx.h" + +/* this files provides definitions required for hash engine drivers */ +#ifndef CC_CONFIG_HASH_SHA_512_SUPPORTED +#define SEP_HASH_LENGTH_WORDS 2 +#else +#define SEP_HASH_LENGTH_WORDS 4 +#endif + +#ifdef BIG__ENDIAN +#define OPAD_CURRENT_LENGTH 0x4000, 0x , 0x, 0x +#define HASH_LARVAL_MD5 0x76543210, 0xFEDCBA98, 0x89ABCDEF, 0x01234567 +#define HASH_LARVAL_SHA1 0xF0E1D2C3, 0x76543210, 0x
Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver
On Thu, Apr 20, 2017 at 4:33 PM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: > On Thu, Apr 20, 2017 at 04:12:55PM +0300, Gilad Ben-Yossef wrote: >> +++ b/drivers/staging/ccree/bsp.h >> @@ -0,0 +1,21 @@ >> +/* >> + * Copyright (C) 2012-2016 ARM Limited or its affiliates. >> + * >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the >> Free >> + * Software Foundation; either version 2 of the License, or (at your option) >> + * any later version. > > Oh, I have to ask, do you really mean "any later version" here and > elsewhere? > > If so, then your MODULE_LICENSE() marking is wrong, please fix that up, > or fix up the license text, I can't take incompatible ones without > getting angry emails from legal people sent to me... > Thanks for noticing this. The copyright + license notice is a boilerplate I got from the powers that be here. I'll consult internally what is the proper action. I don't want to make legal mad either... :-) Thanks again, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [PATCH 0/4] staging: add ccree crypto driver
Hi Mark, On Tue, Apr 18, 2017 at 6:13 PM, Mark Rutland <mark.rutl...@arm.com> wrote: > Hi, > > On Tue, Apr 18, 2017 at 05:07:50PM +0300, Gilad Ben-Yossef wrote: >> Arm TrustZone CryptoCell 700 is a family of cryptographic hardware >> accelerators. It is supported by a long lived series of out of tree >> drivers, which I am now in the process of unifying and upstreaming. >> This is the first drop, supporting the new CryptoCell 712 REE. >> >> The code still needs some cleanup before maturing to a proper >> upstream driver, which I am in the process of doing. However, >> as discussion of some of the capabilities of the hardware and >> its application to some dm-crypt and dm-verity features recently >> took place I though it is better to do this in the open via the >> staging tree. >> >> A Git repository based off of Linux 4.11-rc7 is available at >> https://github.com/gby/linux.git branch ccree. >> >> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> >> CC: Binoy Jayan <binoy.ja...@linaro.org> >> CC: Ofir Drang <ofir.dr...@arm.com> >> >> Gilad Ben-Yossef (4): >> staging: add ccree crypto driver >> staging: ccree: add TODO list >> staging: ccree: add devicetree bindings >> MAINTAINERS: add Gilad BY as maintainer for ccree >> >> .../devicetree/bindings/crypto/arm-cryptocell.txt | 23 + > > I'm interested in looking at the DT binding, but for some reason I've > only recevied the cover letter so far. > > Are my mail servers being particularly slow today, or has something gone > wrong with the Cc list for the remaining patches? > Thanks a bunch for the willingness to review this. My apologies for not communicating this clearly enough - Since it is an pre-existing driver it is rather big. I did not want to inflict a 600K patch on the mailing list so opted to provide a git repo instead, as stated in the email. The patch in question is here: https://github.com/gby/linux/commit/12d92e0bc19aa9bbd891cdab765eaaeabe0b6967 Do let me know if you prefer that I will send at least the smaller patch file via email in the normal fashion. Many thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH 0/4] staging: add ccree crypto driver
Arm TrustZone CryptoCell 700 is a family of cryptographic hardware accelerators. It is supported by a long lived series of out of tree drivers, which I am now in the process of unifying and upstreaming. This is the first drop, supporting the new CryptoCell 712 REE. The code still needs some cleanup before maturing to a proper upstream driver, which I am in the process of doing. However, as discussion of some of the capabilities of the hardware and its application to some dm-crypt and dm-verity features recently took place I though it is better to do this in the open via the staging tree. A Git repository based off of Linux 4.11-rc7 is available at https://github.com/gby/linux.git branch ccree. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> CC: Binoy Jayan <binoy.ja...@linaro.org> CC: Ofir Drang <ofir.dr...@arm.com> Gilad Ben-Yossef (4): staging: add ccree crypto driver staging: ccree: add TODO list staging: ccree: add devicetree bindings MAINTAINERS: add Gilad BY as maintainer for ccree .../devicetree/bindings/crypto/arm-cryptocell.txt | 23 + MAINTAINERS|8 + drivers/staging/Kconfig|2 + drivers/staging/Makefile |2 +- drivers/staging/ccree/Kconfig | 52 + drivers/staging/ccree/Makefile |3 + drivers/staging/ccree/TODO | 27 + drivers/staging/ccree/bsp.h| 21 + drivers/staging/ccree/cc_bitops.h | 62 + drivers/staging/ccree/cc_crypto_ctx.h | 299 +++ drivers/staging/ccree/cc_hal.h | 35 + drivers/staging/ccree/cc_hw_queue_defs.h | 603 + drivers/staging/ccree/cc_lli_defs.h| 57 + drivers/staging/ccree/cc_pal_log.h | 188 ++ drivers/staging/ccree/cc_pal_log_plat.h| 33 + drivers/staging/ccree/cc_pal_types.h | 97 + drivers/staging/ccree/cc_pal_types_plat.h | 29 + drivers/staging/ccree/cc_regs.h| 106 + drivers/staging/ccree/dx_crys_kernel.h | 180 ++ drivers/staging/ccree/dx_env.h | 224 ++ drivers/staging/ccree/dx_host.h| 155 ++ drivers/staging/ccree/dx_reg_base_host.h | 34 + drivers/staging/ccree/dx_reg_common.h | 26 + drivers/staging/ccree/hash_defs.h | 78 + drivers/staging/ccree/hw_queue_defs_plat.h | 43 + drivers/staging/ccree/ssi_aead.c | 2832 drivers/staging/ccree/ssi_aead.h | 120 + drivers/staging/ccree/ssi_buffer_mgr.c | 1876 + drivers/staging/ccree/ssi_buffer_mgr.h | 105 + drivers/staging/ccree/ssi_cipher.c | 1503 +++ drivers/staging/ccree/ssi_cipher.h | 89 + drivers/staging/ccree/ssi_config.h | 61 + drivers/staging/ccree/ssi_driver.c | 557 drivers/staging/ccree/ssi_driver.h | 228 ++ drivers/staging/ccree/ssi_fips.c | 65 + drivers/staging/ccree/ssi_fips.h | 70 + drivers/staging/ccree/ssi_fips_data.h | 315 +++ drivers/staging/ccree/ssi_fips_ext.c | 96 + drivers/staging/ccree/ssi_fips_ll.c| 1681 drivers/staging/ccree/ssi_fips_local.c | 369 +++ drivers/staging/ccree/ssi_fips_local.h | 77 + drivers/staging/ccree/ssi_hash.c | 2751 +++ drivers/staging/ccree/ssi_hash.h | 101 + drivers/staging/ccree/ssi_ivgen.c | 301 +++ drivers/staging/ccree/ssi_ivgen.h | 72 + drivers/staging/ccree/ssi_pm.c | 150 ++ drivers/staging/ccree/ssi_pm.h | 46 + drivers/staging/ccree/ssi_pm_ext.c | 60 + drivers/staging/ccree/ssi_pm_ext.h | 33 + drivers/staging/ccree/ssi_request_mgr.c| 713 + drivers/staging/ccree/ssi_request_mgr.h| 60 + drivers/staging/ccree/ssi_sram_mgr.c | 138 + drivers/staging/ccree/ssi_sram_mgr.h | 80 + drivers/staging/ccree/ssi_sysfs.c | 440 +++ drivers/staging/ccree/ssi_sysfs.h | 54 + 55 files changed, 17429 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/crypto/arm-cryptocell.txt create mode 100644 drivers/staging/ccree/Kconfig create mode 100644 drivers/staging/ccree/Makefile create mode 100644 drivers/staging/ccree/TODO create mode 100644 drivers/staging/ccree/bsp.h create mode 100644 drivers/staging/ccree/cc_bitops.h create mode 100644 drivers/staging/ccree/cc_crypto_ctx.h create mode 100644 drivers/staging/ccree/cc_hal.h create m
Re: [PATCH v2 1/9] staging: ccree: introduce CryptoCell HW driver
Hi, [ Re sending with all recipients this time ... ] On Thu, Apr 20, 2017 at 5:01 PM, Greg Kroah-Hartman <gre...@linuxfoundation.org> wrote: >> > Oh, I have to ask, do you really mean "any later version" here and >> > elsewhere? >> > >> > If so, then your MODULE_LICENSE() marking is wrong, please fix that up, >> > or fix up the license text, I can't take incompatible ones without >> > getting angry emails from legal people sent to me... >> > >> >> Thanks for noticing this. >> >> The copyright + license notice is a boilerplate I got from the powers >> that be here. >> >> I'll consult internally what is the proper action. I don't want to >> make legal mad either... :-) > > Ok, I'll drop this patch series then, and wait for an updated one with > this fixed up. This issue, along with some others pointed by reviewers, are fixed in v3 of the patch set. I will be happy if you choose to take it into the staging tree and will continue to work to cut down the TODO list. Thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH v3 13/15] staging: ccree: fix array_size.cocci warnings
From: kbuild test robot <l...@intel.com> drivers/staging/ccree/ssi_sysfs.c:319:34-35: WARNING: Use ARRAY_SIZE drivers/staging/ccree/ssi_sysfs.c:429:34-35: WARNING: Use ARRAY_SIZE Use ARRAY_SIZE instead of dividing sizeof array with sizeof an element Semantic patch information: This makes an effort to find cases where ARRAY_SIZE can be used such as where there is a division of sizeof the array by the sizeof its first element or by any indexed element or the element type. It replaces the division of the two sizeofs by ARRAY_SIZE. Generated by: scripts/coccinelle/misc/array_size.cocci Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Signed-off-by: Fengguang Wu <fengguang...@intel.com> --- drivers/staging/ccree/ssi_sysfs.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/staging/ccree/ssi_sysfs.c b/drivers/staging/ccree/ssi_sysfs.c index 6db7573..7c514c1 100644 --- a/drivers/staging/ccree/ssi_sysfs.c +++ b/drivers/staging/ccree/ssi_sysfs.c @@ -316,7 +316,7 @@ static ssize_t ssi_sys_help_show(struct kobject *kobj, int i=0, offset = 0; offset += scnprintf(buf + offset, PAGE_SIZE - offset, "Usage:\n"); - for ( i = 0; i < (sizeof(help_str)/sizeof(help_str[0])); i+=2) { + for ( i = 0; i < ARRAY_SIZE(help_str); i+=2) { offset += scnprintf(buf + offset, PAGE_SIZE - offset, "%s\t\t%s\n", help_str[i], help_str[i+1]); } return offset; @@ -426,8 +426,7 @@ int ssi_sysfs_init(struct kobject *sys_dev_obj, struct ssi_drvdata *drvdata) /* Initialize top directory */ retval = sys_init_dir(_top_dir, drvdata, sys_dev_obj, "cc_info", ssi_sys_top_level_attrs, - sizeof(ssi_sys_top_level_attrs) / - sizeof(struct kobj_attribute)); + ARRAY_SIZE(ssi_sys_top_level_attrs)); return retval; } -- 2.1.4
[PATCH v3 10/15] staging: ccree: remove useless NULL test of field
Remove kbuild test robot reported NULL check for a struct field address. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Reported-by: kbuild test robot <fengguang...@intel.com> --- drivers/staging/ccree/ssi_buffer_mgr.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index 0140199..af50904 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -755,9 +755,7 @@ void ssi_buffer_mgr_unmap_aead_request( AES_BLOCK_SIZE, DMA_TO_DEVICE); } - if (_ctx->ccm_adata_sg != NULL) - dma_unmap_sg(dev, _ctx->ccm_adata_sg, - 1, DMA_TO_DEVICE); + dma_unmap_sg(dev, _ctx->ccm_adata_sg, 1, DMA_TO_DEVICE); } if (areq_ctx->gen_ctx.iv_dma_addr != 0) { SSI_RESTORE_DMA_ADDR_TO_48BIT(areq_ctx->gen_ctx.iv_dma_addr); -- 2.1.4
[PATCH v3 12/15] staging: ccree: fix semicolon.cocci warnings
From: kbuild test robot <l...@intel.com> drivers/staging/ccree/ssi_request_mgr.c:623:3-4: Unneeded semicolon Remove unneeded semicolon. Generated by: scripts/coccinelle/misc/semicolon.cocci Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Signed-off-by: Fengguang Wu <fengguang...@intel.com> --- drivers/staging/ccree/ssi_request_mgr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/ccree/ssi_request_mgr.c b/drivers/staging/ccree/ssi_request_mgr.c index 42ab2b1..522bd62 100644 --- a/drivers/staging/ccree/ssi_request_mgr.c +++ b/drivers/staging/ccree/ssi_request_mgr.c @@ -653,7 +653,7 @@ static void comp_handler(unsigned long devarg) /* Avoid race with above clear: Test completion counter once more */ request_mgr_handle->axi_completed += CC_REG_FLD_GET(CRY_KERNEL, AXIM_MON_COMP, VALUE, CC_HAL_READ_REGISTER(AXIM_MON_BASE_OFFSET)); - }; + } } /* after verifing that there is nothing to do, Unmask AXI completion interrupt */ -- 2.1.4
[PATCH v3 15/15] staging: ccree: fix ifnullfree.cocci warnings
From: kbuild test robot <l...@intel.com> drivers/staging/ccree/ssi_hash.c:317:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:320:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:323:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:373:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:375:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:377:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:379:3-8: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:381:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. drivers/staging/ccree/ssi_hash.c:383:2-7: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. NULL check before some freeing functions is not needed. Based on checkpatch warning "kfree(NULL) is safe this check is probably not required" and kfreeaddr.cocci by Julia Lawall. Generated by: scripts/coccinelle/free/ifnullfree.cocci Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Signed-off-by: Fengguang Wu <fengguang...@intel.com> --- drivers/staging/ccree/ssi_hash.c | 27 +-- 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/drivers/staging/ccree/ssi_hash.c b/drivers/staging/ccree/ssi_hash.c index ab191de..8ff5d4e 100644 --- a/drivers/staging/ccree/ssi_hash.c +++ b/drivers/staging/ccree/ssi_hash.c @@ -314,14 +314,11 @@ static int ssi_hash_map_request(struct device *dev, state->digest_buff_dma_addr = 0; } fail3: - if (state->opad_digest_buff != NULL) - kfree(state->opad_digest_buff); + kfree(state->opad_digest_buff); fail2: - if (state->digest_bytes_len != NULL) - kfree(state->digest_bytes_len); + kfree(state->digest_bytes_len); fail1: - if (state->digest_buff != NULL) - kfree(state->digest_buff); +kfree(state->digest_buff); fail_digest_result_buff: if (state->digest_result_buff != NULL) { kfree(state->digest_result_buff); @@ -370,18 +367,12 @@ static void ssi_hash_unmap_request(struct device *dev, state->opad_digest_dma_addr = 0; } - if (state->opad_digest_buff != NULL) - kfree(state->opad_digest_buff); - if (state->digest_bytes_len != NULL) - kfree(state->digest_bytes_len); - if (state->digest_buff != NULL) - kfree(state->digest_buff); - if (state->digest_result_buff != NULL) - kfree(state->digest_result_buff); - if (state->buff1 != NULL) - kfree(state->buff1); - if (state->buff0 != NULL) - kfree(state->buff0); + kfree(state->opad_digest_buff); + kfree(state->digest_bytes_len); + kfree(state->digest_buff); + kfree(state->digest_result_buff); + kfree(state->buff1); + kfree(state->buff0); } static void ssi_hash_unmap_result(struct device *dev, -- 2.1.4
[PATCH v3 11/15] staging: ccree: fix platform_no_drv_owner.cocci warnings
From: kbuild test robot <l...@intel.com> drivers/staging/ccree/ssi_driver.c:484:6-11: No need to set .owner here. The core will do it. Remove .owner field if calls are used which set it automatically Generated by: scripts/coccinelle/api/platform_no_drv_owner.cocci Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Signed-off-by: Fengguang Wu <fengguang...@intel.com> --- drivers/staging/ccree/ssi_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 03a044a..bc19adc 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -539,7 +539,6 @@ MODULE_DEVICE_TABLE(of, arm_cc7x_dev_of_match); static struct platform_driver cc7x_driver = { .driver = { .name = "cc7xree", - .owner = THIS_MODULE, #ifdef CONFIG_OF .of_match_table = arm_cc7x_dev_of_match, #endif -- 2.1.4
[PATCH v3 09/15] MAINTAINERS: add Gilad BY as ccree maintainer
I work for Arm on maintaining the TrustZone CryptoCell driver. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 676c139..f21caa1 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -3066,6 +3066,13 @@ F: drivers/net/ieee802154/cc2520.c F: include/linux/spi/cc2520.h F: Documentation/devicetree/bindings/net/ieee802154/cc2520.txt +CCREE ARM TRUSTZONE CRYPTOCELL 700 REE DRIVER +M: Gilad Ben-Yossef <gi...@benyossef.com> +L: linux-cry...@vger.kernel.org +S: Supported +F: drivers/staging/ccree/ +W: https://developer.arm.com/products/system-ip/trustzone-cryptocell/cryptocell-700-family + CEC DRIVER M: Hans Verkuil <hans.verk...@cisco.com> L: linux-me...@vger.kernel.org -- 2.1.4
[PATCH v3 14/15] staging: ccree: fix ifnullfree.cocci warnings
From: kbuild test robot <l...@intel.com> drivers/staging/ccree/ssi_buffer_mgr.c:530:3-19: WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values. NULL check before some freeing functions is not needed. Based on checkpatch warning "kfree(NULL) is safe this check is probably not required" and kfreeaddr.cocci by Julia Lawall. Generated by: scripts/coccinelle/free/ifnullfree.cocci Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> Signed-off-by: Fengguang Wu <fengguang...@intel.com> --- drivers/staging/ccree/ssi_buffer_mgr.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index af50904..038e2ff 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -1863,8 +1863,7 @@ int ssi_buffer_mgr_fini(struct ssi_drvdata *drvdata) struct buff_mgr_handle *buff_mgr_handle = drvdata->buff_mgr_handle; if (buff_mgr_handle != NULL) { - if (buff_mgr_handle->mlli_buffs_pool != NULL) - dma_pool_destroy(buff_mgr_handle->mlli_buffs_pool); + dma_pool_destroy(buff_mgr_handle->mlli_buffs_pool); kfree(drvdata->buff_mgr_handle); drvdata->buff_mgr_handle = NULL; -- 2.1.4
Re: [PATCH v2 6/9] staging: ccree: add FIPS support
Hi, Thank you for the review. On Thu, Apr 20, 2017 at 4:39 PM, Stephan Müller <smuel...@chronox.de> wrote: >> +/* The function verifies that tdes keys are not weak.*/ >> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen) >> +{ >> +#ifdef CCREE_FIPS_SUPPORT >> +tdes_keys_t *tdes_key = (tdes_keys_t*)key; >> + >> + /* verify key1 != key2 and key3 != key2*/ > > I do not think that this check is necessary. There is no FIPS requirement or > IG that mandates this (unlike the XTS key check). > > If there would be such requirement, we would need a common service function > for all TDES implementations I am not sure. I have forwarded a question internally and based on the answer will either drop this or add a common function and post a patch add the check to all 3DES implementation. This has been added to the staging TODO list for the driver. > >> +if (unlikely( (memcmp((u8*)tdes_key->key1, (u8*)tdes_key->key2, >> sizeof(tdes_key->key1)) == 0) || + >> (memcmp((u8*)tdes_key->key3, >> (u8*)tdes_key->key2, sizeof(tdes_key->key3)) == 0) )) { + >> return -ENOEXEC; >> +} >> +#endif /* CCREE_FIPS_SUPPORT */ >> + >> +return 0; >> +} >> + >> +/* The function verifies that xts keys are not weak.*/ >> +static int ssi_fips_verify_xts_keys(const u8 *key, unsigned int keylen) >> +{ >> +#ifdef CCREE_FIPS_SUPPORT >> +/* Weak key is define as key that its first half (128/256 lsb) >> equals its second half (128/256 msb) */ +int singleKeySize = keylen >> >> 1; >> + >> + if (unlikely(memcmp(key, [singleKeySize], singleKeySize) == 0)) { >> + return -ENOEXEC; > > Use xts_check_key. Will fix. Added to TODO staging list for the driver. > >> +The test vectors were taken from: >> + >> +* AES >> +NIST Special Publication 800-38A 2001 Edition >> +Recommendation for Block Cipher Modes of Operation >> +http://csrc.nist.gov/publications/nistpubs/800-38a/sp800-38a.pdf >> +Appendix F: Example Vectors for Modes of Operation of the AES >> + >> +* AES CTS >> +Advanced Encryption Standard (AES) Encryption for Kerberos 5 >> +February 2005 >> +https://tools.ietf.org/html/rfc3962#appendix-B >> +B. Sample Test Vectors >> + >> +* AES XTS >> +http://csrc.nist.gov/groups/STM/cavp/#08 >> +http://csrc.nist.gov/groups/STM/cavp/documents/aes/XTSTestVectors.zip >> + >> +* AES CMAC >> +http://csrc.nist.gov/groups/STM/cavp/index.html#07 >> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/cmactestvectors.zip >> + >> +* AES-CCM >> +http://csrc.nist.gov/groups/STM/cavp/#07 >> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/ccmtestvectors.zip >> + >> +* AES-GCM >> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/gcmtestvectors.zip >> + >> +* Triple-DES >> +NIST Special Publication 800-67 January 2012 >> +Recommendation for the Triple Data Encryption Algorithm (TDEA) Block Cipher >> +http://csrc.nist.gov/publications/nistpubs/800-67-Rev1/SP-800-67-Rev1.pdf >> +APPENDIX B: EXAMPLE OF TDEA FORWARD AND INVERSE CIPHER OPERATIONS +and >> +http://csrc.nist.gov/groups/STM/cavp/#01 >> +http://csrc.nist.gov/groups/STM/cavp/documents/des/tdesmct_intermediate.zip >> + >> +* HASH >> +http://csrc.nist.gov/groups/STM/cavp/#03 >> +http://csrc.nist.gov/groups/STM/cavp/documents/shs/shabytetestvectors.zip >> + >> +* HMAC >> +http://csrc.nist.gov/groups/STM/cavp/#07 >> +http://csrc.nist.gov/groups/STM/cavp/documents/mac/hmactestvectors.zip >> + >> +*/ > > Is this test vector business really needed? Why do you think that testmgr.c is > not sufficient? Other successful FIPS validations of the kernel crypto API > managed without such special code. That is a very good question. I am guessing this has something to do to with this driver spending its life out of tree and being maintained against old kernel versions that may have had some gaps in FIPS testing and since fixed. I will review what, if at all, is missing from testmgr and fold those missing parts (if found) there and drop this from the driver. > > Also, your entire API seems to implement the approach that if there is a self > test error, you disable the cipher functions, but leave the rest in-tact. The > standard kernel crypto API handling logic is to simply panic the kernel. Is it > really necessary to implement a special case for your driver? > > No it isn't. What ever the behavior we need it should be added, pending review of course, to the generic FIPS logic handling. I do wonder if there is value in alternate behavior of stopping crypto API on FIPS error rather than a panic though. I will try to get an explanation why we do it this way. Handling all these has been added to the driver staging TODO list and will be handled before it matures into drivers/crypto/ Many thanks for the review! Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH v3 04/15] staging: ccree: add IV generation support
Add CryptoCell IV hardware generation support. This patch adds the needed support to drive the HW but does not expose the ability via the kernel crypto API yet. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Makefile | 2 +- drivers/staging/ccree/ssi_buffer_mgr.c | 2 + drivers/staging/ccree/ssi_cipher.c | 11 ++ drivers/staging/ccree/ssi_cipher.h | 1 + drivers/staging/ccree/ssi_driver.c | 9 + drivers/staging/ccree/ssi_driver.h | 7 + drivers/staging/ccree/ssi_ivgen.c | 301 drivers/staging/ccree/ssi_ivgen.h | 72 drivers/staging/ccree/ssi_pm.c | 2 + drivers/staging/ccree/ssi_request_mgr.c | 33 +++- 10 files changed, 438 insertions(+), 2 deletions(-) create mode 100644 drivers/staging/ccree/ssi_ivgen.c create mode 100644 drivers/staging/ccree/ssi_ivgen.h diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index 21a80d5..89afe9a 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index d0d5352..6ff5d6b 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -534,6 +534,7 @@ void ssi_buffer_mgr_unmap_blkcipher_request( SSI_RESTORE_DMA_ADDR_TO_48BIT(req_ctx->gen_ctx.iv_dma_addr); dma_unmap_single(dev, req_ctx->gen_ctx.iv_dma_addr, ivsize, +req_ctx->is_giv ? DMA_BIDIRECTIONAL : DMA_TO_DEVICE); } /* Release pool */ @@ -587,6 +588,7 @@ int ssi_buffer_mgr_map_blkcipher_request( req_ctx->gen_ctx.iv_dma_addr = dma_map_single(dev, (void *)info, ivsize, + req_ctx->is_giv ? DMA_BIDIRECTIONAL: DMA_TO_DEVICE); if (unlikely(dma_mapping_error(dev, req_ctx->gen_ctx.iv_dma_addr))) { diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index d22a1b3..4a95f13 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -819,6 +819,13 @@ static int ssi_blkcipher_process( areq, desc, _len); + /* do we need to generate IV? */ + if (req_ctx->is_giv == true) { + ssi_req.ivgen_dma_addr[0] = req_ctx->gen_ctx.iv_dma_addr; + ssi_req.ivgen_dma_addr_len = 1; + /* set the IV size (8/16 B long)*/ + ssi_req.ivgen_size = ivsize; + } END_CYCLE_COUNT(ssi_req.op_type, STAT_PHASE_2); /* STAT_PHASE_3: Lock HW and push sequence */ @@ -901,6 +908,7 @@ static int ssi_sblkcipher_encrypt(struct blkcipher_desc *desc, unsigned int ivsize = crypto_blkcipher_ivsize(blk_tfm); req_ctx->backup_info = desc->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, dst, src, nbytes, desc->info, ivsize, NULL, DRV_CRYPTO_DIRECTION_ENCRYPT); } @@ -916,6 +924,7 @@ static int ssi_sblkcipher_decrypt(struct blkcipher_desc *desc, unsigned int ivsize = crypto_blkcipher_ivsize(blk_tfm); req_ctx->backup_info = desc->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, dst, src, nbytes, desc->info, ivsize, NULL, DRV_CRYPTO_DIRECTION_DECRYPT); } @@ -948,6 +957,7 @@ static int ssi_ablkcipher_encrypt(struct ablkcipher_request *req) unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); req_ctx->backup_info = req->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_ENCRYPT); } @@ -960,6 +970,7 @@ static int ssi_ablkcipher_decrypt(struct ablkcipher_request *req) unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); req_ctx->backup_info = req->info; + req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_DECRYPT); } diff --git a/drivers/staging/ccree/ssi_cipher.h b/drivers/staging/ccree/ssi_cipher.h index 9ceb0b6..ba4eb7c 100644 --- a/driv
[PATCH v3 07/15] staging: ccree: add TODO list
Add TODO list for moving out of staging tree for ccree crypto driver Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/TODO | 30 ++ 1 file changed, 30 insertions(+) create mode 100644 drivers/staging/ccree/TODO diff --git a/drivers/staging/ccree/TODO b/drivers/staging/ccree/TODO new file mode 100644 index 000..c9f5754 --- /dev/null +++ b/drivers/staging/ccree/TODO @@ -0,0 +1,30 @@ + + +* +* * +* Arm Trust Zone CryptoCell REE Linux driver upstreaming TODO items* +* * +* + +ccree specific items +a.k.a stuff fixing for this driver to move out of staging +~ + +1. Move to using Crypto Engine to handle backlog queueing. +2. Remove synchronous algorithm support leftovers. +3. Separate platform specific code for FIPS and power management into separate platform modules. +4. Drop legacy kernel support code. +5. Move most (all?) #ifdef CONFIG into inline functions. +6. Remove all unused definitions. +7. Re-factor to accomediate newer/older HW revisions besides the 712. +8. Handle the many checkpatch errors. +9. Implement ahash import/export correctly. +10. Go through a proper review of DT bindings and sysfs ABI +11. Sort out FIPS mode: bake tests into testmgr, sort out behaviour on error, +figure if 3DES weak key check is needed + +Kernel infrastructure items +a.k.a stuff we either neither need to fix in the kernel or understand what we're doing wrong + +1. ahash import/export context has a PAGE_SIZE/8 size limit. We need more. +2. Crypto Engine seems to be built for HW with hardware queue depth of 1, we have 600++. -- 2.1.4
[PATCH v3 00/15] staging: ccree: add Arm TrustZone CryptoCell REE driver
Arm TrustZone CryptoCell 700 is a family of cryptographic hardware accelerators. It is supported by a long lived series of out of tree drivers, which I am now in the process of unifying and upstreaming. This is the first drop, supporting the new CryptoCell 712 REE. The code still needs some cleanup before maturing to a proper upstream driver, which I am in the process of doing. However, as discussion of some of the capabilities of the hardware and its application to some dm-crypt and dm-verity features recently took place I though it is better to do this in the open via the staging tree. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> CC: Binoy Jayan <binoy.ja...@linaro.org> CC: Ofir Drang <ofir.dr...@arm.com> CC: Stuart Yoder <stuart.yo...@arm.com> CC: Stephan Muller <smuel...@chronox.de> Changes from v2: - Fix stupid build error on i386 due to left over Arm specific code. - Fix copyright header to match GPLv2 license, as pointed out by Greg KH. - Add proper handling of FIPS mode to TODO list, as pointed by Stephan Müller. - Remove uneeded empty file bsp.h - Fold in a bunch of fixes from kbuild robot. Changes from v1: - Broke up patch set into smaller units for mailing list review as per Greg KH's indication. - Changed DT binding compatible tag as per Mark Rutland suggestion. - Moved DT binding document inside the staging directory and added DT binding review to TODO list as per Mark Rutland's request. Gilad Ben-Yossef (10): staging: ccree: introduce CryptoCell HW driver staging: ccree: add ahash support staging: ccree: add skcipher support staging: ccree: add IV generation support staging: ccree: add AEAD support staging: ccree: add FIPS support staging: ccree: add TODO list staging: ccree: add DT bindings for Arm CryptoCell MAINTAINERS: add Gilad BY as ccree maintainer staging: ccree: remove useless NULL test of field kbuild test robot (5): staging: ccree: fix platform_no_drv_owner.cocci warnings staging: ccree: fix semicolon.cocci warnings staging: ccree: fix array_size.cocci warnings staging: ccree: fix ifnullfree.cocci warnings staging: ccree: fix ifnullfree.cocci warnings MAINTAINERS|7 + drivers/staging/Kconfig|2 + drivers/staging/Makefile |2 +- .../devicetree/bindings/crypto/arm-cryptocell.txt | 27 + drivers/staging/ccree/Kconfig | 43 + drivers/staging/ccree/Makefile |3 + drivers/staging/ccree/TODO | 30 + drivers/staging/ccree/cc_bitops.h | 62 + drivers/staging/ccree/cc_crypto_ctx.h | 299 +++ drivers/staging/ccree/cc_hal.h | 30 + drivers/staging/ccree/cc_hw_queue_defs.h | 603 + drivers/staging/ccree/cc_lli_defs.h| 57 + drivers/staging/ccree/cc_pal_log.h | 188 ++ drivers/staging/ccree/cc_pal_log_plat.h| 33 + drivers/staging/ccree/cc_pal_types.h | 97 + drivers/staging/ccree/cc_pal_types_plat.h | 29 + drivers/staging/ccree/cc_regs.h| 106 + drivers/staging/ccree/dx_crys_kernel.h | 180 ++ drivers/staging/ccree/dx_env.h | 224 ++ drivers/staging/ccree/dx_host.h| 155 ++ drivers/staging/ccree/dx_reg_base_host.h | 34 + drivers/staging/ccree/dx_reg_common.h | 26 + drivers/staging/ccree/hash_defs.h | 78 + drivers/staging/ccree/hw_queue_defs_plat.h | 43 + drivers/staging/ccree/ssi_aead.c | 2832 drivers/staging/ccree/ssi_aead.h | 120 + drivers/staging/ccree/ssi_buffer_mgr.c | 1873 + drivers/staging/ccree/ssi_buffer_mgr.h | 105 + drivers/staging/ccree/ssi_cipher.c | 1503 +++ drivers/staging/ccree/ssi_cipher.h | 89 + drivers/staging/ccree/ssi_config.h | 61 + drivers/staging/ccree/ssi_driver.c | 556 drivers/staging/ccree/ssi_driver.h | 228 ++ drivers/staging/ccree/ssi_fips.c | 65 + drivers/staging/ccree/ssi_fips.h | 70 + drivers/staging/ccree/ssi_fips_data.h | 315 +++ drivers/staging/ccree/ssi_fips_ext.c | 96 + drivers/staging/ccree/ssi_fips_ll.c| 1681 drivers/staging/ccree/ssi_fips_local.c | 369 +++ drivers/staging/ccree/ssi_fips_local.h | 77 + drivers/staging/ccree/ssi_hash.c | 2742 +++ drivers/staging/ccree/ssi_hash.h | 101 + drivers/staging/ccree/ssi_ivgen.c | 301 +++ drivers/staging/ccree/ssi_ivgen.h | 72 + drivers/staging/ccree/ssi_p
[PATCH v3 08/15] staging: ccree: add DT bindings for Arm CryptoCell
This adds DT bindings for the Arm TrustZone CryptoCell cryptographic accelerator IP. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- .../devicetree/bindings/crypto/arm-cryptocell.txt | 27 ++ 1 file changed, 27 insertions(+) create mode 100644 drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt diff --git a/drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt b/drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt new file mode 100644 index 000..2ea6517 --- /dev/null +++ b/drivers/staging/ccree/Documentation/devicetree/bindings/crypto/arm-cryptocell.txt @@ -0,0 +1,27 @@ +Arm TrustZone CryptoCell cryptographic accelerators + +Required properties: +- compatible: must be "arm,cryptocell-712-ree". +- reg: shall contain base register location and length. + Typically length is 0x1. +- interrupts: shall contain the interrupt for the device. + +Optional properties: +- interrupt-parent: can designate the interrupt controller the + device interrupt is connected to, if needed. +- clocks: may contain the clock handling the device, if needed. +- power-domains: may contain a reference to the PM domain, if applicable. + + +Examples: + +Zynq FPGA device + + + arm_cc7x: arm_cc7x@8000 { + compatible = "arm,cryptocell-712-ree"; + interrupt-parent = <>; + interrupts = < 0 30 4 >; + reg = < 0x8000 0x1 >; + }; + -- 2.1.4
[PATCH v3 06/15] staging: ccree: add FIPS support
Add FIPS mode support to CryptoCell driver Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |9 + drivers/staging/ccree/Makefile |1 + drivers/staging/ccree/ssi_aead.c|6 + drivers/staging/ccree/ssi_cipher.c | 52 + drivers/staging/ccree/ssi_driver.c | 19 +- drivers/staging/ccree/ssi_driver.h |2 + drivers/staging/ccree/ssi_fips.c| 65 ++ drivers/staging/ccree/ssi_fips.h| 70 ++ drivers/staging/ccree/ssi_fips_data.h | 315 ++ drivers/staging/ccree/ssi_fips_ext.c| 96 ++ drivers/staging/ccree/ssi_fips_ll.c | 1681 +++ drivers/staging/ccree/ssi_fips_local.c | 369 +++ drivers/staging/ccree/ssi_fips_local.h | 77 ++ drivers/staging/ccree/ssi_hash.c| 21 +- drivers/staging/ccree/ssi_request_mgr.c |2 + 15 files changed, 2783 insertions(+), 2 deletions(-) create mode 100644 drivers/staging/ccree/ssi_fips.c create mode 100644 drivers/staging/ccree/ssi_fips.h create mode 100644 drivers/staging/ccree/ssi_fips_data.h create mode 100644 drivers/staging/ccree/ssi_fips_ext.c create mode 100644 drivers/staging/ccree/ssi_fips_ll.c create mode 100644 drivers/staging/ccree/ssi_fips_local.c create mode 100644 drivers/staging/ccree/ssi_fips_local.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 2d11223..ae62704 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -24,6 +24,15 @@ config CRYPTO_DEV_CCREE cryptographic operations on the system REE. If unsure say Y. +config CCREE_FIPS_SUPPORT + bool "Turn on CryptoCell 7XX REE FIPS mode support" + depends on CRYPTO_DEV_CCREE + default n + help + Say 'Y' to enable support for FIPS compliant mode by the + CCREE driver. + If unsure say N. + config CCREE_DISABLE_COHERENT_DMA_OPS bool "Disable Coherent DMA operations for the CCREE driver" depends on CRYPTO_DEV_CCREE diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index b9285c0..44f3e3e 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-$(CCREE_FIPS_SUPPORT) += ssi_fips.o ssi_fips_ll.o ssi_fips_ext.o ssi_fips_local.o diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index 33d72d2..0382917 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -36,6 +36,7 @@ #include "ssi_hash.h" #include "ssi_sysfs.h" #include "ssi_sram_mgr.h" +#include "ssi_fips_local.h" #define template_aead template_u.aead @@ -153,6 +154,8 @@ static int ssi_aead_init(struct crypto_aead *tfm) container_of(alg, struct ssi_crypto_alg, aead_alg); SSI_LOG_DEBUG("Initializing context @%p for %s\n", ctx, crypto_tfm_alg_name(&(tfm->base))); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); + /* Initialize modes in instance */ ctx->cipher_mode = ssi_alg->cipher_mode; ctx->flow_mode = ssi_alg->flow_mode; @@ -572,6 +575,7 @@ ssi_aead_setkey(struct crypto_aead *tfm, const u8 *key, unsigned int keylen) SSI_LOG_DEBUG("Setting key in context @%p for %s. key=%p keylen=%u\n", ctx, crypto_tfm_alg_name(crypto_aead_tfm(tfm)), key, keylen); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); /* STAT_PHASE_0: Init and sanity checks */ START_CYCLE_COUNT(); @@ -699,6 +703,7 @@ static int ssi_aead_setauthsize( { struct ssi_aead_ctx *ctx = crypto_aead_ctx(authenc); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); /* Unsupported auth. sizes */ if ((authsize == 0) || (authsize >crypto_aead_maxauthsize(authenc))) { @@ -2006,6 +2011,7 @@ static int ssi_aead_process(struct aead_request *req, enum drv_crypto_direction SSI_LOG_DEBUG("%s context=%p req=%p iv=%p src=%p src_ofs=%d dst=%p dst_ofs=%d cryptolen=%d\n", ((direct==DRV_CRYPTO_DIRECTION_ENCRYPT)?"Encrypt":"Decrypt"), ctx, req, req->iv, sg_virt(req->src), req->src->offset, sg_virt(req->dst), req->dst->offset, req->cryptlen); + CHECK_AND_RETURN_UPON_FIPS_ERROR(); /* STAT_PHASE_0: Init and sanity checks */ START_CYCLE_COUNT(); diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 4a95f13..664ed7e 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -31,6 +31,7 @@ #include "ssi_cipher.h" #include "ssi_r
[PATCH v3 05/15] staging: ccree: add AEAD support
Add CryptoCell AEAD support Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |1 + drivers/staging/ccree/Makefile |2 +- drivers/staging/ccree/cc_crypto_ctx.h | 21 + drivers/staging/ccree/ssi_aead.c | 2826 drivers/staging/ccree/ssi_aead.h | 120 ++ drivers/staging/ccree/ssi_buffer_mgr.c | 899 ++ drivers/staging/ccree/ssi_buffer_mgr.h |4 + drivers/staging/ccree/ssi_driver.c | 11 + drivers/staging/ccree/ssi_driver.h |4 + 9 files changed, 3887 insertions(+), 1 deletion(-) create mode 100644 drivers/staging/ccree/ssi_aead.c create mode 100644 drivers/staging/ccree/ssi_aead.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 3fff040..2d11223 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -5,6 +5,7 @@ config CRYPTO_DEV_CCREE select CRYPTO_HASH select CRYPTO_BLKCIPHER select CRYPTO_DES + select CRYPTO_AEAD select CRYPTO_AUTHENC select CRYPTO_SHA1 select CRYPTO_MD5 diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index 89afe9a..b9285c0 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_aead.o ssi_ivgen.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/cc_crypto_ctx.h b/drivers/staging/ccree/cc_crypto_ctx.h index a7f7d95..9e10b26 100644 --- a/drivers/staging/ccree/cc_crypto_ctx.h +++ b/drivers/staging/ccree/cc_crypto_ctx.h @@ -263,6 +263,27 @@ struct drv_ctx_cipher { (CC_AES_KEY_SIZE_MAX/sizeof(uint32_t))]; }; +/* authentication and encryption with associated data class */ +struct drv_ctx_aead { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_AES */ + enum drv_cipher_mode mode; + enum drv_crypto_direction direction; + uint32_t key_size; /* numeric value in bytes */ + uint32_t nonce_size; /* nonce size (octets) */ + uint32_t header_size; /* finit additional data size (octets) */ + uint32_t text_size; /* finit text data size (octets) */ + uint32_t tag_size; /* mac size, element of {4, 6, 8, 10, 12, 14, 16} */ + /* block_state1/2 is the AES engine block state */ + uint8_t block_state[CC_AES_BLOCK_SIZE]; + uint8_t mac_state[CC_AES_BLOCK_SIZE]; /* MAC result */ + uint8_t nonce[CC_AES_BLOCK_SIZE]; /* nonce buffer */ + uint8_t key[CC_AES_KEY_SIZE_MAX]; + /* reserve to end of allocated context size */ + uint32_t reserved[CC_DRV_CTX_SIZE_WORDS - 8 - + 3 * (CC_AES_BLOCK_SIZE/sizeof(uint32_t)) - + CC_AES_KEY_SIZE_MAX/sizeof(uint32_t)]; +}; + /***/ /* MESSAGE BASED CONTEXTS **/ /***/ diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c new file mode 100644 index 000..33d72d2 --- /dev/null +++ b/drivers/staging/ccree/ssi_aead.c @@ -0,0 +1,2826 @@ +/* + * Copyright (C) 2012-2017 ARM Limited or its affiliates. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "ssi_config.h" +#include "ssi_driver.h" +#include "ssi_buffer_mgr.h" +#include "ssi_aead.h" +#include "ssi_request_mgr.h" +#include "ssi_hash.h" +#include "ssi_sysfs.h" +#include "ssi_sram_mgr.h" + +#define template_aead template_u.aead + +#define MAX_AEAD_SETKEY_SEQ 12 +#define MAX_AEAD_PROCESS_SEQ 23 + +#define MAX_HMAC_DIGEST_SIZE (SHA256_DIGEST_SIZE) +#define MAX_HMAC_BLOCK_SIZE (SHA256_BLOCK_SIZE) + +#define AES_CCM_RFC4309_NONCE_SIZE 3 +#define MAX_NONCE_SIZE CTR_RFC3686_NONCE_SIZE + + +/* Value of each ICV_CMP byte (of 8) in case of success */ +#define ICV_VERIF_OK 0x01 + +st
Re: [PATCH v2 6/9] staging: ccree: add FIPS support
On Sun, Apr 23, 2017 at 12:48 PM, Gilad Ben-Yossef <gi...@benyossef.com> wrote: > Hi, > > Thank you for the review. > > On Thu, Apr 20, 2017 at 4:39 PM, Stephan Müller <smuel...@chronox.de> wrote: > >>> +/* The function verifies that tdes keys are not weak.*/ >>> +static int ssi_fips_verify_3des_keys(const u8 *key, unsigned int keylen) >>> +{ >>> +#ifdef CCREE_FIPS_SUPPORT >>> +tdes_keys_t *tdes_key = (tdes_keys_t*)key; >>> + >>> + /* verify key1 != key2 and key3 != key2*/ >> >> I do not think that this check is necessary. There is no FIPS requirement or >> IG that mandates this (unlike the XTS key check). >> >> If there would be such requirement, we would need a common service function >> for all TDES implementations Well, it turns out there is and we do :-) This is from crypto/des_generic.c: /* * RFC2451: * * For DES-EDE3, there is no known need to reject weak or * complementation keys. Any weakness is obviated by the use of * multiple keys. * * However, if the first two or last two independent 64-bit keys are * equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the * same as DES. Implementers MUST reject keys that exhibit this * property. * */ int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, unsigned int keylen) However, this does not check that k1 == k3. In this case DES3 becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 by NIST Special Publication 800-131A*. Would it be acceptable if I offer a patch adding this check to __des3_ede_setkey() and use that in the ccree driver? * http://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-131Ar1.pdf Many thanks, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [PATCH v2 6/9] staging: ccree: add FIPS support
On Mon, Apr 24, 2017 at 9:16 AM, Stephan Müller <smuel...@chronox.de> wrote: > Am Montag, 24. April 2017, 08:06:09 CEST schrieb Gilad Ben-Yossef: > > Hi Gilad, >> >> Well, it turns out there is and we do :-) >> >> This is from crypto/des_generic.c: >> >> /* >> * RFC2451: >> * >> * For DES-EDE3, there is no known need to reject weak or >> * complementation keys. Any weakness is obviated by the use of >> * multiple keys. >> * >> * However, if the first two or last two independent 64-bit keys are >> * equal (k1 == k2 or k2 == k3), then the DES3 operation is simply the >> * same as DES. Implementers MUST reject keys that exhibit this >> * property. >> * >> */ >> int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, >> unsigned int keylen) >> >> However, this does not check that k1 == k3. In this case DES3 >> becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 >> by NIST Special Publication 800-131A*. > > It is correct that the RFC wants at least a 2key 3DES. And it is correct that > SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS 140-2 > does *not* require a technical verification of the 3 keys being not identical. > > Note, formally, FIPS 140-2 requires that the 3 keys (i.e. all 192 bits) must > be obtained from *one* call to a DRBG or KDF (separate independent calls to, > say, obtain one key at a time is *not* permitted). Of course, fixing the > parity bits is allowed after obtaining the random number. >> >> Would it be acceptable if I offer a patch adding this check to >> __des3_ede_setkey() >> and use that in the ccree driver? > > I am not sure it makes sense as the core requirement is the *one* invocation > of the DRBG. Thanks you for the clarification. As I think is obvious by now I am not a FIPS expert by any stretch. Isn't the requirements on DRBG or KDF invocations pertain to key generation only? What happens if you don't derive the keys on the system, but wish to use keys derived elsewhere? I assumed the limitations on weak keys etc. were meant to protect against that scenario and are therefore independent from key generation requirements, but I may have misunderstood that. Anyway, if I understand you correctly, what you are saying is that these checks might make an implementation RFC 2451 and SP800-131A compliant respectively but they are not required for FIPS 140-2 compliance? did I understand that correctly? If so, since two 3DES implementation in the kernel already do the RFC 2451 compliant check I assume you would not object to the ccree driver using the same function, disconnect from FIPS being set or not, just as is being done with the other 3DES implementation. In addition, would it be OK if we did an extra check in the ccree driver for SP800-131A key compliance and disable encryption (but allow decryption) if the key fails the check? again, this would be independent from FIPS mode? Thanks again, Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
Re: [PATCH v2 6/9] staging: ccree: add FIPS support
On Mon, Apr 24, 2017 at 9:21 AM, Stephan Müller <smuel...@chronox.de> wrote: > Am Montag, 24. April 2017, 08:16:50 CEST schrieb Stephan Müller: > > Hi Gilad, > >> > >> > int __des3_ede_setkey(u32 *expkey, u32 *flags, const u8 *key, >> > >> > unsigned int keylen) >> > >> > However, this does not check that k1 == k3. In this case DES3 >> > becomes 2DES (2-keys TDEA), the use of which was dropped post 2015 >> > by NIST Special Publication 800-131A*. >> >> It is correct that the RFC wants at least a 2key 3DES. And it is correct >> that SP800-131A mandates 3key 3DES post 2015. All I am saying is that FIPS >> 140-2 does *not* require a technical verification of the 3 keys being not >> identical. > > One side note: we had discussed a patch to this function in the past, see [1]. > > [1] https://patchwork.kernel.org/patch/8293441/ > Thanks, I was not aware of that. I guess we could change the function to indicate that a key is valid for decryption but not encryption and have the implementation limiting based on that if there is an interest in SP800-131A compliance. Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH v3 03/15] staging: ccree: add skcipher support
Add CryptoCell skcipher support Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |8 + drivers/staging/ccree/Makefile |2 +- drivers/staging/ccree/cc_crypto_ctx.h | 21 + drivers/staging/ccree/ssi_buffer_mgr.c | 147 drivers/staging/ccree/ssi_buffer_mgr.h | 16 + drivers/staging/ccree/ssi_cipher.c | 1440 drivers/staging/ccree/ssi_cipher.h | 88 ++ drivers/staging/ccree/ssi_driver.c | 14 + drivers/staging/ccree/ssi_driver.h | 30 + 9 files changed, 1765 insertions(+), 1 deletion(-) create mode 100644 drivers/staging/ccree/ssi_cipher.c create mode 100644 drivers/staging/ccree/ssi_cipher.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index a528a99..3fff040 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -3,11 +3,19 @@ config CRYPTO_DEV_CCREE depends on CRYPTO_HW && OF && HAS_DMA default n select CRYPTO_HASH + select CRYPTO_BLKCIPHER + select CRYPTO_DES + select CRYPTO_AUTHENC select CRYPTO_SHA1 select CRYPTO_MD5 select CRYPTO_SHA256 select CRYPTO_SHA512 select CRYPTO_HMAC + select CRYPTO_AES + select CRYPTO_CBC + select CRYPTO_ECB + select CRYPTO_CTR + select CRYPTO_XTS help Say 'Y' to enable a driver for the Arm TrustZone CryptoCell C7xx. Currently only the CryptoCell 712 REE is supported. diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index f94e225..21a80d5 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_cipher.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/cc_crypto_ctx.h b/drivers/staging/ccree/cc_crypto_ctx.h index a4aa066..a7f7d95 100644 --- a/drivers/staging/ccree/cc_crypto_ctx.h +++ b/drivers/staging/ccree/cc_crypto_ctx.h @@ -242,6 +242,27 @@ struct drv_ctx_hmac { CC_DIGEST_SIZE_MAX - CC_HMAC_BLOCK_SIZE_MAX]; }; +struct drv_ctx_cipher { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_AES */ + enum drv_cipher_mode mode; + enum drv_crypto_direction direction; + enum drv_crypto_key_type crypto_key_type; + enum drv_crypto_padding_type padding_type; + uint32_t key_size; /* numeric value in bytes */ + uint32_t data_unit_size; /* required for XTS */ + /* block_state is the AES engine block state. + * It is used by the host to pass IV or counter at initialization. + * It is used by SeP for intermediate block chaining state and for + * returning MAC algorithms results. */ + uint8_t block_state[CC_AES_BLOCK_SIZE]; + uint8_t key[CC_AES_KEY_SIZE_MAX]; + uint8_t xex_key[CC_AES_KEY_SIZE_MAX]; + /* reserve to end of allocated context size */ + uint32_t reserved[CC_DRV_CTX_SIZE_WORDS - 7 - + CC_AES_BLOCK_SIZE/sizeof(uint32_t) - 2 * + (CC_AES_KEY_SIZE_MAX/sizeof(uint32_t))]; +}; + /***/ /* MESSAGE BASED CONTEXTS **/ /***/ diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index aceb01c..d0d5352 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -28,6 +28,7 @@ #include "ssi_buffer_mgr.h" #include "cc_lli_defs.h" +#include "ssi_cipher.h" #include "ssi_hash.h" #define LLI_MAX_NUM_OF_DATA_ENTRIES 128 @@ -517,6 +518,152 @@ static inline int ssi_ahash_handle_curr_buf(struct device *dev, return 0; } +void ssi_buffer_mgr_unmap_blkcipher_request( + struct device *dev, + void *ctx, + unsigned int ivsize, + struct scatterlist *src, + struct scatterlist *dst) +{ + struct blkcipher_req_ctx *req_ctx = (struct blkcipher_req_ctx *)ctx; + + if (likely(req_ctx->gen_ctx.iv_dma_addr != 0)) { + SSI_LOG_DEBUG("Unmapped iv: iv_dma_addr=0x%llX iv_size=%u\n", + (unsigned long long)req_ctx->gen_ctx.iv_dma_addr, + ivsize); + SSI_RESTORE_DMA_ADDR_TO_48BIT(req_ctx->gen_ctx.iv_dma_addr); + dma_unmap_single(dev, req_ctx->gen_ctx.iv_dma_addr, +ivsize, +DMA_TO_DEVICE); + } + /* Release pool */ + if (req_ctx->dma
[PATCH v3 02/15] staging: ccree: add ahash support
Add CryptoCell async. hash and HMAC support. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig |6 + drivers/staging/ccree/Makefile |2 +- drivers/staging/ccree/cc_crypto_ctx.h | 22 + drivers/staging/ccree/hash_defs.h | 78 + drivers/staging/ccree/ssi_buffer_mgr.c | 311 +++- drivers/staging/ccree/ssi_buffer_mgr.h |6 + drivers/staging/ccree/ssi_driver.c | 11 +- drivers/staging/ccree/ssi_driver.h |4 +- drivers/staging/ccree/ssi_hash.c | 2732 drivers/staging/ccree/ssi_hash.h | 101 ++ drivers/staging/ccree/ssi_pm.c |4 + 11 files changed, 3263 insertions(+), 14 deletions(-) create mode 100644 drivers/staging/ccree/hash_defs.h create mode 100644 drivers/staging/ccree/ssi_hash.c create mode 100644 drivers/staging/ccree/ssi_hash.h diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 0f723d7..a528a99 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -2,6 +2,12 @@ config CRYPTO_DEV_CCREE tristate "Support for ARM TrustZone CryptoCell C7XX family of Crypto accelerators" depends on CRYPTO_HW && OF && HAS_DMA default n + select CRYPTO_HASH + select CRYPTO_SHA1 + select CRYPTO_MD5 + select CRYPTO_SHA256 + select CRYPTO_SHA512 + select CRYPTO_HMAC help Say 'Y' to enable a driver for the Arm TrustZone CryptoCell C7xx. Currently only the CryptoCell 712 REE is supported. diff --git a/drivers/staging/ccree/Makefile b/drivers/staging/ccree/Makefile index 972af69..f94e225 100644 --- a/drivers/staging/ccree/Makefile +++ b/drivers/staging/ccree/Makefile @@ -1,2 +1,2 @@ obj-$(CONFIG_CRYPTO_DEV_CCREE) := ccree.o -ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o +ccree-y := ssi_driver.o ssi_sysfs.o ssi_buffer_mgr.o ssi_request_mgr.o ssi_hash.o ssi_sram_mgr.o ssi_pm.o ssi_pm_ext.o diff --git a/drivers/staging/ccree/cc_crypto_ctx.h b/drivers/staging/ccree/cc_crypto_ctx.h index 3547cb4..a4aa066 100644 --- a/drivers/staging/ccree/cc_crypto_ctx.h +++ b/drivers/staging/ccree/cc_crypto_ctx.h @@ -220,6 +220,28 @@ struct drv_ctx_generic { } __attribute__((__may_alias__)); +struct drv_ctx_hash { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_HASH */ + enum drv_hash_mode mode; + uint8_t digest[CC_DIGEST_SIZE_MAX]; + /* reserve to end of allocated context size */ + uint8_t reserved[CC_CTX_SIZE - 2 * sizeof(uint32_t) - + CC_DIGEST_SIZE_MAX]; +}; + +/* drv_ctx_hmac should have the same structure as drv_ctx_hash except + k0, k0_size fields */ +struct drv_ctx_hmac { + enum drv_crypto_alg alg; /* DRV_CRYPTO_ALG_HMAC */ + enum drv_hash_mode mode; + uint8_t digest[CC_DIGEST_SIZE_MAX]; + uint32_t k0[CC_HMAC_BLOCK_SIZE_MAX/sizeof(uint32_t)]; + uint32_t k0_size; + /* reserve to end of allocated context size */ + uint8_t reserved[CC_CTX_SIZE - 3 * sizeof(uint32_t) - + CC_DIGEST_SIZE_MAX - CC_HMAC_BLOCK_SIZE_MAX]; +}; + /***/ /* MESSAGE BASED CONTEXTS **/ /***/ diff --git a/drivers/staging/ccree/hash_defs.h b/drivers/staging/ccree/hash_defs.h new file mode 100644 index 000..5ab0861 --- /dev/null +++ b/drivers/staging/ccree/hash_defs.h @@ -0,0 +1,78 @@ +/* + * Copyright (C) 2012-2017 ARM Limited or its affiliates. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef _HASH_DEFS_H__ +#define _HASH_DEFS_H__ + +#include "cc_crypto_ctx.h" + +/* this files provides definitions required for hash engine drivers */ +#ifndef CC_CONFIG_HASH_SHA_512_SUPPORTED +#define SEP_HASH_LENGTH_WORDS 2 +#else +#define SEP_HASH_LENGTH_WORDS 4 +#endif + +#ifdef BIG__ENDIAN +#define OPAD_CURRENT_LENGTH 0x4000, 0x , 0x, 0x +#define HASH_LARVAL_MD5 0x76543210, 0xFEDCBA98, 0x89ABCDEF, 0x01234567 +#define HASH_LARVAL_SHA1 0xF0E1D2C3, 0x76543210, 0xFEDCBA98, 0x89ABCDEF, 0x01234567 +#define HASH_LARVAL_SHA224 0XA44FFABE, 0XA78FF964, 0X11155868, 0X310BC0FF, 0X39590
Re: [RFC PATCH v4] IV Generation algorithms for dm-crypt
On Wed, Mar 1, 2017 at 5:38 PM, Milan Broz <gmazyl...@gmail.com> wrote: > > On 03/01/2017 02:04 PM, Milan Broz wrote: >> On 03/01/2017 01:42 PM, Gilad Ben-Yossef wrote: >> ... >> >>> I can certainly understand if you don't wont to take the patch until >>> we have results with >>> dm-crypt itself but the difference between 8 separate invocation of >>> the engine for 512 >>> bytes of XTS and a single invocation for 4KB are pretty big. >> >> Yes, I know it. But the same can be achieved if we just implement >> 4k sector encryption in dmcrypt. It is incompatible with LUKS1 >> (but next LUKS version will support it) but I think this is not >> a problem for now. >> >> If the underlying device supports atomic write of 4k sectors, then >> there should not be a problem. >> >> This is one of the speed-up I would like to compare with the IV approach, >> because everyone should benefit from 4k sectors in the end. >> And no crypto API changes are needed here. >> >> (I have an old patch for this, so I will try to revive it.) > > If anyone interested, simple experimental patch for larger sector size > (up to the page size) for dmcrypt is in this branch: > > http://git.kernel.org/cgit/linux/kernel/git/mbroz/linux.git/log/?h=dm-crypt-4k-sector > > It would be nice to check what performance gain could be provided > by this simple approach. I gave it a spin on a x86_64 with 8 CPUs with AES-NI using cryptd and on Arm using CryptoCell hardware accelerator. There was no difference in performance between 512 and 4096 bytes cluster size on the x86_64 (800 MB loop file system) There was an improvement in latency of 3.2% between 512 and 4096 bytes cluster size on the Arm. I expect the performance benefits for this test for Binoy's patch to be the same. In both cases the very naive test was a simple dd with block size of 4096 bytes or the raw block device. I do not know what effect having a bigger cluster size would have on have on other more complex file system operations. Is there any specific benchmark worth testing with? Gilad -- Gilad Ben-Yossef Chief Coffee Drinker "If you take a class in large-scale robotics, can you end up in a situation where the homework eats your dog?" -- Jean-Baptiste Queru
[PATCH v3 17/22] staging: ccree: clean up comments
Clean up comments: fix style, trim long lines and remove useless ones. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_aead.c| 40 + drivers/staging/ccree/ssi_aead.h| 47 ++--- drivers/staging/ccree/ssi_buffer_mgr.c | 52 + drivers/staging/ccree/ssi_cipher.c | 10 +-- drivers/staging/ccree/ssi_config.h | 7 +++-- drivers/staging/ccree/ssi_driver.c | 8 +++-- drivers/staging/ccree/ssi_driver.h | 9 -- drivers/staging/ccree/ssi_hash.c| 16 +++--- drivers/staging/ccree/ssi_hash.h| 10 +-- drivers/staging/ccree/ssi_ivgen.c | 7 +++-- drivers/staging/ccree/ssi_ivgen.h | 3 +- drivers/staging/ccree/ssi_request_mgr.c | 29 -- drivers/staging/ccree/ssi_sysfs.c | 9 +++--- 13 files changed, 167 insertions(+), 80 deletions(-) diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index 515a603..88305f0 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -267,7 +267,10 @@ static void ssi_aead_complete(struct device *dev, void *ssi_req, ctx->authsize, SSI_SG_FROM_BUF); - /* If an IV was generated, copy it back to the user provided buffer. */ + /* +* If an IV was generated, copy it back to the user provided +* buffer. +*/ if (areq_ctx->backup_giv) { if (ctx->cipher_mode == DRV_CIPHER_CTR) memcpy(areq_ctx->backup_giv, @@ -288,8 +291,9 @@ static int xcbc_setkey(struct cc_hw_desc *desc, struct ssi_aead_ctx *ctx) { /* Load the AES key */ hw_desc_init([0]); - /* We are using for the source/user key the same buffer as for the output keys, -* because after this key loading it is not needed anymore + /* We are using for the source/user key the same buffer as for the +* output keys, because after this key loading it is not needed +* anymore. */ set_din_type([0], DMA_DLLI, ctx->auth_state.xcbc.xcbc_keys_dma_addr, ctx->auth_keylen, @@ -1570,7 +1574,9 @@ static int config_ccm_adata(struct aead_request *req) struct aead_req_ctx *req_ctx = aead_request_ctx(req); //unsigned int size_of_a = 0, rem_a_size = 0; unsigned int lp = req->iv[0]; - /* Note: The code assume that req->iv[0] already contains the value of L' of RFC3610 */ + /* Note: The code assumes that req->iv[0] already contains the value +* of L' of RFC3610 +*/ unsigned int l = lp + 1; /* This is L' of RFC 3610. */ unsigned int m = ctx->authsize; /* This is M' of RFC 3610. */ u8 *b0 = req_ctx->ccm_config + CCM_B0_OFFSET; @@ -1624,9 +1630,14 @@ static void ssi_rfc4309_ccm_process(struct aead_request *req) /* L' */ memset(areq_ctx->ctr_iv, 0, AES_BLOCK_SIZE); - areq_ctx->ctr_iv[0] = 3; /* For RFC 4309, always use 4 bytes for message length (at most 2^32-1 bytes). */ + /* For RFC 4309, always use 4 bytes for message length +* (at most 2^32-1 bytes). +*/ + areq_ctx->ctr_iv[0] = 3; - /* In RFC 4309 there is an 11-bytes nonce+IV part, that we build here. */ + /* In RFC 4309 there is an 11-bytes nonce+IV part, that we build +* here. +*/ memcpy(areq_ctx->ctr_iv + CCM_BLOCK_NONCE_OFFSET, ctx->ctr_nonce, CCM_BLOCK_NONCE_SIZE); memcpy(areq_ctx->ctr_iv + CCM_BLOCK_IV_OFFSET, req->iv, @@ -1701,7 +1712,9 @@ static inline void ssi_aead_gcm_setup_ghash_desc(struct aead_request *req, set_setup_mode([idx], SETUP_LOAD_KEY0); idx++; - /* Load GHASH initial STATE (which is 0). (for any hash there is an initial state) */ + /* Load GHASH initial STATE (which is 0). (for any hash there is an +* initial state). +*/ hw_desc_init([idx]); set_din_const([idx], 0x0, AES_BLOCK_SIZE); set_dout_no_dma([idx], 0, 0, 1); @@ -1938,7 +1951,10 @@ static int config_gcm_context(struct aead_request *req) memcpy(_ctx->gcm_len_block.len_a, , sizeof(temp64)); temp64 = cpu_to_be64(cryptlen * 8); memcpy(_ctx->gcm_len_block.len_c, , 8); - } else { //rfc4543=> all data(AAD,IV,Plain) are considered additional data that is nothing is encrypted. + } else { + /* rfc4543=> all data(AAD,IV,Plain) are considered additional +* data that is nothing is encrypted. +*/ __be64 temp64; temp64 = @@ -
[PATCH v3 15/22] staging: ccree: fix line indentation and breaks
Fix source line indentation and breaks in ssi_aead.c Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_aead.c | 1024 -- 1 file changed, 532 insertions(+), 492 deletions(-) diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index 03533c8..515a603 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -101,7 +101,8 @@ static void ssi_aead_exit(struct crypto_aead *tfm) dev = >drvdata->plat_dev->dev; /* Unmap enckey buffer */ if (ctx->enckey) { - dma_free_coherent(dev, AES_MAX_KEY_SIZE, ctx->enckey, ctx->enckey_dma_addr); + dma_free_coherent(dev, AES_MAX_KEY_SIZE, ctx->enckey, + ctx->enckey_dma_addr); SSI_LOG_DEBUG("Freed enckey DMA buffer enckey_dma_addr=%pad\n", >enckey_dma_addr); ctx->enckey_dma_addr = 0; @@ -116,8 +117,9 @@ static void ssi_aead_exit(struct crypto_aead *tfm) xcbc->xcbc_keys, xcbc->xcbc_keys_dma_addr); } - SSI_LOG_DEBUG("Freed xcbc_keys DMA buffer xcbc_keys_dma_addr=%pad\n", - >xcbc_keys_dma_addr); + SSI_LOG_DEBUG + ("Freed xcbc_keys DMA buffer xcbc_keys_dma_addr=%pad\n", +>xcbc_keys_dma_addr); xcbc->xcbc_keys_dma_addr = 0; xcbc->xcbc_keys = NULL; } else if (ctx->auth_mode != DRV_HASH_NULL) { /* HMAC auth. */ @@ -127,8 +129,9 @@ static void ssi_aead_exit(struct crypto_aead *tfm) dma_free_coherent(dev, 2 * MAX_HMAC_DIGEST_SIZE, hmac->ipad_opad, hmac->ipad_opad_dma_addr); - SSI_LOG_DEBUG("Freed ipad_opad DMA buffer ipad_opad_dma_addr=%pad\n", - >ipad_opad_dma_addr); + SSI_LOG_DEBUG + ("Freed ipad_opad DMA buffer ipad_opad_dma_addr=%pad\n", +>ipad_opad_dma_addr); hmac->ipad_opad_dma_addr = 0; hmac->ipad_opad = NULL; } @@ -136,8 +139,9 @@ static void ssi_aead_exit(struct crypto_aead *tfm) dma_free_coherent(dev, MAX_HMAC_BLOCK_SIZE, hmac->padded_authkey, hmac->padded_authkey_dma_addr); - SSI_LOG_DEBUG("Freed padded_authkey DMA buffer padded_authkey_dma_addr=%pad\n", - >padded_authkey_dma_addr); + SSI_LOG_DEBUG + ("Freed padded_authkey DMA buffer padded_authkey_dma_addr=%pad\n", +>padded_authkey_dma_addr); hmac->padded_authkey_dma_addr = 0; hmac->padded_authkey = NULL; } @@ -150,8 +154,9 @@ static int ssi_aead_init(struct crypto_aead *tfm) struct aead_alg *alg = crypto_aead_alg(tfm); struct ssi_aead_ctx *ctx = crypto_aead_ctx(tfm); struct ssi_crypto_alg *ssi_alg = - container_of(alg, struct ssi_crypto_alg, aead_alg); - SSI_LOG_DEBUG("Initializing context @%p for %s\n", ctx, crypto_tfm_alg_name(>base)); + container_of(alg, struct ssi_crypto_alg, aead_alg); + SSI_LOG_DEBUG("Initializing context @%p for %s\n", ctx, + crypto_tfm_alg_name(>base)); /* Initialize modes in instance */ ctx->cipher_mode = ssi_alg->cipher_mode; @@ -168,7 +173,8 @@ static int ssi_aead_init(struct crypto_aead *tfm) SSI_LOG_ERR("Failed allocating key buffer\n"); goto init_failed; } - SSI_LOG_DEBUG("Allocated enckey buffer in context ctx->enckey=@%p\n", ctx->enckey); + SSI_LOG_DEBUG("Allocated enckey buffer in context ctx->enckey=@%p\n", + ctx->enckey); /* Set default authlen value */ @@ -200,13 +206,13 @@ static int ssi_aead_init(struct crypto_aead *tfm) goto init_failed; } - SSI_LOG_DEBUG("Allocated authkey buffer in context ctx->authkey=@%p\n", - hmac->ipad_opad); + SSI_LOG_DEBUG + ("Allocated authkey buffer in context ctx->authkey=@%p\n", +hmac->ipad_opad);
[PATCH v3 14/22] staging: ccree: fix struct init braces
Put struct init braces on line of it's own. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_hash.c | 32 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/drivers/staging/ccree/ssi_hash.c b/drivers/staging/ccree/ssi_hash.c index e2dc5d8..6baa449 100644 --- a/drivers/staging/ccree/ssi_hash.c +++ b/drivers/staging/ccree/ssi_hash.c @@ -41,26 +41,42 @@ struct ssi_hash_handle { }; static const u32 digest_len_init[] = { - 0x0040, 0x, 0x, 0x }; + 0x0040, 0x, 0x, 0x +}; + static const u32 md5_init[] = { - SHA1_H3, SHA1_H2, SHA1_H1, SHA1_H0 }; + SHA1_H3, SHA1_H2, SHA1_H1, SHA1_H0 +}; + static const u32 sha1_init[] = { - SHA1_H4, SHA1_H3, SHA1_H2, SHA1_H1, SHA1_H0 }; + SHA1_H4, SHA1_H3, SHA1_H2, SHA1_H1, SHA1_H0 +}; + static const u32 sha224_init[] = { SHA224_H7, SHA224_H6, SHA224_H5, SHA224_H4, - SHA224_H3, SHA224_H2, SHA224_H1, SHA224_H0 }; + SHA224_H3, SHA224_H2, SHA224_H1, SHA224_H0 +}; + static const u32 sha256_init[] = { SHA256_H7, SHA256_H6, SHA256_H5, SHA256_H4, - SHA256_H3, SHA256_H2, SHA256_H1, SHA256_H0 }; + SHA256_H3, SHA256_H2, SHA256_H1, SHA256_H0 +}; + #if (DX_DEV_SHA_MAX > 256) static const u32 digest_len_sha512_init[] = { - 0x0080, 0x, 0x, 0x }; + 0x0080, 0x, 0x, 0x +}; + static const u64 sha384_init[] = { SHA384_H7, SHA384_H6, SHA384_H5, SHA384_H4, - SHA384_H3, SHA384_H2, SHA384_H1, SHA384_H0 }; + SHA384_H3, SHA384_H2, SHA384_H1, SHA384_H0 +}; + static const u64 sha512_init[] = { SHA512_H7, SHA512_H6, SHA512_H5, SHA512_H4, - SHA512_H3, SHA512_H2, SHA512_H1, SHA512_H0 }; + SHA512_H3, SHA512_H2, SHA512_H1, SHA512_H0 +}; + #endif static void ssi_hash_create_xcbc_setup(struct ahash_request *areq, -- 2.1.4
[PATCH v3 22/22] staging: ccree: remove BUG macro usage
Replace BUG() macro usage that crash the kernel with alternatives that signal error and/or try to recover. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_buffer_mgr.c | 14 ++ drivers/staging/ccree/ssi_cipher.c | 1 - drivers/staging/ccree/ssi_pm.c | 3 ++- drivers/staging/ccree/ssi_request_mgr.c | 23 +-- 4 files changed, 25 insertions(+), 16 deletions(-) diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index 051d948..6d5af50 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -86,11 +86,6 @@ static unsigned int ssi_buffer_mgr_get_sgl_nents( unsigned int nents = 0; while (nbytes != 0) { - if (sg_is_chain(sg_list)) { - SSI_LOG_ERR("Unexpected chained entry in sg (entry =0x%X)\n", - nents); - BUG(); - } if (sg_list->length != 0) { nents++; /* get the number of bytes in the last entry */ @@ -861,7 +856,8 @@ static inline int ssi_buffer_mgr_aead_chain_assoc( * unexpected */ if (!current_sg) { SSI_LOG_ERR("reached end of sg list. unexpected\n"); - BUG(); + rc = -EINVAL; + goto chain_assoc_exit; } sg_index += current_sg->length; mapped_nents++; @@ -1163,7 +1159,8 @@ static inline int ssi_buffer_mgr_aead_chain_data( //if have reached the end of the sgl, then this is unexpected if (!areq_ctx->src_sgl) { SSI_LOG_ERR("reached end of sg list. unexpected\n"); - BUG(); + return -EINVAL; + goto chain_data_exit; } sg_index += areq_ctx->src_sgl->length; src_mapped_nents--; @@ -1207,7 +1204,8 @@ static inline int ssi_buffer_mgr_aead_chain_data( //if have reached the end of the sgl, then this is unexpected if (!areq_ctx->dst_sgl) { SSI_LOG_ERR("reached end of sg list. unexpected\n"); - BUG(); + rc = -EINVAL; + goto chain_data_exit; } sg_index += areq_ctx->dst_sgl->length; dst_mapped_nents--; diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index cc550b5..73ba4eb 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -543,7 +543,6 @@ ssi_blkcipher_create_setup_desc( break; default: SSI_LOG_ERR("Unsupported cipher mode (%d)\n", cipher_mode); - BUG(); } } diff --git a/drivers/staging/ccree/ssi_pm.c b/drivers/staging/ccree/ssi_pm.c index 31325e6..a50671a 100644 --- a/drivers/staging/ccree/ssi_pm.c +++ b/drivers/staging/ccree/ssi_pm.c @@ -109,7 +109,8 @@ int ssi_power_mgr_runtime_put_suspend(struct device *dev) rc = pm_runtime_put_autosuspend(dev); } else { /* Something wrong happens*/ - BUG(); + SSI_LOG_ERR("request to suspend already suspended queue"); + rc = -EBUSY; } return rc; } diff --git a/drivers/staging/ccree/ssi_request_mgr.c b/drivers/staging/ccree/ssi_request_mgr.c index b671eff..8bf72e7 100644 --- a/drivers/staging/ccree/ssi_request_mgr.c +++ b/drivers/staging/ccree/ssi_request_mgr.c @@ -366,11 +366,16 @@ int send_request( enqueue_seq(cc_base, _mgr_h->compl_desc, (is_dout ? 0 : 1)); if (unlikely(req_mgr_h->q_free_slots < total_seq_len)) { - /*This means that there was a problem with the resume*/ - BUG(); + /* This situation should never occur. Maybe indicating problem +* with resuming power. Set the free slot count to 0 and hope +* for the best. +*/ + SSI_LOG_ERR("HW free slot count mismatch."); + req_mgr_h->q_free_slots = 0; + } else { + /* Update the free slots in HW queue */ + req_mgr_h->q_free_slots -= total_seq_len; } - /* Update the free slots in HW queue */ - req_mgr_h->q_free_slots -= total_seq_len; spin_unlock_bh(_mgr_h->hw_lock); @@ -459,8 +464,13 @@ static void proc_completions(struct ssi_drvdata *drvdata) /* Dequeue request */ if (unlikely(request_mgr_hand
[PATCH v3 21/22] staging: ccree: save ciphertext for CTS IV
The crypto API requires saving the last blocks of ciphertext in req->info for use as IV for CTS mode. The ccree driver was not doing it and so failing tcrypt tests in some situations. This patch fixes the issue. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_cipher.c | 31 +-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index aa722e1..cc550b5 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -24,6 +24,7 @@ #include #include #include +#include #include "ssi_config.h" #include "ssi_driver.h" @@ -696,6 +697,7 @@ static int ssi_blkcipher_complete(struct device *dev, { int completion_error = 0; u32 inflight_counter; + struct ablkcipher_request *req = (struct ablkcipher_request *)areq; ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst); @@ -706,6 +708,22 @@ static int ssi_blkcipher_complete(struct device *dev, ctx_p->drvdata->inflight_counter--; if (areq) { + /* +* The crypto API expects us to set the req->info to the last +* ciphertext block. For encrypt, simply copy from the result. +* For decrypt, we must copy from a saved buffer since this +* could be an in-place decryption operation and the src is +* lost by this point. +*/ + if (req_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) { + memcpy(req->info, req_ctx->backup_info, ivsize); + kfree(req_ctx->backup_info); + } else { + scatterwalk_map_and_copy(req->info, req->dst, +(req->nbytes - ivsize), +ivsize, 0); + } + ablkcipher_request_complete(areq, completion_error); return 0; } @@ -859,7 +877,6 @@ static int ssi_ablkcipher_encrypt(struct ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = ablkcipher_request_ctx(req); unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); - req_ctx->backup_info = req->info; req_ctx->is_giv = false; return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_ENCRYPT); @@ -872,8 +889,18 @@ static int ssi_ablkcipher_decrypt(struct ablkcipher_request *req) struct blkcipher_req_ctx *req_ctx = ablkcipher_request_ctx(req); unsigned int ivsize = crypto_ablkcipher_ivsize(ablk_tfm); - req_ctx->backup_info = req->info; + /* +* Allocate and save the last IV sized bytes of the source, which will +* be lost in case of in-place decryption and might be needed for CTS. +*/ + req_ctx->backup_info = kmalloc(ivsize, GFP_KERNEL); + if (!req_ctx->backup_info) + return -ENOMEM; + + scatterwalk_map_and_copy(req_ctx->backup_info, req->src, +(req->nbytes - ivsize), ivsize, 0); req_ctx->is_giv = false; + return ssi_blkcipher_process(tfm, req_ctx, req->dst, req->src, req->nbytes, req->info, ivsize, (void *)req, DRV_CRYPTO_DIRECTION_DECRYPT); } -- 2.1.4
[PATCH v3 18/22] staging: ccree: move over to BIT macro for bit defines
Use BIT macro for bit definitions where needed. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_cipher.h | 10 +- drivers/staging/ccree/ssi_driver.c | 3 ++- drivers/staging/ccree/ssi_driver.h | 6 +++--- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/drivers/staging/ccree/ssi_cipher.h b/drivers/staging/ccree/ssi_cipher.h index 296b375..c9a83df 100644 --- a/drivers/staging/ccree/ssi_cipher.h +++ b/drivers/staging/ccree/ssi_cipher.h @@ -27,11 +27,11 @@ #include "ssi_buffer_mgr.h" /* Crypto cipher flags */ -#define CC_CRYPTO_CIPHER_KEY_KFDE0(1 << 0) -#define CC_CRYPTO_CIPHER_KEY_KFDE1(1 << 1) -#define CC_CRYPTO_CIPHER_KEY_KFDE2(1 << 2) -#define CC_CRYPTO_CIPHER_KEY_KFDE3(1 << 3) -#define CC_CRYPTO_CIPHER_DU_SIZE_512B (1 << 4) +#define CC_CRYPTO_CIPHER_KEY_KFDE0 BIT(0) +#define CC_CRYPTO_CIPHER_KEY_KFDE1 BIT(1) +#define CC_CRYPTO_CIPHER_KEY_KFDE2 BIT(2) +#define CC_CRYPTO_CIPHER_KEY_KFDE3 BIT(3) +#define CC_CRYPTO_CIPHER_DU_SIZE_512B BIT(4) #define CC_CRYPTO_CIPHER_KEY_KFDE_MASK (CC_CRYPTO_CIPHER_KEY_KFDE0 | CC_CRYPTO_CIPHER_KEY_KFDE1 | CC_CRYPTO_CIPHER_KEY_KFDE2 | CC_CRYPTO_CIPHER_KEY_KFDE3) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 91c0b71..6ec5287 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -202,7 +202,8 @@ int init_cc_regs(struct ssi_drvdata *drvdata, bool is_probe) CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_ICR), val); /* Unmask relevant interrupt cause */ - val = (~(SSI_COMP_IRQ_MASK | SSI_AXI_ERR_IRQ_MASK | SSI_GPR0_IRQ_MASK)); + val = (unsigned int)(~(SSI_COMP_IRQ_MASK | SSI_AXI_ERR_IRQ_MASK | + SSI_GPR0_IRQ_MASK)); CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IMR), val); #ifdef DX_HOST_IRQ_TIMER_INIT_VAL_REG_OFFSET diff --git a/drivers/staging/ccree/ssi_driver.h b/drivers/staging/ccree/ssi_driver.h index e37a55a..0b9c7e6 100644 --- a/drivers/staging/ccree/ssi_driver.h +++ b/drivers/staging/ccree/ssi_driver.h @@ -68,12 +68,12 @@ #define SSI_AXI_IRQ_MASK ((1 << DX_AXIM_CFG_BRESPMASK_BIT_SHIFT) | (1 << DX_AXIM_CFG_RRESPMASK_BIT_SHIFT) |\ (1 << DX_AXIM_CFG_INFLTMASK_BIT_SHIFT) | (1 << DX_AXIM_CFG_COMPMASK_BIT_SHIFT)) -#define SSI_AXI_ERR_IRQ_MASK (1 << DX_HOST_IRR_AXI_ERR_INT_BIT_SHIFT) +#define SSI_AXI_ERR_IRQ_MASK BIT(DX_HOST_IRR_AXI_ERR_INT_BIT_SHIFT) -#define SSI_COMP_IRQ_MASK (1 << DX_HOST_IRR_AXIM_COMP_INT_BIT_SHIFT) +#define SSI_COMP_IRQ_MASK BIT(DX_HOST_IRR_AXIM_COMP_INT_BIT_SHIFT) /* TEE FIPS status interrupt */ -#define SSI_GPR0_IRQ_MASK (1 << DX_HOST_IRR_GPR0_BIT_SHIFT) +#define SSI_GPR0_IRQ_MASK BIT(DX_HOST_IRR_GPR0_BIT_SHIFT) #define SSI_CRA_PRIO 3000 -- 2.1.4
[PATCH v3 19/22] staging: ccree: fix code indent
Fix multiple code indentation issues. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_buffer_mgr.c | 23 --- drivers/staging/ccree/ssi_cipher.c | 2 +- drivers/staging/ccree/ssi_sysfs.c | 4 +++- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index 202387b..051d948 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -420,8 +420,8 @@ ssi_aead_handle_config_buf(struct device *dev, sg_init_one(_ctx->ccm_adata_sg, config_data, AES_BLOCK_SIZE + areq_ctx->ccm_hdr_size); if (unlikely(dma_map_sg(dev, _ctx->ccm_adata_sg, 1, DMA_TO_DEVICE) != 1)) { - SSI_LOG_ERR("dma_map_sg() config buffer failed\n"); - return -ENOMEM; + SSI_LOG_ERR("dma_map_sg() config buffer failed\n"); + return -ENOMEM; } SSI_LOG_DEBUG("Mapped curr_buff: dma_address=%pad page=%p addr=%pK " "offset=%u length=%u\n", @@ -451,8 +451,8 @@ static inline int ssi_ahash_handle_curr_buf(struct device *dev, sg_init_one(areq_ctx->buff_sg, curr_buff, curr_buff_cnt); if (unlikely(dma_map_sg(dev, areq_ctx->buff_sg, 1, DMA_TO_DEVICE) != 1)) { - SSI_LOG_ERR("dma_map_sg() src buffer failed\n"); - return -ENOMEM; + SSI_LOG_ERR("dma_map_sg() src buffer failed\n"); + return -ENOMEM; } SSI_LOG_DEBUG("Mapped curr_buff: dma_address=%pad page=%p addr=%pK " "offset=%u length=%u\n", @@ -1050,15 +1050,16 @@ static inline int ssi_buffer_mgr_prepare_aead_data_mlli( * verification is made by CPU compare in order to * simplify MAC verification upon request completion */ - u32 size_to_skip = req->assoclen; + u32 size_to_skip = req->assoclen; - if (areq_ctx->is_gcm4543) - size_to_skip += crypto_aead_ivsize(tfm); + if (areq_ctx->is_gcm4543) + size_to_skip += crypto_aead_ivsize(tfm); - ssi_buffer_mgr_copy_scatterlist_portion( - areq_ctx->backup_mac, req->src, - size_to_skip + req->cryptlen - areq_ctx->req_authsize, - size_to_skip + req->cryptlen, SSI_SG_TO_BUF); + ssi_buffer_mgr_copy_scatterlist_portion(areq_ctx->backup_mac, + req->src, + size_to_skip + req->cryptlen - areq_ctx->req_authsize, + size_to_skip + req->cryptlen, + SSI_SG_TO_BUF); areq_ctx->icv_virt_addr = areq_ctx->backup_mac; } else { /* Contig. ICV */ /*Should hanlde if the sg is not contig.*/ diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 14930ce..aa722e1 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -139,7 +139,7 @@ static int validate_data_size(struct ssi_ablkcipher_ctx *ctx_p, unsigned int siz break; case S_DIN_to_DES: if (likely(IS_ALIGNED(size, DES_BLOCK_SIZE))) - return 0; + return 0; break; #if SSI_CC_HAS_MULTI2 case S_DIN_to_MULTI2: diff --git a/drivers/staging/ccree/ssi_sysfs.c b/drivers/staging/ccree/ssi_sysfs.c index a0ab3c6..40cd3be2 100644 --- a/drivers/staging/ccree/ssi_sysfs.c +++ b/drivers/staging/ccree/ssi_sysfs.c @@ -316,7 +316,9 @@ static ssize_t ssi_sys_help_show(struct kobject *kobj, offset += scnprintf(buf + offset, PAGE_SIZE - offset, "Usage:\n"); for (i = 0; i < ARRAY_SIZE(help_str); i += 2) - offset += scnprintf(buf + offset, PAGE_SIZE - offset, "%s\t\t%s\n", help_str[i], help_str[i + 1]); + offset += scnprintf(buf + offset, PAGE_SIZE - offset, + "%s\t\t%s\n", help_str[i], + help_str[i + 1]); return offset; } -- 2.1.4
[PATCH v3 20/22] staging: ccree: replace noop macro with inline
Replace noop macro with a noop inline function Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_driver.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/ccree/ssi_driver.h b/drivers/staging/ccree/ssi_driver.h index 0b9c7e6..063a1cc 100644 --- a/drivers/staging/ccree/ssi_driver.h +++ b/drivers/staging/ccree/ssi_driver.h @@ -190,8 +190,8 @@ struct async_gen_req_ctx { #ifdef DX_DUMP_BYTES void dump_byte_array(const char *name, const u8 *the_array, unsigned long size); #else -#define dump_byte_array(name, array, size) do {\ -} while (0); +static inline void dump_byte_array(const char *name, const u8 *the_array, + unsigned long size) {}; #endif int init_cc_regs(struct ssi_drvdata *drvdata, bool is_probe); -- 2.1.4
[PATCH v3 13/22] staging: ccree: fix line indentation and breaks
Fix source line indentation and breaks Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_hash.c | 494 ++- 1 file changed, 284 insertions(+), 210 deletions(-) diff --git a/drivers/staging/ccree/ssi_hash.c b/drivers/staging/ccree/ssi_hash.c index b95c3ce..e2dc5d8 100644 --- a/drivers/staging/ccree/ssi_hash.c +++ b/drivers/staging/ccree/ssi_hash.c @@ -33,7 +33,6 @@ #define SSI_MAX_AHASH_SEQ_LEN 12 #define SSI_MAX_HASH_OPAD_TMP_KEYS_SIZE MAX(SSI_MAX_HASH_BLCK_SIZE, 3 * AES_BLOCK_SIZE) - struct ssi_hash_handle { ssi_sram_addr_t digest_len_sram_addr; /* const value in SRAM*/ ssi_sram_addr_t larval_digest_sram_addr; /* const value in SRAM */ @@ -64,10 +63,9 @@ static const u64 sha512_init[] = { SHA512_H3, SHA512_H2, SHA512_H1, SHA512_H0 }; #endif -static void ssi_hash_create_xcbc_setup( - struct ahash_request *areq, - struct cc_hw_desc desc[], - unsigned int *seq_size); +static void ssi_hash_create_xcbc_setup(struct ahash_request *areq, + struct cc_hw_desc desc[], + unsigned int *seq_size); static void ssi_hash_create_cmac_setup(struct ahash_request *areq, struct cc_hw_desc desc[], @@ -94,7 +92,8 @@ struct ssi_hash_ctx { * the initial digest if HASH. */ u8 digest_buff[SSI_MAX_HASH_DIGEST_SIZE] cacheline_aligned; - u8 opad_tmp_keys_buff[SSI_MAX_HASH_OPAD_TMP_KEYS_SIZE] cacheline_aligned; + u8 opad_tmp_keys_buff[SSI_MAX_HASH_OPAD_TMP_KEYS_SIZE] + cacheline_aligned; dma_addr_t opad_tmp_keys_dma_addr cacheline_aligned; dma_addr_t digest_buff_dma_addr; @@ -107,18 +106,17 @@ struct ssi_hash_ctx { bool is_hmac; }; -static void ssi_hash_create_data_desc( - struct ahash_req_ctx *areq_ctx, - struct ssi_hash_ctx *ctx, - unsigned int flow_mode, struct cc_hw_desc desc[], - bool is_not_last_data, - unsigned int *seq_size); +static void ssi_hash_create_data_desc(struct ahash_req_ctx *areq_ctx, + struct ssi_hash_ctx *ctx, + unsigned int flow_mode, + struct cc_hw_desc desc[], + bool is_not_last_data, + unsigned int *seq_size); static inline void ssi_set_hash_endianity(u32 mode, struct cc_hw_desc *desc) { if (unlikely((mode == DRV_HASH_MD5) || -(mode == DRV_HASH_SHA384) || -(mode == DRV_HASH_SHA512))) { +(mode == DRV_HASH_SHA384) || (mode == DRV_HASH_SHA512))) { set_bytes_swap(desc, 1); } else { set_cipher_config0(desc, HASH_DIGEST_RESULT_LITTLE_ENDIAN); @@ -130,17 +128,18 @@ static int ssi_hash_map_result(struct device *dev, unsigned int digestsize) { state->digest_result_dma_addr = - dma_map_single(dev, (void *)state->digest_result_buff, - digestsize, - DMA_BIDIRECTIONAL); + dma_map_single(dev, (void *)state->digest_result_buff, + digestsize, DMA_BIDIRECTIONAL); if (unlikely(dma_mapping_error(dev, state->digest_result_dma_addr))) { - SSI_LOG_ERR("Mapping digest result buffer %u B for DMA failed\n", - digestsize); + SSI_LOG_ERR + ("Mapping digest result buffer %u B for DMA failed\n", +digestsize); return -ENOMEM; } - SSI_LOG_DEBUG("Mapped digest result buffer %u B at va=%pK to dma=%pad\n", - digestsize, state->digest_result_buff, - >digest_result_dma_addr); + SSI_LOG_DEBUG + ("Mapped digest result buffer %u B at va=%pK to dma=%pad\n", +digestsize, state->digest_result_buff, +>digest_result_dma_addr); return 0; } @@ -150,8 +149,8 @@ static int ssi_hash_map_request(struct device *dev, struct ssi_hash_ctx *ctx) { bool is_hmac = ctx->is_hmac; - ssi_sram_addr_t larval_digest_addr = ssi_ahash_get_larval_digest_sram_addr( - ctx->drvdata, ctx->hash_mode); + ssi_sram_addr_t larval_digest_addr = + ssi_ahash_get_larval_digest_sram_addr(ctx->drvdata, ctx->hash_mode); struct ssi_crypto_req ssi_req = {}; struct cc_hw_desc desc; int rc = -ENOMEM; @@ -166,40 +165,56 @@ static int ssi_hash_map_request(struct device *dev, SSI_LOG_ERR("Allocating buff1 in context failed\n");
[PATCH v3 08/22] staging: ccree: remove m32r as supported platform
M32R requires special handling due due to how it has implemented ioread32. It is also an orphaned arch on Linux and doesn't seem to be worth the trouble. So until we have a real user, remove support for it. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/ccree/Kconfig b/drivers/staging/ccree/Kconfig index 0b3092b..89af1c5 100644 --- a/drivers/staging/ccree/Kconfig +++ b/drivers/staging/ccree/Kconfig @@ -1,6 +1,6 @@ config CRYPTO_DEV_CCREE tristate "Support for ARM TrustZone CryptoCell C7XX family of Crypto accelerators" - depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA + depends on CRYPTO && CRYPTO_HW && OF && HAS_DMA && !M32R default n select CRYPTO_HASH select CRYPTO_BLKCIPHER -- 2.1.4
[PATCH v3 09/22] staging: ccree: Fix format/argument mismatches
From: Joe Perches <j...@perches.com> By default, debug logging is disabled by CC_DEBUG not being defined. Convert SSI_LOG_DEBUG to use no_printk instead of an empty define to validate formats and arguments. Fix fallout. Miscellanea: o One of the conversions now uses %pR instead of multiple uses of %pad Signed-off-by: Joe Perches <j...@perches.com> [ gby: rebase on top of latest changes ] Acked-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_aead.c| 8 drivers/staging/ccree/ssi_buffer_mgr.c | 29 +--- drivers/staging/ccree/ssi_cipher.c | 10 +- drivers/staging/ccree/ssi_driver.c | 10 -- drivers/staging/ccree/ssi_driver.h | 2 +- drivers/staging/ccree/ssi_hash.c| 34 - drivers/staging/ccree/ssi_request_mgr.c | 6 +++--- 7 files changed, 47 insertions(+), 52 deletions(-) diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index 66eedbe..03533c8 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -103,7 +103,7 @@ static void ssi_aead_exit(struct crypto_aead *tfm) if (ctx->enckey) { dma_free_coherent(dev, AES_MAX_KEY_SIZE, ctx->enckey, ctx->enckey_dma_addr); SSI_LOG_DEBUG("Freed enckey DMA buffer enckey_dma_addr=%pad\n", - ctx->enckey_dma_addr); + >enckey_dma_addr); ctx->enckey_dma_addr = 0; ctx->enckey = NULL; } @@ -117,7 +117,7 @@ static void ssi_aead_exit(struct crypto_aead *tfm) xcbc->xcbc_keys_dma_addr); } SSI_LOG_DEBUG("Freed xcbc_keys DMA buffer xcbc_keys_dma_addr=%pad\n", - xcbc->xcbc_keys_dma_addr); + >xcbc_keys_dma_addr); xcbc->xcbc_keys_dma_addr = 0; xcbc->xcbc_keys = NULL; } else if (ctx->auth_mode != DRV_HASH_NULL) { /* HMAC auth. */ @@ -128,7 +128,7 @@ static void ssi_aead_exit(struct crypto_aead *tfm) hmac->ipad_opad, hmac->ipad_opad_dma_addr); SSI_LOG_DEBUG("Freed ipad_opad DMA buffer ipad_opad_dma_addr=%pad\n", - hmac->ipad_opad_dma_addr); + >ipad_opad_dma_addr); hmac->ipad_opad_dma_addr = 0; hmac->ipad_opad = NULL; } @@ -137,7 +137,7 @@ static void ssi_aead_exit(struct crypto_aead *tfm) hmac->padded_authkey, hmac->padded_authkey_dma_addr); SSI_LOG_DEBUG("Freed padded_authkey DMA buffer padded_authkey_dma_addr=%pad\n", - hmac->padded_authkey_dma_addr); + >padded_authkey_dma_addr); hmac->padded_authkey_dma_addr = 0; hmac->padded_authkey = NULL; } diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index d7ce293..0b81fd5 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -14,6 +14,7 @@ * along with this program; if not, see <http://www.gnu.org/licenses/>. */ +#include #include #include #include @@ -33,14 +34,10 @@ #include "ssi_hash.h" #include "ssi_aead.h" -#ifdef CC_DEBUG #define GET_DMA_BUFFER_TYPE(buff_type) ( \ ((buff_type) == SSI_DMA_BUF_NULL) ? "BUF_NULL" : \ ((buff_type) == SSI_DMA_BUF_DLLI) ? "BUF_DLLI" : \ ((buff_type) == SSI_DMA_BUF_MLLI) ? "BUF_MLLI" : "BUF_INVALID") -#else -#define GET_DMA_BUFFER_TYPE(buff_type) -#endif enum dma_buffer_type { DMA_NULL_TYPE = -1, @@ -260,7 +257,7 @@ static int ssi_buffer_mgr_generate_mlli( mlli_params->mlli_len = (total_nents * LLI_ENTRY_BYTE_SIZE); SSI_LOG_DEBUG("MLLI params: virt_addr=%pK dma_addr=%pad mlli_len=0x%X\n", - mlli_params->mlli_virt_addr, mlli_params->mlli_dma_addr, + mlli_params->mlli_virt_addr, _params->mlli_dma_addr, mlli_params->mlli_len); build_mlli_exit: @@ -275,7 +272,7 @@ static inline void ssi_buffer_mgr_add_buffer_entry( unsigned int index = sgl_data->num_of_buffers; SSI_LOG_DEBUG("index=%u single_buff=%pad buffer_len=0x%08X is_last=%d\n", - index, buffer_dma, buffer_len, is_last_entry); +
[PATCH v3 06/22] staging: ccree: simplify resource release on error
The resource release on probe/init error was being handled in an awkward manner and possibly leaking memory on certain (unlikely) error path. Fix it by simplifying the error resource release and making it easier to track. Reported-by: Dan Carpenter <dan.carpen...@oracle.com> Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_aead.c | 3 +- drivers/staging/ccree/ssi_cipher.c | 3 +- drivers/staging/ccree/ssi_driver.c | 102 - drivers/staging/ccree/ssi_hash.c | 3 +- 4 files changed, 59 insertions(+), 52 deletions(-) diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index a8cb432..66eedbe 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -2719,6 +2719,7 @@ int ssi_aead_alloc(struct ssi_drvdata *drvdata) goto fail0; } + INIT_LIST_HEAD(_handle->aead_list); drvdata->aead_handle = aead_handle; aead_handle->sram_workspace_addr = ssi_sram_mgr_alloc( @@ -2729,8 +2730,6 @@ int ssi_aead_alloc(struct ssi_drvdata *drvdata) goto fail1; } - INIT_LIST_HEAD(_handle->aead_list); - /* Linux crypto */ for (alg = 0; alg < ARRAY_SIZE(aead_algs); alg++) { t_alg = ssi_aead_create_alg(_algs[alg]); diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index d98178d..712b21d 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -1281,9 +1281,8 @@ int ssi_ablkcipher_alloc(struct ssi_drvdata *drvdata) if (!ablkcipher_handle) return -ENOMEM; - drvdata->blkcipher_handle = ablkcipher_handle; - INIT_LIST_HEAD(_handle->blkcipher_alg_list); + drvdata->blkcipher_handle = ablkcipher_handle; /* Linux crypto */ SSI_LOG_DEBUG("Number of algorithms = %zu\n", ARRAY_SIZE(blkcipher_algs)); diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index c18e7e3..1b95f90 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -233,16 +233,14 @@ static int init_cc_resources(struct platform_device *plat_dev) if (!new_drvdata) { SSI_LOG_ERR("Failed to allocate drvdata"); rc = -ENOMEM; - goto init_cc_res_err; + goto post_drvdata_err; } + dev_set_drvdata(_dev->dev, new_drvdata); + new_drvdata->plat_dev = plat_dev; new_drvdata->clk = of_clk_get(np, 0); new_drvdata->coherent = of_dma_is_coherent(np); - /*Initialize inflight counter used in dx_ablkcipher_secure_complete used for count of BYSPASS blocks operations*/ - new_drvdata->inflight_counter = 0; - - dev_set_drvdata(_dev->dev, new_drvdata); /* Get device resources */ /* First CC registers space */ req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0); @@ -250,38 +248,42 @@ static int init_cc_resources(struct platform_device *plat_dev) new_drvdata->cc_base = devm_ioremap_resource(_dev->dev, req_mem_cc_regs); if (IS_ERR(new_drvdata->cc_base)) { + SSI_LOG_ERR("Failed to ioremap registers"); rc = PTR_ERR(new_drvdata->cc_base); - goto init_cc_res_err; + goto post_drvdata_err; } + SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n", req_mem_cc_regs->name, req_mem_cc_regs->start, req_mem_cc_regs->end); SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n", _mem_cc_regs->start, new_drvdata->cc_base); + cc_base = new_drvdata->cc_base; + /* Then IRQ */ new_drvdata->irq = platform_get_irq(plat_dev, 0); if (new_drvdata->irq < 0) { SSI_LOG_ERR("Failed getting IRQ resource\n"); rc = new_drvdata->irq; - goto init_cc_res_err; + goto post_drvdata_err; } + rc = devm_request_irq(_dev->dev, new_drvdata->irq, cc_isr, IRQF_SHARED, "arm_cc7x", new_drvdata); if (rc) { SSI_LOG_ERR("Could not register to interrupt %d\n", new_drvdata->irq); - goto init_cc_res_err; + goto post_drvdata_err; } - init_completion(_drvdata->icache_setup_completion); - SSI_LOG_DEBUG("Registered to IRQ: %d\n", new_drvdata->irq); - new_drvdata->plat_dev = plat_dev; + + init_completion(_drvdata->icache_setup_completion); rc =
[PATCH v3 11/22] staging: ccree: fix line indentation and breaks
Fix wrong indentation and line breaks, including missing tabs, breaking lines longer then 80 char or wrongly broken. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_driver.c | 107 +++-- 1 file changed, 67 insertions(+), 40 deletions(-) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 81cb63d..0ce2f57 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -87,27 +87,31 @@ void dump_byte_array(const char *name, const u8 *the_array, unsigned long size) ret = snprintf(line_buf, sizeof(line_buf), "%s[%lu]: ", name, size); if (ret < 0) { - SSI_LOG_ERR("snprintf returned %d . aborting buffer array dump\n", ret); + SSI_LOG_ERR + ("snprintf returned %d . aborting buffer array dump\n", +ret); return; } line_offset = ret; for (i = 0, cur_byte = the_array; (i < size) && (line_offset < sizeof(line_buf)); i++, cur_byte++) { - ret = snprintf(line_buf + line_offset, - sizeof(line_buf) - line_offset, - "0x%02X ", *cur_byte); + ret = snprintf(line_buf + line_offset, + sizeof(line_buf) - line_offset, + "0x%02X ", *cur_byte); if (ret < 0) { - SSI_LOG_ERR("snprintf returned %d . aborting buffer array dump\n", ret); + SSI_LOG_ERR + ("snprintf returned %d . aborting buffer array dump\n", +ret); return; } line_offset += ret; - if (line_offset > 75) { /* Cut before line end */ + if (line_offset > 75) { /* Cut before line end */ SSI_LOG_DEBUG("%s\n", line_buf); line_offset = 0; } } - if (line_offset > 0) /* Dump remaining line */ + if (line_offset > 0)/* Dump remaining line */ SSI_LOG_DEBUG("%s\n", line_buf); } #endif @@ -124,7 +128,7 @@ static irqreturn_t cc_isr(int irq, void *dev_id) /* read the interrupt status */ irr = CC_HAL_READ_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IRR)); SSI_LOG_DEBUG("Got IRR=0x%08X\n", irr); - if (unlikely(irr == 0)) { /* Probably shared interrupt line */ + if (unlikely(irr == 0)) { /* Probably shared interrupt line */ SSI_LOG_ERR("Got interrupt with empty IRR\n"); return IRQ_NONE; } @@ -137,7 +141,8 @@ static irqreturn_t cc_isr(int irq, void *dev_id) /* Completion interrupt - most probable */ if (likely((irr & SSI_COMP_IRQ_MASK) != 0)) { /* Mask AXI completion interrupt - will be unmasked in Deferred service handler */ - CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IMR), imr | SSI_COMP_IRQ_MASK); + CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IMR), + imr | SSI_COMP_IRQ_MASK); irr &= ~SSI_COMP_IRQ_MASK; complete_request(drvdata); } @@ -145,7 +150,8 @@ static irqreturn_t cc_isr(int irq, void *dev_id) /* TEE FIPS interrupt */ if (likely((irr & SSI_GPR0_IRQ_MASK) != 0)) { /* Mask interrupt - will be unmasked in Deferred service handler */ - CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IMR), imr | SSI_GPR0_IRQ_MASK); + CC_HAL_WRITE_REGISTER(CC_REG_OFFSET(HOST_RGF, HOST_IMR), + imr | SSI_GPR0_IRQ_MASK); irr &= ~SSI_GPR0_IRQ_MASK; fips_handler(drvdata); } @@ -155,14 +161,18 @@ static irqreturn_t cc_isr(int irq, void *dev_id) u32 axi_err; /* Read the AXI error ID */ - axi_err = CC_HAL_READ_REGISTER(CC_REG_OFFSET(CRY_KERNEL, AXIM_MON_ERR)); - SSI_LOG_DEBUG("AXI completion error: axim_mon_err=0x%08X\n", axi_err); + axi_err = + CC_HAL_READ_REGISTER(CC_REG_OFFSET +(CRY_KERNEL, AXIM_MON_ERR)); + SSI_LOG_DEBUG("AXI completion error: axim_mon_err=0x%08X\n", + axi_err); irr &= ~SSI_AXI_ERR_IRQ_MASK; } if (unlikely(irr != 0)) { - SSI_LOG_DEBUG("IRR includes unknown cause bits (0x%08X)\n", irr); + SSI_LOG_DEBUG("IRR includes unknown cause bits (0x%08X)\n&qu
[PATCH v3 05/22] staging: ccree: Use platform_get_irq and devm_request_irq
From: Suniel Mahesh <suni...@techveda.org> It is recommended to use managed function devm_request_irq(), which simplifies driver cleanup paths and driver code. This patch does the following: (a) replace platform_get_resource(), request_irq() and corresponding error handling with platform_get_irq() and devm_request_irq(). (b) remove struct resource pointer(res_irq) in struct ssi_drvdata as it seems redundant. (c) change type of member irq in struct ssi_drvdata from unsigned int to int, as return type of platform_get_irq is int and can be used in error handling. (d) remove irq_registered variable from driver probe as it seems redundant. (e) free_irq is not required any more, devm_request_irq() free's it on driver detach. (f) adjust log messages accordingly and remove any blank lines. Signed-off-by: Suniel Mahesh <suni...@techveda.org> Acked-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_driver.c | 30 +- drivers/staging/ccree/ssi_driver.h | 3 +-- 2 files changed, 10 insertions(+), 23 deletions(-) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 603eb03..c18e7e3 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -222,7 +222,6 @@ static int init_cc_resources(struct platform_device *plat_dev) { struct resource *req_mem_cc_regs = NULL; void __iomem *cc_base = NULL; - bool irq_registered = false; struct ssi_drvdata *new_drvdata; struct device *dev = _dev->dev; struct device_node *np = dev->of_node; @@ -262,26 +261,22 @@ static int init_cc_resources(struct platform_device *plat_dev) _mem_cc_regs->start, new_drvdata->cc_base); cc_base = new_drvdata->cc_base; /* Then IRQ */ - new_drvdata->res_irq = platform_get_resource(plat_dev, IORESOURCE_IRQ, 0); - if (unlikely(!new_drvdata->res_irq)) { + new_drvdata->irq = platform_get_irq(plat_dev, 0); + if (new_drvdata->irq < 0) { SSI_LOG_ERR("Failed getting IRQ resource\n"); - rc = -ENODEV; + rc = new_drvdata->irq; goto init_cc_res_err; } - rc = request_irq(new_drvdata->res_irq->start, cc_isr, -IRQF_SHARED, "arm_cc7x", new_drvdata); - if (unlikely(rc != 0)) { - SSI_LOG_ERR("Could not register to interrupt %llu\n", - (unsigned long long)new_drvdata->res_irq->start); + rc = devm_request_irq(_dev->dev, new_drvdata->irq, cc_isr, + IRQF_SHARED, "arm_cc7x", new_drvdata); + if (rc) { + SSI_LOG_ERR("Could not register to interrupt %d\n", + new_drvdata->irq); goto init_cc_res_err; } init_completion(_drvdata->icache_setup_completion); - irq_registered = true; - SSI_LOG_DEBUG("Registered to IRQ (%s) %llu\n", - new_drvdata->res_irq->name, - (unsigned long long)new_drvdata->res_irq->start); - + SSI_LOG_DEBUG("Registered to IRQ: %d\n", new_drvdata->irq); new_drvdata->plat_dev = plat_dev; rc = cc_clk_on(new_drvdata); @@ -410,10 +405,6 @@ static int init_cc_resources(struct platform_device *plat_dev) #ifdef ENABLE_CC_SYSFS ssi_sysfs_fini(); #endif - if (irq_registered) { - free_irq(new_drvdata->res_irq->start, new_drvdata); - new_drvdata->res_irq = NULL; - } dev_set_drvdata(_dev->dev, NULL); } return rc; @@ -443,11 +434,8 @@ static void cleanup_cc_resources(struct platform_device *plat_dev) #ifdef ENABLE_CC_SYSFS ssi_sysfs_fini(); #endif - fini_cc_regs(drvdata); cc_clk_off(drvdata); - free_irq(drvdata->res_irq->start, drvdata); - drvdata->res_irq = NULL; dev_set_drvdata(_dev->dev, NULL); } diff --git a/drivers/staging/ccree/ssi_driver.h b/drivers/staging/ccree/ssi_driver.h index 518c0bf..88ef370 100644 --- a/drivers/staging/ccree/ssi_driver.h +++ b/drivers/staging/ccree/ssi_driver.h @@ -128,9 +128,8 @@ struct ssi_crypto_req { * @fw_ver:SeP loaded firmware version */ struct ssi_drvdata { - struct resource *res_irq; void __iomem *cc_base; - unsigned int irq; + int irq; u32 irq_mask; u32 fw_ver; /* Calibration time of start/stop -- 2.1.4
[PATCH v3 03/22] staging: ccree: Replace kzalloc with devm_kzalloc
From: Suniel Mahesh <suni...@techveda.org> It is recommended to use managed function devm_kzalloc, which simplifies driver cleanup paths and driver code. This patch does the following: (a) replace kzalloc with devm_kzalloc. (b) drop kfree(), because memory allocated with devm_kzalloc() is automatically freed on driver detach, otherwise it leads to a double free. (c) remove unnecessary blank lines. Signed-off-by: Suniel Mahesh <suni...@techveda.org> [gby: rebase on top of latest coding style fixes changes] Acked-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_driver.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 1cae2b7..97dfc2c 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -223,13 +223,15 @@ static int init_cc_resources(struct platform_device *plat_dev) struct resource *req_mem_cc_regs = NULL; void __iomem *cc_base = NULL; bool irq_registered = false; - struct ssi_drvdata *new_drvdata = kzalloc(sizeof(*new_drvdata), GFP_KERNEL); + struct ssi_drvdata *new_drvdata; struct device *dev = _dev->dev; struct device_node *np = dev->of_node; u32 signature_val; int rc = 0; - if (unlikely(!new_drvdata)) { + new_drvdata = devm_kzalloc(_dev->dev, sizeof(*new_drvdata), + GFP_KERNEL); + if (!new_drvdata) { SSI_LOG_ERR("Failed to allocate drvdata"); rc = -ENOMEM; goto init_cc_res_err; @@ -434,10 +436,8 @@ static int init_cc_resources(struct platform_device *plat_dev) resource_size(new_drvdata->res_mem)); new_drvdata->res_mem = NULL; } - kfree(new_drvdata); dev_set_drvdata(_dev->dev, NULL); } - return rc; } @@ -478,8 +478,6 @@ static void cleanup_cc_resources(struct platform_device *plat_dev) drvdata->cc_base = NULL; drvdata->res_mem = NULL; } - - kfree(drvdata); dev_set_drvdata(_dev->dev, NULL); } -- 2.1.4
[PATCH v3 02/22] staging: ccree: kmalloc by sizeof var not type
Change places where we alloc memory by sizeof type to sizeof var. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_aead.c| 4 ++-- drivers/staging/ccree/ssi_cipher.c | 4 ++-- drivers/staging/ccree/ssi_driver.c | 2 +- drivers/staging/ccree/ssi_hash.c| 4 ++-- drivers/staging/ccree/ssi_ivgen.c | 2 +- drivers/staging/ccree/ssi_request_mgr.c | 2 +- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index d8f2249..a8cb432 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -2658,7 +2658,7 @@ static struct ssi_crypto_alg *ssi_aead_create_alg(struct ssi_alg_template *templ struct ssi_crypto_alg *t_alg; struct aead_alg *alg; - t_alg = kzalloc(sizeof(struct ssi_crypto_alg), GFP_KERNEL); + t_alg = kzalloc(sizeof(*t_alg), GFP_KERNEL); if (!t_alg) { SSI_LOG_ERR("failed to allocate t_alg\n"); return ERR_PTR(-ENOMEM); @@ -2713,7 +2713,7 @@ int ssi_aead_alloc(struct ssi_drvdata *drvdata) int rc = -ENOMEM; int alg; - aead_handle = kmalloc(sizeof(struct ssi_aead_handle), GFP_KERNEL); + aead_handle = kmalloc(sizeof(*aead_handle), GFP_KERNEL); if (!aead_handle) { rc = -ENOMEM; goto fail0; diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 068b10b..d98178d 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -1215,7 +1215,7 @@ struct ssi_crypto_alg *ssi_ablkcipher_create_alg(struct ssi_alg_template *templa struct ssi_crypto_alg *t_alg; struct crypto_alg *alg; - t_alg = kzalloc(sizeof(struct ssi_crypto_alg), GFP_KERNEL); + t_alg = kzalloc(sizeof(*t_alg), GFP_KERNEL); if (!t_alg) { SSI_LOG_ERR("failed to allocate t_alg\n"); return ERR_PTR(-ENOMEM); @@ -1276,7 +1276,7 @@ int ssi_ablkcipher_alloc(struct ssi_drvdata *drvdata) int rc = -ENOMEM; int alg; - ablkcipher_handle = kmalloc(sizeof(struct ssi_blkcipher_handle), + ablkcipher_handle = kmalloc(sizeof(*ablkcipher_handle), GFP_KERNEL); if (!ablkcipher_handle) return -ENOMEM; diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index d104dbd..1cae2b7 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -223,7 +223,7 @@ static int init_cc_resources(struct platform_device *plat_dev) struct resource *req_mem_cc_regs = NULL; void __iomem *cc_base = NULL; bool irq_registered = false; - struct ssi_drvdata *new_drvdata = kzalloc(sizeof(struct ssi_drvdata), GFP_KERNEL); + struct ssi_drvdata *new_drvdata = kzalloc(sizeof(*new_drvdata), GFP_KERNEL); struct device *dev = _dev->dev; struct device_node *np = dev->of_node; u32 signature_val; diff --git a/drivers/staging/ccree/ssi_hash.c b/drivers/staging/ccree/ssi_hash.c index 3a734df..6c08b1d 100644 --- a/drivers/staging/ccree/ssi_hash.c +++ b/drivers/staging/ccree/ssi_hash.c @@ -2055,7 +2055,7 @@ ssi_hash_create_alg(struct ssi_hash_template *template, bool keyed) struct crypto_alg *alg; struct ahash_alg *halg; - t_crypto_alg = kzalloc(sizeof(struct ssi_hash_alg), GFP_KERNEL); + t_crypto_alg = kzalloc(sizeof(*t_crypto_alg), GFP_KERNEL); if (!t_crypto_alg) { SSI_LOG_ERR("failed to allocate t_alg\n"); return ERR_PTR(-ENOMEM); @@ -2221,7 +2221,7 @@ int ssi_hash_alloc(struct ssi_drvdata *drvdata) int rc = 0; int alg; - hash_handle = kzalloc(sizeof(struct ssi_hash_handle), GFP_KERNEL); + hash_handle = kzalloc(sizeof(*hash_handle), GFP_KERNEL); if (!hash_handle) { SSI_LOG_ERR("kzalloc failed to allocate %zu B\n", sizeof(struct ssi_hash_handle)); diff --git a/drivers/staging/ccree/ssi_ivgen.c b/drivers/staging/ccree/ssi_ivgen.c index bca44af..93a2a94 100644 --- a/drivers/staging/ccree/ssi_ivgen.c +++ b/drivers/staging/ccree/ssi_ivgen.c @@ -191,7 +191,7 @@ int ssi_ivgen_init(struct ssi_drvdata *drvdata) int rc; /* Allocate "this" context */ - drvdata->ivgen_handle = kzalloc(sizeof(struct ssi_ivgen_ctx), GFP_KERNEL); + drvdata->ivgen_handle = kzalloc(sizeof(*drvdata->ivgen_handle), GFP_KERNEL); if (!drvdata->ivgen_handle) { SSI_LOG_ERR("Not enough memory to allocate IVGEN context (%zu B)\n", sizeof(struct ssi_ivgen_ctx)); diff --git a/drivers/staging/ccree/ssi_request_mgr.c b/drivers/staging/ccree/ssi_request_mgr.c index 9a4bb5c..cae9
[PATCH v3 04/22] staging: ccree: Convert to devm_ioremap_resource for map, unmap
From: Suniel Mahesh <suni...@techveda.org> It is recommended to use managed function devm_ioremap_resource(), which simplifies driver cleanup paths and driver code. This patch does the following: (a) replace request_mem_region(), ioremap() and corresponding error handling with devm_ioremap_resource(). (b) remove struct resource pointer(res_mem) in struct ssi_drvdata as it seems redundant, use struct resource pointer which is defined locally and adjust return value of platform_get_resource() accordingly. (c) release_mem_region() and iounmap() are dropped, since devm_ioremap_ resource() releases and unmaps mem region on driver detach. (d) adjust log messages accordingly and remove any blank lines. Signed-off-by: Suniel Mahesh <suni...@techveda.org> [gby: rebase on top of latest coding style fixes changes] Acked-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_driver.c | 60 ++ drivers/staging/ccree/ssi_driver.h | 1 - 2 files changed, 15 insertions(+), 46 deletions(-) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 97dfc2c..603eb03 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -246,35 +246,21 @@ static int init_cc_resources(struct platform_device *plat_dev) dev_set_drvdata(_dev->dev, new_drvdata); /* Get device resources */ /* First CC registers space */ - new_drvdata->res_mem = platform_get_resource(plat_dev, IORESOURCE_MEM, 0); - if (unlikely(!new_drvdata->res_mem)) { - SSI_LOG_ERR("Failed getting IO memory resource\n"); - rc = -ENODEV; - goto init_cc_res_err; - } - SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n", - new_drvdata->res_mem->name, - new_drvdata->res_mem->start, - new_drvdata->res_mem->end); + req_mem_cc_regs = platform_get_resource(plat_dev, IORESOURCE_MEM, 0); /* Map registers space */ - req_mem_cc_regs = request_mem_region(new_drvdata->res_mem->start, resource_size(new_drvdata->res_mem), "arm_cc7x_regs"); - if (unlikely(!req_mem_cc_regs)) { - SSI_LOG_ERR("Couldn't allocate registers memory region at 0x%08X\n", - (unsigned int)new_drvdata->res_mem->start); - rc = -EBUSY; - goto init_cc_res_err; - } - cc_base = ioremap(new_drvdata->res_mem->start, resource_size(new_drvdata->res_mem)); - if (unlikely(!cc_base)) { - SSI_LOG_ERR("ioremap[CC](0x%08X,0x%08X) failed\n", - (unsigned int)new_drvdata->res_mem->start, - (unsigned int)resource_size(new_drvdata->res_mem)); - rc = -ENOMEM; + new_drvdata->cc_base = devm_ioremap_resource(_dev->dev, +req_mem_cc_regs); + if (IS_ERR(new_drvdata->cc_base)) { + rc = PTR_ERR(new_drvdata->cc_base); goto init_cc_res_err; } - SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n", _drvdata->res_mem->start, cc_base); - new_drvdata->cc_base = cc_base; - + SSI_LOG_DEBUG("Got MEM resource (%s): start=%pad end=%pad\n", + req_mem_cc_regs->name, + req_mem_cc_regs->start, + req_mem_cc_regs->end); + SSI_LOG_DEBUG("CC registers mapped from %pa to 0x%p\n", + _mem_cc_regs->start, new_drvdata->cc_base); + cc_base = new_drvdata->cc_base; /* Then IRQ */ new_drvdata->res_irq = platform_get_resource(plat_dev, IORESOURCE_IRQ, 0); if (unlikely(!new_drvdata->res_irq)) { @@ -424,17 +410,9 @@ static int init_cc_resources(struct platform_device *plat_dev) #ifdef ENABLE_CC_SYSFS ssi_sysfs_fini(); #endif - - if (req_mem_cc_regs) { - if (irq_registered) { - free_irq(new_drvdata->res_irq->start, new_drvdata); - new_drvdata->res_irq = NULL; - iounmap(cc_base); - new_drvdata->cc_base = NULL; - } - release_mem_region(new_drvdata->res_mem->start, - resource_size(new_drvdata->res_mem)); - new_drvdata->res_mem = NULL; + if (irq_registered) { + free_irq(new_drvdata->res_irq->start, new_drvdata); + new_drvdata->res_irq = NULL; } dev_set_drvdata(_dev->dev, NU
[PATCH v3 16/22] staging: ccree: fix spelling mistakes
Fix various spelling mistakes in comments. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_cipher.c | 2 +- drivers/staging/ccree/ssi_hash.c| 2 +- drivers/staging/ccree/ssi_hash.h| 2 +- drivers/staging/ccree/ssi_ivgen.c | 2 +- drivers/staging/ccree/ssi_request_mgr.c | 2 +- drivers/staging/ccree/ssi_request_mgr.h | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/drivers/staging/ccree/ssi_cipher.c b/drivers/staging/ccree/ssi_cipher.c index 8d65e97..e417bfd 100644 --- a/drivers/staging/ccree/ssi_cipher.c +++ b/drivers/staging/ccree/ssi_cipher.c @@ -697,7 +697,7 @@ static int ssi_blkcipher_complete(struct device *dev, ssi_buffer_mgr_unmap_blkcipher_request(dev, req_ctx, ivsize, src, dst); - /*Set the inflight couter value to local variable*/ + /*Set the inflight counter value to local variable*/ inflight_counter = ctx_p->drvdata->inflight_counter; /*Decrease the inflight counter*/ if (ctx_p->flow_mode == BYPASS && ctx_p->drvdata->inflight_counter > 0) diff --git a/drivers/staging/ccree/ssi_hash.c b/drivers/staging/ccree/ssi_hash.c index 6baa449..cfd5f5c 100644 --- a/drivers/staging/ccree/ssi_hash.c +++ b/drivers/staging/ccree/ssi_hash.c @@ -2575,7 +2575,7 @@ static void ssi_hash_create_data_desc(struct ahash_req_ctx *areq_ctx, * \param drvdata * \param mode The Hash mode. Supported modes: MD5/SHA1/SHA224/SHA256 * - * \return u32 The address of the inital digest in SRAM + * \return u32 The address of the initial digest in SRAM */ ssi_sram_addr_t ssi_ahash_get_larval_digest_sram_addr(void *drvdata, u32 mode) { diff --git a/drivers/staging/ccree/ssi_hash.h b/drivers/staging/ccree/ssi_hash.h index 2400e38..c884727 100644 --- a/drivers/staging/ccree/ssi_hash.h +++ b/drivers/staging/ccree/ssi_hash.h @@ -95,7 +95,7 @@ ssi_ahash_get_initial_digest_len_sram_addr(void *drvdata, u32 mode); * \param drvdata * \param mode The Hash mode. Supported modes: MD5/SHA1/SHA224/SHA256/SHA384/SHA512 * - * \return u32 The address of the inital digest in SRAM + * \return u32 The address of the initial digest in SRAM */ ssi_sram_addr_t ssi_ahash_get_larval_digest_sram_addr(void *drvdata, u32 mode); diff --git a/drivers/staging/ccree/ssi_ivgen.c b/drivers/staging/ccree/ssi_ivgen.c index 93a2a94..ba70237 100644 --- a/drivers/staging/ccree/ssi_ivgen.c +++ b/drivers/staging/ccree/ssi_ivgen.c @@ -200,7 +200,7 @@ int ssi_ivgen_init(struct ssi_drvdata *drvdata) } ivgen_ctx = drvdata->ivgen_handle; - /* Allocate pool's header for intial enc. key/IV */ + /* Allocate pool's header for initial enc. key/IV */ ivgen_ctx->pool_meta = dma_alloc_coherent(device, SSI_IVPOOL_META_SIZE, _ctx->pool_meta_dma, GFP_KERNEL); diff --git a/drivers/staging/ccree/ssi_request_mgr.c b/drivers/staging/ccree/ssi_request_mgr.c index 27324bb..9ca2536 100644 --- a/drivers/staging/ccree/ssi_request_mgr.c +++ b/drivers/staging/ccree/ssi_request_mgr.c @@ -205,7 +205,7 @@ static inline int request_mgr_queues_status_check( unsigned long poll_queue; /* SW queue is checked only once as it will not -* be chaned during the poll becasue the spinlock_bh +* be chaned during the poll because the spinlock_bh * is held by the thread */ if (unlikely(((req_mgr_h->req_queue_head + 1) & diff --git a/drivers/staging/ccree/ssi_request_mgr.h b/drivers/staging/ccree/ssi_request_mgr.h index bdbbf89..b248fb6 100644 --- a/drivers/staging/ccree/ssi_request_mgr.h +++ b/drivers/staging/ccree/ssi_request_mgr.h @@ -36,7 +36,7 @@ int request_mgr_init(struct ssi_drvdata *drvdata); * If "false": this function adds a dummy descriptor completion * and waits upon completion signal. * - * \return int Returns -EINPROGRESS if "is_dout=ture"; "0" if "is_dout=false" + * \return int Returns -EINPROGRESS if "is_dout=true"; "0" if "is_dout=false" */ int send_request( struct ssi_drvdata *drvdata, struct ssi_crypto_req *ssi_req, -- 2.1.4
[PATCH v3 12/22] staging: ccree: align box comment correctly
Fix indentation in first comment. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_hash.c | 28 ++-- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/staging/ccree/ssi_hash.c b/drivers/staging/ccree/ssi_hash.c index b7d6586..b95c3ce 100644 --- a/drivers/staging/ccree/ssi_hash.c +++ b/drivers/staging/ccree/ssi_hash.c @@ -1,18 +1,18 @@ /* - * Copyright (C) 2012-2017 ARM Limited or its affiliates. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License version 2 as - * published by the Free Software Foundation. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program; if not, see <http://www.gnu.org/licenses/>. - */ + * Copyright (C) 2012-2017 ARM Limited or its affiliates. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ #include #include -- 2.1.4
[PATCH v3 01/22] staging: ccree: fix split strings
Fix strings in log messages being split across lines and the resulting alignment issues when being fixed. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_aead.c| 9 ++-- drivers/staging/ccree/ssi_buffer_mgr.c | 86 ++--- drivers/staging/ccree/ssi_cipher.c | 27 +-- drivers/staging/ccree/ssi_driver.c | 4 +- drivers/staging/ccree/ssi_hash.c| 43 - drivers/staging/ccree/ssi_ivgen.c | 8 +-- drivers/staging/ccree/ssi_request_mgr.c | 13 ++--- 7 files changed, 81 insertions(+), 109 deletions(-) diff --git a/drivers/staging/ccree/ssi_aead.c b/drivers/staging/ccree/ssi_aead.c index f5ca0e3..d8f2249 100644 --- a/drivers/staging/ccree/ssi_aead.c +++ b/drivers/staging/ccree/ssi_aead.c @@ -240,9 +240,8 @@ static void ssi_aead_complete(struct device *dev, void *ssi_req, void __iomem *c if (areq_ctx->gen_ctx.op_type == DRV_CRYPTO_DIRECTION_DECRYPT) { if (memcmp(areq_ctx->mac_buf, areq_ctx->icv_virt_addr, ctx->authsize) != 0) { - SSI_LOG_DEBUG("Payload authentication failure, " - "(auth-size=%d, cipher=%d).\n", - ctx->authsize, ctx->cipher_mode); + SSI_LOG_DEBUG("Payload authentication failure, (auth-size=%d, cipher=%d).\n", + ctx->authsize, ctx->cipher_mode); /* In case of payload authentication failure, MUST NOT * revealed the decrypted message --> zero its memory. */ @@ -455,8 +454,8 @@ ssi_get_plain_hmac_key(struct crypto_aead *tfm, const u8 *key, unsigned int keyl if (likely(keylen != 0)) { key_dma_addr = dma_map_single(dev, (void *)key, keylen, DMA_TO_DEVICE); if (unlikely(dma_mapping_error(dev, key_dma_addr))) { - SSI_LOG_ERR("Mapping key va=0x%p len=%u for" - " DMA failed\n", key, keylen); + SSI_LOG_ERR("Mapping key va=0x%p len=%u for DMA failed\n", + key, keylen); return -ENOMEM; } if (keylen > blocksize) { diff --git a/drivers/staging/ccree/ssi_buffer_mgr.c b/drivers/staging/ccree/ssi_buffer_mgr.c index 6393609..d7ce293 100644 --- a/drivers/staging/ccree/ssi_buffer_mgr.c +++ b/drivers/staging/ccree/ssi_buffer_mgr.c @@ -82,8 +82,8 @@ static unsigned int ssi_buffer_mgr_get_sgl_nents( while (nbytes != 0) { if (sg_is_chain(sg_list)) { - SSI_LOG_ERR("Unexpected chained entry " - "in sg (entry =0x%X)\n", nents); + SSI_LOG_ERR("Unexpected chained entry in sg (entry =0x%X)\n", + nents); BUG(); } if (sg_list->length != 0) { @@ -259,11 +259,9 @@ static int ssi_buffer_mgr_generate_mlli( /* Set MLLI size for the bypass operation */ mlli_params->mlli_len = (total_nents * LLI_ENTRY_BYTE_SIZE); - SSI_LOG_DEBUG("MLLI params: " -"virt_addr=%pK dma_addr=%pad mlli_len=0x%X\n", - mlli_params->mlli_virt_addr, - mlli_params->mlli_dma_addr, - mlli_params->mlli_len); + SSI_LOG_DEBUG("MLLI params: virt_addr=%pK dma_addr=%pad mlli_len=0x%X\n", + mlli_params->mlli_virt_addr, mlli_params->mlli_dma_addr, + mlli_params->mlli_len); build_mlli_exit: return rc; @@ -276,9 +274,8 @@ static inline void ssi_buffer_mgr_add_buffer_entry( { unsigned int index = sgl_data->num_of_buffers; - SSI_LOG_DEBUG("index=%u single_buff=%pad " -"buffer_len=0x%08X is_last=%d\n", -index, buffer_dma, buffer_len, is_last_entry); + SSI_LOG_DEBUG("index=%u single_buff=%pad buffer_len=0x%08X is_last=%d\n", + index, buffer_dma, buffer_len, is_last_entry); sgl_data->nents[index] = 1; sgl_data->entry[index].buffer_dma = buffer_dma; sgl_data->offset[index] = 0; @@ -359,8 +356,7 @@ static int ssi_buffer_mgr_map_scatterlist( SSI_LOG_ERR("dma_map_sg() single buffer failed\n"); return -ENOMEM; } - SSI_LOG_DEBUG("Mapped sg: dma_address=%pad " -"page=%p addr=%pK offset=%u " + SSI_LOG_DEBUG("Mapped sg: dma_address=%pad page=%p addr=%pK offset=%u "
[PATCH v3 07/22] staging: ccree: remove unused completion
icache_setup_completion is no longer used. Remove it. Signed-off-by: Gilad Ben-Yossef <gi...@benyossef.com> --- drivers/staging/ccree/ssi_driver.c | 2 -- drivers/staging/ccree/ssi_driver.h | 1 - 2 files changed, 3 deletions(-) diff --git a/drivers/staging/ccree/ssi_driver.c b/drivers/staging/ccree/ssi_driver.c index 1b95f90..928c988 100644 --- a/drivers/staging/ccree/ssi_driver.c +++ b/drivers/staging/ccree/ssi_driver.c @@ -279,8 +279,6 @@ static int init_cc_resources(struct platform_device *plat_dev) } SSI_LOG_DEBUG("Registered to IRQ: %d\n", new_drvdata->irq); - init_completion(_drvdata->icache_setup_completion); - rc = cc_clk_on(new_drvdata); if (rc) goto post_drvdata_err; diff --git a/drivers/staging/ccree/ssi_driver.h b/drivers/staging/ccree/ssi_driver.h index 88ef370..9b6476d 100644 --- a/drivers/staging/ccree/ssi_driver.h +++ b/drivers/staging/ccree/ssi_driver.h @@ -138,7 +138,6 @@ struct ssi_drvdata { u32 monitor_null_cycles; struct platform_device *plat_dev; ssi_sram_addr_t mlli_sram_addr; - struct completion icache_setup_completion; void *buff_mgr_handle; void *hash_handle; void *aead_handle; -- 2.1.4