| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sat, 19 Nov 2016 01:01:03 +0530
From: George Cherian <gcherian@...iumnetworks.com>
To: David Daney <ddaney@...iumnetworks.com>, <gcherianv@...il.com>
CC: <linux-kernel@...r.kernel.org>, <linux-crypto@...r.kernel.org>,
<davem@...emloft.net>, <herbert@...dor.apana.org.au>,
George Cherian <george.cherian@...ium.com>
Subject: Re: [PATCH 1/3] drivers: crypto: Add Support for Octeon-tx CPT Engine
Hi David,
Thanks for the review.
On Saturday 19 November 2016 12:25 AM, David Daney wrote:
> On 11/18/2016 07:00 AM, gcherianv@...il.com wrote:
>> From: George Cherian <george.cherian@...ium.com>
>>
>> Enable the Physical Function diver for the Cavium Crypto Engine (CPT)
>> found in Octeon-tx series of SoC's. CPT is the Cryptographic
>> Acceleration
>> Unit. CPT includes microcoded GigaCypher symmetric engines (SEs) and
>> asymmetric engines (AEs).
>>
>> Signed-off-by: George Cherian <george.cherian@...ium.com>
>
>
> How was this tested?
Using ecryptfs and dm-crypt.
>
>
>> ---
>> drivers/crypto/cavium/cpt/Kconfig | 22 +
>> drivers/crypto/cavium/cpt/Makefile | 2 +
>> drivers/crypto/cavium/cpt/cpt.h | 90 +++
>> drivers/crypto/cavium/cpt/cpt_common.h | 377 +++++++++++++
>> drivers/crypto/cavium/cpt/cpt_hw_types.h | 940
>> +++++++++++++++++++++++++++++++
>> drivers/crypto/cavium/cpt/cpt_main.c | 891
>> +++++++++++++++++++++++++++++
>> drivers/crypto/cavium/cpt/cpt_pf_mbox.c | 174 ++++++
>> 7 files changed, 2496 insertions(+)
>> create mode 100644 drivers/crypto/cavium/cpt/Kconfig
>> create mode 100644 drivers/crypto/cavium/cpt/Makefile
>> create mode 100644 drivers/crypto/cavium/cpt/cpt.h
>> create mode 100644 drivers/crypto/cavium/cpt/cpt_common.h
>> create mode 100644 drivers/crypto/cavium/cpt/cpt_hw_types.h
>> create mode 100644 drivers/crypto/cavium/cpt/cpt_main.c
>> create mode 100644 drivers/crypto/cavium/cpt/cpt_pf_mbox.c
>>
>> diff --git a/drivers/crypto/cavium/cpt/Kconfig
>> b/drivers/crypto/cavium/cpt/Kconfig
>> new file mode 100644
>> index 0000000..8fe3f44
>> --- /dev/null
>> +++ b/drivers/crypto/cavium/cpt/Kconfig
>> @@ -0,0 +1,22 @@
>> +#
>> +# Cavium crypto device configuration
>> +#
>> +
>> +config CRYPTO_DEV_CPT
>> + tristate
>> + select HW_RANDOM_OCTEON
>
> This makes no sense. HW_RANDOM_OCTEON is for a mips64 based SOC and
> isn't present on devices that have this crypto block. Why select this?
>
Yeah true... I actually wanted to this one instead
|CONFIG_HW_RANDOM_CAVIUM|
>
>> + select CRYPTO_AES
>> + select CRYPTO_DES
>> + select CRYPTO_BLKCIPHER
>> + select FW_LOADER
>> +
>> +config OCTEONTX_CPT_PF
>> + tristate "Octeon-tx CPT Physical function driver"
>> + depends on ARCH_THUNDER
>> + select CRYPTO_DEV_CPT
>> + help
>> + Support for Cavium CPT block found in octeon-tx series of
>> + processors.
>> +
>> + To compile this as a module, choose M here: the module will be
>> + called cptpf.
>> diff --git a/drivers/crypto/cavium/cpt/Makefile
>> b/drivers/crypto/cavium/cpt/Makefile
>> new file mode 100644
>> index 0000000..bf758e2
>> --- /dev/null
>> +++ b/drivers/crypto/cavium/cpt/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_OCTEONTX_CPT_PF) += cptpf.o
>> +cptpf-objs := cpt_main.o cpt_pf_mbox.o
>> diff --git a/drivers/crypto/cavium/cpt/cpt.h
>> b/drivers/crypto/cavium/cpt/cpt.h
>> new file mode 100644
>> index 0000000..63d12da
>> --- /dev/null
>> +++ b/drivers/crypto/cavium/cpt/cpt.h
>> @@ -0,0 +1,90 @@
>> +/*
>> + * Copyright (C) 2016 Cavium, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify it
>> + * under the terms of version 2 of the GNU General Public License
>> + * as published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __CPT_H
>> +#define __CPT_H
>> +
>> +#include "cpt_common.h"
>> +
>> +#define BASE_PROC_DIR "cavium"
>> +
>> +#define PF 0
>> +#define VF 1
>> +
>> +struct cpt_device;
>> +
>> +struct microcode {
>> + uint8_t is_mc_valid;
>
> s/uint8_t/u8/ ??
>
> That could be done everywhere.
will do
>
> [...]
>> diff --git a/drivers/crypto/cavium/cpt/cpt_common.h
>> b/drivers/crypto/cavium/cpt/cpt_common.h
>> new file mode 100644
>> index 0000000..351ed4a
>> --- /dev/null
>> +++ b/drivers/crypto/cavium/cpt/cpt_common.h
>> @@ -0,0 +1,377 @@
>> +/*
>> + * Copyright (C) 2016 Cavium, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> modify it
>> + * under the terms of version 2 of the GNU General Public License
>> + * as published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef __CPT_COMMON_H
>> +#define __CPT_COMMON_H
>> +
>> +#include <asm/byteorder.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/types.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/pci.h>
>> +#include <linux/cpumask.h>
>> +#include <linux/string.h>
>> +#include <linux/pci_regs.h>
>> +#include <linux/delay.h>
>> +#include <linux/printk.h>
>> +#include <linux/sched.h>
>> +#include <linux/completion.h>
>> +#include <asm/arch_timer.h>
>> +#include <linux/types.h>
>> +
>> +#include "cpt_hw_types.h"
>> +
>> +/* configuration space offsets */
>> +#ifndef PCI_VENDOR_ID
>> +#define PCI_VENDOR_ID 0x00 /* 16 bits */
>> +#endif
>> +#ifndef PCI_DEVICE_ID
>> +#define PCI_DEVICE_ID 0x02 /* 16 bits */
>> +#endif
>> +#ifndef PCI_REVISION_ID
>> +#define PCI_REVISION_ID 0x08 /* Revision ID */
>> +#endif
>> +#ifndef PCI_CAPABILITY_LIST
>> +#define PCI_CAPABILITY_LIST 0x34 /* first capability list entry */
>> +#endif
>> +
>
> Standard PCI core functions give you access to all that information,
> use pdev->device, pdev->revision, etc. instead of reinventing the
> wheel here with all these #defines.
>
>
>> +/* Device ID */
>> +#define PCI_VENDOR_ID_CAVIUM 0x177d
>
> This is defined in pci_ids.h, use value from there instead of placing
> a duplicate definition here.
>
okay will remove them
>> +#define CPT_81XX_PCI_PF_DEVICE_ID 0xa040
>> +#define CPT_81XX_PCI_VF_DEVICE_ID 0xa041
>> +
>> +#define PASS_1_0 0x0
>> +
>> +/* CPT Models ((Device ID<<16)|Revision ID) */
>> +/* CPT models */
>> +#define CPT_81XX_PASS1_0 ((CPT_81XX_PCI_PF_DEVICE_ID << 8) | PASS_1_0)
>> +#define CPTVF_81XX_PASS1_0 ((CPT_81XX_PCI_VF_DEVICE_ID << 8) |
>> PASS_1_0)
>> +
>> +#define PF 0
>> +#define VF 1
>> +
>> +#define DEFAULT_DEVICE_QUEUES CPT_NUM_QS_PER_VF
>> +
>> +#define SUCCESS (0)
>> +#define FAIL (1)
>> +
>> +#ifndef ROUNDUP4
>> +#define ROUNDUP4(val) (((val) + 3) & 0xfffffffc)
>> +#endif
>> +
>> +#ifndef ROUNDUP8
>> +#define ROUNDUP8(val) (((val) + 7) & 0xfffffff8)
>> +#endif
>> +
>> +#ifndef ROUNDUP16
>> +#define ROUNDUP16(val) (((val) + 15) & 0xfffffff0)
>> +#endif
>> +
>
> kernel.h has round_up(), use that instead of defining all these.
>
>> +#define ERR_ADDR_LEN 8
>> +
>
> What is that for? It looks unused.
>
> [...]
>> +/*###### PCIE EP-Mode Configuration Registers #########*/
>> +#define PCIEEP0_CFG000 (0x0)
>> +#define PCIEEP0_CFG002 (0x8)
>> +#define PCIEEP0_CFG011 (0x2C)
>> +#define PCIEEP0_CFG020 (0x50)
>> +#define PCIEEP0_CFG025 (0x64)
>> +#define PCIEEP0_CFG030 (0x78)
>> +#define PCIEEP0_CFG044 (0xB0)
>> +#define PCIEEP0_CFG045 (0xB4)
>> +#define PCIEEP0_CFG082 (0x148)
>> +#define PCIEEP0_CFG095 (0x17C)
>> +#define PCIEEP0_CFG096 (0x180)
>> +#define PCIEEP0_CFG097 (0x184)
>> +#define PCIEEP0_CFG103 (0x19C)
>> +#define PCIEEP0_CFG460 (0x730)
>> +#define PCIEEP0_CFG461 (0x734)
>> +#define PCIEEP0_CFG462 (0x738)
>> +
>> +/*####### PCIe EP-Mode SR-IOV Configuration Registers #####*/
>> +#define PCIEEPVF0_CFG000 (0x0)
>> +#define PCIEEPVF0_CFG002 (0x8)
>> +#define PCIEEPVF0_CFG011 (0x2C)
>> +#define PCIEEPVF0_CFG030 (0x78)
>> +#define PCIEEPVF0_CFG044 (0xB0)
>> +
>
> Where are all those defines used? What are they for?
>
>
> That's all I can look at for now.
>
I will address your comments in next version.
> David.
>
Powered by blists - more mailing lists