| Message ID | 20230130154242.112613-1-jiajie.ho@starfivetech.com |
|---|---|
| Headers | show |
| Series | crypto: starfive - Add drivers for crypto engine | expand |
> -----Original Message----- > From: Jia Jie Ho <jiajie.ho@starfivetech.com> > Sent: 30 January, 2023 11:43 PM > To: Herbert Xu <herbert@gondor.apana.org.au>; David S . Miller > <davem@davemloft.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner Berthing > <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com> > Cc: linux-crypto@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-riscv@lists.infradead.org > Subject: [PATCH v2 0/4] crypto: starfive - Add drivers for crypto engine > > This patch series adds kernel driver support for StarFive JH7110 crypto engine. > The first patch add Documentations for the device and Patch 2 adds device > probe and DMA init for the module. Patch 3 adds crypto and DMA dts node > for VisionFive 2 board. Patch 4 adds hash/hmac support to the module. > > Patch 3 needs to be applied on top of: > https://patchwork.kernel.org/project/linux- > riscv/patch/20221220011247.35560-7-hal.feng@starfivetech.com/ > https://patchwork.kernel.org/project/linux- > riscv/cover/20230120024445.244345-1-xingyu.wu@starfivetech.com/ > > Changes v1->v2: > - Fixed yaml filename and format (Krzysztof) > - Removed unnecessary property names in yaml (Krzysztof) > - Moved of_device_id table close to usage (Krzysztof) > - Use dev_err_probe for error returns (Krzysztof) > - Dropped redundant readl and writel wrappers (Krzysztof) > - Updated commit signed offs (Conor) > - Dropped redundant node in dts, module set to on in dtsi (Conor) > > Jia Jie Ho (4): > dt-bindings: crypto: Add StarFive crypto module > crypto: starfive - Add crypto engine support > riscv: dts: starfive: Add crypto and DMA node for VisionFive 2 > crypto: starfive - Add hash and HMAC support > > .../crypto/starfive,jh7110-crypto.yaml | 70 ++ > MAINTAINERS | 7 + > arch/riscv/boot/dts/starfive/jh7110.dtsi | 27 + > drivers/crypto/Kconfig | 1 + > drivers/crypto/Makefile | 1 + > drivers/crypto/starfive/Kconfig | 20 + > drivers/crypto/starfive/Makefile | 4 + > drivers/crypto/starfive/starfive-cryp.c | 238 ++++ > drivers/crypto/starfive/starfive-hash.c | 1095 +++++++++++++++++ > drivers/crypto/starfive/starfive-regs.h | 71 ++ > drivers/crypto/starfive/starfive-str.h | 99 ++ > 11 files changed, 1633 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/crypto/starfive,jh7110-crypto.yaml > create mode 100644 drivers/crypto/starfive/Kconfig create mode 100644 > drivers/crypto/starfive/Makefile create mode 100644 > drivers/crypto/starfive/starfive-cryp.c > create mode 100644 drivers/crypto/starfive/starfive-hash.c > create mode 100644 drivers/crypto/starfive/starfive-regs.h > create mode 100644 drivers/crypto/starfive/starfive-str.h > > -- > 2.25.1 Hi all, Could you please review this patch series? Or should I send a new version with Rob's sign-off on patch 1? Thank you in advance. Best regards, Jia Jie
On Mon, Jan 30, 2023 at 11:42:42PM +0800, Jia Jie Ho wrote: > > + cryp->hash_data = (void *)__get_free_pages(GFP_KERNEL | GFP_DMA32, pages); Why do you copy everything before you feed it to the hardware? If the issue is alignment then surely you should only to copy a small amount of header (and perhaps trailer) for that? > +static int starfive_hash_export(struct ahash_request *req, void *out) > +{ > + struct starfive_cryp_request_ctx *rctx = ahash_request_ctx(req); > + > + memcpy(out, rctx, sizeof(*rctx)); > + > + return 0; > +} You are supposed to extract the entire hardware state after each operation and store that in the request context. Since your request context doesn't appear to contain any hash state, this can't possibly work. Does your hardware allow the non-finalised hash state to be exported, and re-imported later? If not then you can only implement support for digest and must use a fallback for everything else. Cheers,
> -----Original Message----- > From: Herbert Xu <herbert@gondor.apana.org.au> > Sent: 9 February, 2023 5:15 PM > To: JiaJie Ho <jiajie.ho@starfivetech.com> > Cc: David S . Miller <davem@davemloft.net>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Emil Renner Berthing > <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>; linux- > crypto@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-riscv@lists.infradead.org > Subject: Re: [PATCH v2 4/4] crypto: starfive - Add hash and HMAC support > > On Mon, Jan 30, 2023 at 11:42:42PM +0800, Jia Jie Ho wrote: > > > > + cryp->hash_data = (void *)__get_free_pages(GFP_KERNEL | > GFP_DMA32, > > +pages); > > Why do you copy everything before you feed it to the hardware? > If the issue is alignment then surely you should only to copy a small amount > of header (and perhaps trailer) for that? > The DMA can only support 32-bit addressing. So, I am copying everything in case kernel allocated memory region >32-bit for a user app. > > +static int starfive_hash_export(struct ahash_request *req, void *out) > > +{ > > + struct starfive_cryp_request_ctx *rctx = ahash_request_ctx(req); > > + > > + memcpy(out, rctx, sizeof(*rctx)); > > + > > + return 0; > > +} > > You are supposed to extract the entire hardware state after each operation > and store that in the request context. Since your request context doesn't > appear to contain any hash state, this can't possibly work. > > Does your hardware allow the non-finalised hash state to be exported, and > re-imported later? If not then you can only implement support for digest and > must use a fallback for everything else. The hardware doesn't support this. I'll add the fallback in the next version. Thanks for taking time reviewing this patch series. Regards, Jia Jie
On Thu, Feb 09, 2023 at 09:33:06AM +0000, JiaJie Ho wrote: > > The DMA can only support 32-bit addressing. > So, I am copying everything in case kernel allocated memory region >32-bit for a user app. Does your hardware support scatter-and-gather? If so you should at least allocate individual pages rather than one contiguous buffer. Then you can allocate them on-demand rather than before-hand. It would also be nice to not do the copy if the input you were given was in low memory (and contiguous if your hardware doesn't do SG). Thanks,
On Mon, Jan 30, 2023 at 11:42:42PM +0800, Jia Jie Ho wrote: > > +static inline int starfive_hash_wait_hmac_done(struct starfive_cryp_ctx *ctx) > +{ > + struct starfive_cryp_dev *cryp = ctx->cryp; > + u32 status; > + > + return readl_relaxed_poll_timeout(cryp->base + STARFIVE_HASH_SHACSR, status, > + (status & STARFIVE_HASH_HMAC_DONE), 10, 100000); > +} > + > +static inline int starfive_hash_wait_busy(struct starfive_cryp_ctx *ctx) > +{ > + struct starfive_cryp_dev *cryp = ctx->cryp; > + u32 status; > + > + return readl_relaxed_poll_timeout(cryp->base + STARFIVE_HASH_SHACSR, status, > + !(status & STARFIVE_HASH_BUSY), 10, 100000); > +} > + > +static inline int starfive_hash_wait_key_done(struct starfive_cryp_ctx *ctx) > +{ > + struct starfive_cryp_dev *cryp = ctx->cryp; > + u32 status; > + > + return readl_relaxed_poll_timeout(cryp->base + STARFIVE_HASH_SHACSR, status, > + (status & STARFIVE_HASH_KEY_DONE), 10, 100000); > +} Is there no IRQ mechanism for this? Cheers,
> -----Original Message----- > From: Herbert Xu <herbert@gondor.apana.org.au> > Sent: 9 February, 2023 5:44 PM > To: JiaJie Ho <jiajie.ho@starfivetech.com> > Cc: David S . Miller <davem@davemloft.net>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Emil Renner Berthing > <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>; linux- > crypto@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-riscv@lists.infradead.org > Subject: Re: [PATCH v2 4/4] crypto: starfive - Add hash and HMAC support > > On Thu, Feb 09, 2023 at 09:33:06AM +0000, JiaJie Ho wrote: > > > > The DMA can only support 32-bit addressing. > > So, I am copying everything in case kernel allocated memory region >32-bit > for a user app. > > Does your hardware support scatter-and-gather? If so you should at least > allocate individual pages rather than one contiguous buffer. > > Then you can allocate them on-demand rather than before-hand. > > It would also be nice to not do the copy if the input you were given was in > low memory (and contiguous if your hardware doesn't do SG). > I'll try this then. Thanks, Jia Jie
> -----Original Message----- > From: Herbert Xu <herbert@gondor.apana.org.au> > Sent: 9 February, 2023 5:47 PM > To: JiaJie Ho <jiajie.ho@starfivetech.com> > Cc: David S . Miller <davem@davemloft.net>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Emil Renner Berthing > <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>; linux- > crypto@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-riscv@lists.infradead.org > Subject: Re: [PATCH v2 4/4] crypto: starfive - Add hash and HMAC support > > On Mon, Jan 30, 2023 at 11:42:42PM +0800, Jia Jie Ho wrote: > > > > +static inline int starfive_hash_wait_hmac_done(struct > > +starfive_cryp_ctx *ctx) { > > + struct starfive_cryp_dev *cryp = ctx->cryp; > > + u32 status; > > + > > + return readl_relaxed_poll_timeout(cryp->base + > STARFIVE_HASH_SHACSR, status, > > + (status & > STARFIVE_HASH_HMAC_DONE), 10, 100000); } > > + > > +static inline int starfive_hash_wait_busy(struct starfive_cryp_ctx > > +*ctx) { > > + struct starfive_cryp_dev *cryp = ctx->cryp; > > + u32 status; > > + > > + return readl_relaxed_poll_timeout(cryp->base + > STARFIVE_HASH_SHACSR, status, > > + !(status & STARFIVE_HASH_BUSY), > 10, 100000); } > > + > > +static inline int starfive_hash_wait_key_done(struct > > +starfive_cryp_ctx *ctx) { > > + struct starfive_cryp_dev *cryp = ctx->cryp; > > + u32 status; > > + > > + return readl_relaxed_poll_timeout(cryp->base + > STARFIVE_HASH_SHACSR, status, > > + (status & > STARFIVE_HASH_KEY_DONE), 10, 100000); } > > Is there no IRQ mechanism for this? Only hmac done has IRQ, I'll add that in the next version. Thanks Jia Jie
On Thu, Feb 09, 2023 at 09:33:06AM +0000, JiaJie Ho wrote:
.
> The DMA can only support 32-bit addressing.
Isn't the DMA mapping API supposed to solve this automatically
for you? IOW just do the dma_map call and it should transparently
copy any high addresses to lowmem bounce buffers.
Cheers,
On Thu, Feb 09, 2023 at 09:33:06AM +0000, JiaJie Ho wrote: > > Why do you copy everything before you feed it to the hardware? > > If the issue is alignment then surely you should only to copy a small amount > > of header (and perhaps trailer) for that? > > > > The DMA can only support 32-bit addressing. > So, I am copying everything in case kernel allocated memory region >32-bit for a user app. The DMA API takes care of that.
On Thu, Feb 09, 2023 at 05:43:34PM +0800, Herbert Xu wrote: > On Thu, Feb 09, 2023 at 09:33:06AM +0000, JiaJie Ho wrote: > > > > The DMA can only support 32-bit addressing. > > So, I am copying everything in case kernel allocated memory region >32-bit for a user app. > > Does your hardware support scatter-and-gather? If so you should > at least allocate individual pages rather than one contiguous buffer. > > Then you can allocate them on-demand rather than before-hand. > > It would also be nice to not do the copy if the input you were > given was in low memory (and contiguous if your hardware doesn't > do SG). All of that is done by the DMA API, or more specifically swiotlb and does not need to be duplicated in individual drivers.
> -----Original Message----- > From: Christoph Hellwig <hch@infradead.org> > Sent: 14 February, 2023 2:13 PM > To: Herbert Xu <herbert@gondor.apana.org.au> > Cc: JiaJie Ho <jiajie.ho@starfivetech.com>; David S . Miller > <davem@davemloft.net>; Rob Herring <robh+dt@kernel.org>; Krzysztof > Kozlowski <krzysztof.kozlowski+dt@linaro.org>; Emil Renner Berthing > <kernel@esmil.dk>; Conor Dooley <conor.dooley@microchip.com>; linux- > crypto@vger.kernel.org; devicetree@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-riscv@lists.infradead.org > Subject: Re: [PATCH v2 4/4] crypto: starfive - Add hash and HMAC support > > On Thu, Feb 09, 2023 at 05:43:34PM +0800, Herbert Xu wrote: > > On Thu, Feb 09, 2023 at 09:33:06AM +0000, JiaJie Ho wrote: > > > > > > The DMA can only support 32-bit addressing. > > > So, I am copying everything in case kernel allocated memory region >32- > bit for a user app. > > > > Does your hardware support scatter-and-gather? If so you should at > > least allocate individual pages rather than one contiguous buffer. > > > > Then you can allocate them on-demand rather than before-hand. > > > > It would also be nice to not do the copy if the input you were given > > was in low memory (and contiguous if your hardware doesn't do SG). > > All of that is done by the DMA API, or more specifically swiotlb and does not > need to be duplicated in individual drivers. I'll update the driver accordingly. Thanks Christoph and Herbert for the pointers. Regards Jia Jie