linux-kernel - Re: [PATCH v3 1/6] dt-bindings: crypto: Document support for SPAcc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALxtO0mH=GwhQxQBsmMQYd+qgAue9WxXN1XWo9BncVJvJk6d8A@mail.gmail.com>
Date: Wed, 4 Jun 2025 17:50:33 +0530
From: Pavitrakumar Managutte <pavitrakumarm@...avyalabs.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org, 
	devicetree@...r.kernel.org, herbert@...dor.apana.org.au, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, Ruud.Derwig@...opsys.com, 
	manjunath.hadli@...avyalabs.com, adityak@...avyalabs.com, 
	Bhoomika Kadabi <bhoomikak@...avyalabs.com>
Subject: Re: [PATCH v3 1/6] dt-bindings: crypto: Document support for SPAcc

Hi Krzysztof.
  Appreciate your inputs. My comments are embedded below.

Warm regards,
PK

On Tue, Jun 3, 2025 at 5:34 PM Krzysztof Kozlowski <krzk@...nel.org> wrote:
>
> On 03/06/2025 13:45, Pavitrakumar Managutte wrote:
> > Hi Krzysztof,
> >   Thanks for the inputs, my comments are embedded below.
> >
> > Warm regards,
> > PK
> >
> > On Mon, Jun 2, 2025 at 11:28 AM Krzysztof Kozlowski <krzk@...nel.org> wrote:
> >>
> >> On 02/06/2025 07:32, Pavitrakumar Managutte wrote:
> >>> Add DT bindings related to the SPAcc driver for Documentation.
> >>> DWC Synopsys Security Protocol Accelerator(SPAcc) Hardware Crypto
> >>> Engine is a crypto IP designed by Synopsys.
> >>>
> >>> Co-developed-by: Bhoomika Kadabi <bhoomikak@...avyalabs.com>
> >>> Signed-off-by: Bhoomika Kadabi <bhoomikak@...avyalabs.com>
> >>> Signed-off-by: Pavitrakumar Managutte <pavitrakumarm@...avyalabs.com>
> >>> Acked-by: Ruud Derwig <Ruud.Derwig@...opsys.com>
> >>
> >> Where was this Ack given? It's not on the lists, it's not public, so it
> >> cannot be after your SoB.
> >
> > PK: Yes, its not on the mailing list. I will remove that.
>
> If it was given in private, then happened for sure before you sent the
> patch, so it should be above your SoB.

PK: Sure, I will fix that. Yes, that was an internal Ack.

>
> ...
>
> >>> +
> >>> +  snps,vspacc-id:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description: |
> >>> +      Virtual SPAcc instance identifier.
> >>> +      The SPAcc hardware supports multiple virtual instances (determined by
> >>> +      ELP_SPACC_CONFIG_VSPACC_CNT parameter), and this ID is used to identify
> >>> +      which virtual instance this node represents.
> >>
> >> No, IDs are not accepted.
> >
> > PK: This represents the specific virtual SPAcc that is being used in
> > the current configuration. It is used to index into the register banks
> > and the context memories of the virtual SPAcc that is being used. The
> > SPAcc IP can be configured as dedicated virtual SPAccs in
> > heterogeneous environments.
>
> OK. Why registers are not narrowed to only this instance? It feels like
> you provide here full register space for multiple devices and then
> select the bank with above ID.

PK: No, we cant narrow the registers to only this instance since its
is just a single SPAcc with multiple virtual SPAcc instances. The same
set of registers(aka register banks) and context memories are
repeated, but sit at different offset addresses (i*4000 +
register-offsets). The crypto hardware engine inside is shared by all
the virtual SPAccs. This is very much for a heterogeneous computing
scenario.

>
>
> > This was also discssed with Rob Herring and updated from
> > "vpsacc-index" to "vspacc-id" based on Rob's inputs
> > https://lore.kernel.org/linux-crypto/CALxtO0mkmyaDYta0tfx9Q1qi_GY0OwUoFDDVmcL15UH_fEZ25w@mail.gmail.com/
>
> Yeah, it is still ID and thus look at his comment about proper
> justification.

PK: Agreed

>
> >
> >>
> >>> +    minimum: 0
> >>> +    maximum: 7
> >>> +
> >>> +  snps,spacc-internal-counter:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    description: |
> >>> +      Hardware counter that generates an interrupt based on a count value.
> >>> +      This counter starts ticking when there is a completed job sitting on
> >>> +      the status fifo to be serviced. This makes sure that no jobs are
> >>> +      starved of processing.
> >>
> >> Not a DT property.
> >
> > PK: This is a hardware counter which starts ticking when a processed
> > job is sitting on the STAT FIFO. This makes sure a JOB does not stay
> > in STATUS FIFO unprocessed.
> >
> > This was called watchdog timer - wdtimer, which we renamed to
> > "spacc-internal-counter" based on your inputs.
> > https://lore.kernel.org/linux-crypto/CALxtO0k4RkopERap_ykrMTZ4Qtdzm8hEPJGLCQ2pknQGjfQ4Eg@mail.gmail.com/
>
> I suggested to use watchdog schema if this device has a watchdog feature.
>
> Why would you configure here different values for the same hardware in
> different boards?

PK: Agreed, it does not make sense to have this here in DT. I am
moving this as a Kconfig option.

>
>
>
> Best regards,
> Krzysztof