| 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
| ||
|
Message-ID: <39b3abc52848f00d8952f703ae60988e@walle.cc>
Date: Fri, 12 Nov 2021 20:22:33 +0100
From: Michael Walle <michael@...le.cc>
To: Andrey Zhizhikin <andrey.zhizhikin@...ca-geosystems.com>
Cc: horia.geanta@....com, pankaj.gupta@....com,
herbert@...dor.apana.org.au, davem@...emloft.net,
iuliana.prodan@....com, linux-crypto@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] crypto: caam - convert to use capabilities
Am 2021-11-11 17:46, schrieb Andrey Zhizhikin:
> CAAM driver contains several variables, which are used for indication
> that ertail capabilities are detected during initial probing of the
> device. They are defined as u8, but mainly used as boolean variables to
> identify capabillities.
>
> Clean-up all assorted variables, collect them into one bitfield value
> which encodes capabilities as bit, and use them in the execution flow
> instead.
>
> Signed-off-by: Andrey Zhizhikin <andrey.zhizhikin@...ca-geosystems.com>
> ---
> Changes in V2: No change, this patch is newly introduced
>
> drivers/crypto/caam/caamalg_qi.c | 2 +-
> drivers/crypto/caam/ctrl.c | 49 ++++++++++++++++++--------------
> drivers/crypto/caam/intern.h | 16 +++++------
> drivers/crypto/caam/regs.h | 2 --
> 4 files changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/crypto/caam/caamalg_qi.c
> b/drivers/crypto/caam/caamalg_qi.c
> index 189a7438b29c..372a319e8434 100644
> --- a/drivers/crypto/caam/caamalg_qi.c
> +++ b/drivers/crypto/caam/caamalg_qi.c
> @@ -2610,7 +2610,7 @@ int caam_qi_algapi_init(struct device *ctrldev)
> bool registered = false;
>
> /* Make sure this runs only on (DPAA 1.x) QI */
> - if (!priv->qi_present || caam_dpaa2)
> + if (!(priv->caam_caps | CAAM_CAPS_QI_PRESENT) || caam_dpaa2)
Typo?
if (!(priv->caam_caps & CAAM_CAPS_QI_PRESENT) || caam_dpaa2)
> return 0;
>
> /*
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index ca0361b2dbb0..7a14a69d89c7 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -100,7 +100,7 @@ static inline int run_descriptor_deco0(struct
> device *ctrldev, u32 *desc,
> int i;
>
>
> - if (ctrlpriv->virt_en == 1 ||
> + if ((ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED) ||
> /*
> * Apparently on i.MX8M{Q,M,N,P} it doesn't matter if virt_en ==
> 1
> * and the following steps should be performed regardless
> @@ -169,7 +169,7 @@ static inline int run_descriptor_deco0(struct
> device *ctrldev, u32 *desc,
> *status = rd_reg32(&deco->op_status_hi) &
> DECO_OP_STATUS_HI_ERR_MASK;
>
> - if (ctrlpriv->virt_en == 1)
> + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED)
> clrsetbits_32(&ctrl->deco_rsr, DECORSR_JR0, 0);
>
> /* Mark the DECO as free */
> @@ -622,7 +622,6 @@ static int caam_probe(struct platform_device *pdev)
> struct dentry *dfs_root;
> u32 scfgr, comp_params;
> u8 rng_vid;
> - int pg_size;
> int BLOCK_OFFSET = 0;
> bool pr_support = false;
>
> @@ -666,11 +665,12 @@ static int caam_probe(struct platform_device
> *pdev)
> else
> caam_ptr_sz = sizeof(u32);
> caam_dpaa2 = !!(comp_params & CTPR_MS_DPAA2);
> - ctrlpriv->qi_present = !!(comp_params & CTPR_MS_QI_MASK);
> + ctrlpriv->caam_caps |= (!!(comp_params & CTPR_MS_QI_MASK)) ?
> + CAAM_CAPS_QI_PRESENT : 0;
"!!" is superfluous here. and the braces, too. But IMHO this is
easier on the eye:
if (comp_params & CTPR_MS_QI_MASK)
ctrl->priv->caam_cap |= CAAM_CAPS_QI_PRESENT;
> #ifdef CONFIG_CAAM_QI
> /* If (DPAA 1.x) QI present, check whether dependencies are available
> */
> - if (ctrlpriv->qi_present && !caam_dpaa2) {
> + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) {
> ret = qman_is_probed();
> if (!ret) {
> return -EPROBE_DEFER;
> @@ -692,11 +692,14 @@ static int caam_probe(struct platform_device
> *pdev)
> /* Allocating the BLOCK_OFFSET based on the supported page size on
> * the platform
> */
> - pg_size = (comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT;
> - if (pg_size == 0)
> - BLOCK_OFFSET = PG_SIZE_4K;
> + ctrlpriv->caam_caps |=
> + (!!((comp_params & CTPR_MS_PG_SZ_MASK) >> CTPR_MS_PG_SZ_SHIFT)) ?
> + CAAM_CAPS_64K_PAGESIZE : 0;
same here as above.
if (comp_params & CTPR_MS_PG_SZ_MASK)
ctrl->priv->caam_cap |= CAAM_CAPS_64K_PAGESIZE;
Assuming that SZ_MASK and SZ_SHIFT fits together ;)
> +
> + if (ctrlpriv->caam_caps & CAAM_CAPS_64K_PAGESIZE)
> + BLOCK_OFFSET = SZ_64K;
> else
> - BLOCK_OFFSET = PG_SIZE_64K;
> + BLOCK_OFFSET = SZ_4K;
btw.. that all uppercase BLOCK_OFFSET looks super odd. Can we get
rid of that too? I haven't checked but pg_size didn't make any
sense before, did it? At least if SZ_MASK has more than one bit.
> ctrlpriv->ctrl = (struct caam_ctrl __iomem __force *)ctrl;
> ctrlpriv->assure = (struct caam_assurance __iomem __force *)
> @@ -711,11 +714,11 @@ static int caam_probe(struct platform_device
> *pdev)
> /* Get the IRQ of the controller (for security violations only) */
> ctrlpriv->secvio_irq = irq_of_parse_and_map(nprop, 0);
> np = of_find_compatible_node(NULL, NULL, "fsl,qoriq-mc");
> - ctrlpriv->mc_en = !!np;
> + ctrlpriv->caam_caps |= (!!np) ? CAAM_CAPS_MC_ENABLED : 0;
if (np)
ctrlpriv->caam_caps |= CAAM_CAPS_MC_ENABLED;
> of_node_put(np);
>
> #ifdef CONFIG_FSL_MC_BUS
> - if (ctrlpriv->mc_en) {
> + if (ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) {
> struct fsl_mc_version *mc_version;
>
> mc_version = fsl_mc_get_version();
> @@ -732,7 +735,7 @@ static int caam_probe(struct platform_device *pdev)
> * In case of SoCs with Management Complex, MC f/w performs
> * the configuration.
> */
> - if (!ctrlpriv->mc_en)
> + if (!(ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED))
> clrsetbits_32(&ctrl->mcr, MCFGR_AWCACHE_MASK,
> MCFGR_AWCACHE_CACH | MCFGR_AWCACHE_BUFF |
> MCFGR_WDENABLE | MCFGR_LARGE_BURST);
> @@ -745,7 +748,6 @@ static int caam_probe(struct platform_device *pdev)
> */
> scfgr = rd_reg32(&ctrl->scfgr);
>
> - ctrlpriv->virt_en = 0;
> if (comp_params & CTPR_MS_VIRT_EN_INCL) {
> /* VIRT_EN_INCL = 1 & VIRT_EN_POR = 1 or
> * VIRT_EN_INCL = 1 & VIRT_EN_POR = 0 & SCFGR_VIRT_EN = 1
> @@ -753,14 +755,14 @@ static int caam_probe(struct platform_device
> *pdev)
> if ((comp_params & CTPR_MS_VIRT_EN_POR) ||
> (!(comp_params & CTPR_MS_VIRT_EN_POR) &&
> (scfgr & SCFGR_VIRT_EN)))
> - ctrlpriv->virt_en = 1;
> + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED;
at first sight it looked like a wrong indendation, but it the old
code was wrong.
> } else {
> /* VIRT_EN_INCL = 0 && VIRT_EN_POR_VALUE = 1 */
> if (comp_params & CTPR_MS_VIRT_EN_POR)
> - ctrlpriv->virt_en = 1;
> + ctrlpriv->caam_caps |= CAAM_CAPS_VIRT_ENABLED;
> }
>
> - if (ctrlpriv->virt_en == 1)
> + if (ctrlpriv->caam_caps & CAAM_CAPS_VIRT_ENABLED)
> clrsetbits_32(&ctrl->jrstart, 0, JRSTART_JR0_START |
> JRSTART_JR1_START | JRSTART_JR2_START |
> JRSTART_JR3_START);
> @@ -785,7 +787,7 @@ static int caam_probe(struct platform_device *pdev)
> caam_debugfs_init(ctrlpriv, dfs_root);
>
> /* Check to see if (DPAA 1.x) QI present. If so, enable */
> - if (ctrlpriv->qi_present && !caam_dpaa2) {
> + if ((ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) && !caam_dpaa2) {
> ctrlpriv->qi = (struct caam_queue_if __iomem __force *)
> ((__force uint8_t *)ctrl +
> BLOCK_OFFSET * QI_BLOCK_NUMBER
> @@ -810,12 +812,13 @@ static int caam_probe(struct platform_device
> *pdev)
> (ring + JR_BLOCK_NUMBER) *
> BLOCK_OFFSET
> );
> - ctrlpriv->total_jobrs++;
> ring++;
> + ctrlpriv->caam_caps |= BIT(ring);
I think this deserves an own macro. At the moment you assume that
the lower bits are for the rings, right? I'd like to see something
like
ctrlpriv->caam_caps |= JR_PRESENT(ring);
then have
#define JR_PRESENT_MASK GENMASK(7, 0)
#define JR_PRESENT(x) (BIT(x) & JR_PRESENT_MASK)
together with all the other bits in caam_caps. Or something
along that. I guess you got the idea.
> }
>
> /* If no QI and no rings specified, quit and go home */
> - if ((!ctrlpriv->qi_present) && (!ctrlpriv->total_jobrs)) {
> + if (!(ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) &&
> + (hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK) == 0))
> {
> dev_err(dev, "no queues configured, terminating\n");
> return -ENOMEM;
> }
> @@ -832,7 +835,8 @@ static int caam_probe(struct platform_device *pdev)
> * already instantiated, do RNG instantiation
> * In case of SoCs with Management Complex, RNG is managed by MC f/w.
> */
> - if (!(ctrlpriv->mc_en && pr_support) && rng_vid >= 4) {
> + if (!((ctrlpriv->caam_caps & CAAM_CAPS_MC_ENABLED) && pr_support) &&
> + rng_vid >= 4) {
> ctrlpriv->rng4_sh_init =
> rd_reg32(&ctrl->r4tst[0].rdsta);
> /*
> @@ -900,8 +904,9 @@ static int caam_probe(struct platform_device *pdev)
> /* Report "alive" for developer to see */
> dev_info(dev, "device ID = 0x%016llx (Era %d)\n", caam_id,
> ctrlpriv->era);
> - dev_info(dev, "job rings = %d, qi = %d\n",
> - ctrlpriv->total_jobrs, ctrlpriv->qi_present);
> + dev_info(dev, "job rings = %ld, qi = %s\n",
> + hweight_long(ctrlpriv->caam_caps & CAAM_CAPS_JOBRS_MASK),
> + (ctrlpriv->caam_caps & CAAM_CAPS_QI_PRESENT) ? "yes" : "no");
>
> ret = devm_of_platform_populate(dev);
> if (ret)
> diff --git a/drivers/crypto/caam/intern.h
> b/drivers/crypto/caam/intern.h
> index 7d45b21bd55a..37f0b93c7087 100644
> --- a/drivers/crypto/caam/intern.h
> +++ b/drivers/crypto/caam/intern.h
> @@ -86,15 +86,15 @@ struct caam_drv_private {
>
> struct iommu_domain *domain;
>
> - /*
> - * Detected geometry block. Filled in from device tree if powerpc,
> - * or from register-based version detection code
> - */
> - u8 total_jobrs; /* Total Job Rings in device */
> - u8 qi_present; /* Nonzero if QI present in device */
> - u8 mc_en; /* Nonzero if MC f/w is active */
> + unsigned long caam_caps; /* CAAM Module capabilities */
> +
> +#define CAAM_CAPS_QI_PRESENT BIT(0) /* Queue Manager interface (QI)
> implemented */
> +#define CAAM_CAPS_JOBRS_MASK GENMASK(15, 1) /* Job Ring is available
> in NS World */
ok I see you already have something like that. See above. That BIT()
in the code above should go away.
> +#define CAAM_CAPS_MC_ENABLED BIT(16) /* Management Complex is enabled
> (F/W is active) */
> +#define CAAM_CAPS_VIRT_ENABLED BIT(17) /* Virtualization enabled */
> +#define CAAM_CAPS_64K_PAGESIZE BIT(18) /* CAAM register page size
> (64KB if set, 4KB if unset) */
> +
> int secvio_irq; /* Security violation interrupt number */
> - int virt_en; /* Virtualization enabled in CAAM */
> int era; /* CAAM Era (internal HW revision) */
>
> #define RNG4_MAX_HANDLES 2
> diff --git a/drivers/crypto/caam/regs.h b/drivers/crypto/caam/regs.h
> index 3738625c0250..186e76e6a3e7 100644
> --- a/drivers/crypto/caam/regs.h
> +++ b/drivers/crypto/caam/regs.h
> @@ -1023,6 +1023,4 @@ struct caam_deco {
> #define ASSURE_BLOCK_NUMBER 6
> #define QI_BLOCK_NUMBER 7
> #define DECO_BLOCK_NUMBER 8
> -#define PG_SIZE_4K 0x1000
> -#define PG_SIZE_64K 0x10000
nice ;)
> #endif /* REGS_H */
Otherwise, I really like this cleanup.
Thanks,
-michael
Powered by blists - more mailing lists