Message-ID: <22f3b2c5-517f-5bcf-157d-3ade74cba548@linux.intel.com>
Date: Tue, 12 Aug 2025 13:21:49 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Ron Li <xiangrongl@...dia.com>
cc: Herbert Xu <herbert@...dor.apana.org.au>, 
    "David S. Miller" <davem@...emloft.net>, 
    "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>, 
    Hans de Goede <hdegoede@...hat.com>, Vadim Pasternak <vadimp@...dia.com>, 
    Khalil Blaiech <kblaiech@...dia.com>, 
    David Thompson <davthompson@...dia.com>, 
    "platform-driver-x86@...r.kernel.org" <platform-driver-x86@...r.kernel.org>, 
    LKML <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1] platform/mellanox: Add mlxbf_pka driver for BlueField
 Soc

On Mon, 11 Aug 2025, Ron Li wrote:

> Hi Ilpo,
> I have answers for three of the review questions. You can search for "Ron Answer" to locate them.

Next time if the mail is long, please trim the response to only contain 
the relevant parts of the exchange so you won't even need to mark them 
like that. :-)

> All the other review questions have been fixed in the patch v2.
>
> > -----Original Message-----
> > From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
> > Sent: Friday, May 16, 2025 5:20 AM
> > To: Ron Li <xiangrongl@...dia.com>; Herbert Xu
> > <herbert@...dor.apana.org.au>; David S. Miller <davem@...emloft.net>;
> > linux-crypto@...r.kernel.org
> > Cc: Hans de Goede <hdegoede@...hat.com>; Vadim Pasternak
> > <vadimp@...dia.com>; Khalil Blaiech <kblaiech@...dia.com>; David
> > Thompson <davthompson@...dia.com>; platform-driver-
> > x86@...r.kernel.org; LKML <linux-kernel@...r.kernel.org>
> > Subject: Re: [PATCH v1] platform/mellanox: Add mlxbf_pka driver for BlueField
> > Soc
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Thu, 15 May 2025, Ron Li wrote:
> > 
> > > Add the mlxbf_pka driver to support the BlueField SoC Public Key
> > > Acceleration (PKA) hardware. The PKA provides a simple, complete
> > > framework for crypto public key hardware offload. It supports direct
> > > access to the public key hardware resources from the user space, and
> > > makes available several arithmetic operations: some basic operations
> > > (e.g., addition and multiplication), some complex operations (e.g.,
> > > modular exponentiation and modular inversion), and high-level
> > > operations such as RSA, Diffie-Hallman, Elliptic Curve Cryptography,
> > > and the Federal Digital Signature Algorithm (DSA as documented in
> > > FIPS-186) public-private key systems.
> > >
> > > The PKA driver initializes the PKA hardware interface and implements
> > > file operations so that user space libraries can bypass the kernel and
> > > have direct access to a specific set of device registers. The Arm cores
> > > interface to the PKA hardware through rings and a 64KB memory known as
> > > Window RAM. There are multiple PKA devices on the BlueField SoC. In
> > > general, each PKA device has 4 rings, 1 window RAM and 1 True Random
> > > Number Generator (TRNG). Thus, the driver has been designed to probe
> > > each PKA and each individual ring inside a given PKA. It also registers
> > > the TRNG to feed the kernel entropy (i.e., /dev/hwrng). To implement
> > > such design, the driver creates individual device files for each ring
> > > and TRNG module. The ring device files are identified using their ids,
> > > i.e., /dev/mlxbf_pka/<ring_id>.
> > >
> > > The main driver logic such as probe() and remove() are implemented in
> > > mlxbf_pka_drv.c. The PKA ring device operations are also implemented in
> > > this source file, such as open(), release() and mmap().
> > >
> > > The mlxbf_pka_dev.c source file implements functions to operate the
> > > underlying PKA hardware, such as TRNG operation, PKA hardware I/O
> > > access, PKA memory resource operation, etc.
> > >
> > > The PKA driver is a lighweight driver that implements file operations
> > > and map memory regions of the PKA hardware to user space drivers and
> > > libraries. There is no in-kernel crypto support. Therefore, the PKA
> > > driver is included under drivers/platform/mellanox.
> > >
> > > Testing
> > >
> > > - Successful build of kernel for ARM64.
> > >
> > > - Tested ARM64 build on several Mellanox BlueField 2 and 3 SoC boards
> > > that include the PKA hardware. The testing includes the validation of
> > > the PKA hardware execution, random number generation and public key
> > > acceleration performance.
> > 
> > Hi,
> > 
> > We've the in-kernel crypto framework but I don't see any attempt to build
> > into that framework AFAICT. Why is that? You brush it off as "The PKA
> > driver is a lightweight driver ..." but lets see if the crypto people
> > agree with that approach (I added them).
> > 
> > (Please also Cc crypto people in any further submission.)

I don't see crypto people Cc'ed in v2 :-(.

> > > +     ring->ring_info->cmmd_base = cmd_desc_ring_base;
> > 
> > Is cmmd a typo?
> > 
> Ron Answer: This is from the PKA IP document description:
> Command ring base address control words (RING_CMMD_BASE_0 … _n).

Hmm, is that document consistent in shortening "command" to "cmmd"? It 
feel quite untypical and might be a typo in that document too.

If it looks inconsistent in that document, my suggestion is to use 
cmd_base and add a comment where you define ring_info's struct to map it 
into the RING_CMMD_BASE_0 spelling in the document.

> > > +             /* Check test status bits. */
> > > +             csr_reg_off = mlxbf_pka_dev_get_register_offset(csr_reg_base,
> > > +                                                             MLXBF_PKA_TRNG_INTACK_ADDR);
> > > +             if (status & MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL) {
> > > +                     mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> > > +                                            MLXBF_PKA_TRNG_STATUS_MONOBIT_FAIL);
> > > +                     *monobit_fail_cnt += 1;
> > > +             } else if (status & MLXBF_PKA_TRNG_STATUS_RUN_FAIL) {
> > > +                     mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> > > +                                            MLXBF_PKA_TRNG_STATUS_RUN_FAIL);
> > > +                     *run_fail_cnt += 1;
> > > +             } else if (status & MLXBF_PKA_TRNG_STATUS_POKER_FAIL) {
> > > +                     mlxbf_pka_dev_io_write(csr_reg_ptr, csr_reg_off,
> > > +                                            MLXBF_PKA_TRNG_STATUS_POKER_FAIL);
> > > +                     *poker_fail_cnt += 1;
> > > +             }
> > 
> > Are these fails prioritized like this so it's should count just into one
> > counter if more than one FAIL is asserted?
> > 
> Ron Answer: the monobit_fail_cnt, run_fail_cnt and poker_fail_cnt are used to represent different type
> of failure counts. Will be printed separately in debug message.

Unfortunately, this didn't answer my question. Can those bits be asserted 
at the same time? If they can, is it okay to count the failure into just a 
one of the failure counters?

> > > +static void mlxbf_pka_drv_get_mem_res(struct mlxbf_pka_device
> > *mlxbf_pka_dev,
> > > +                                   struct mlxbf_pka_dev_mem_res *mem_res,
> > > +                                   u64 wndw_ram_off_mask)
> > > +{
> > > +     enum mlxbf_pka_mem_res_idx acpi_mem_idx;
> > > +
> > > +     acpi_mem_idx = MLXBF_PKA_ACPI_EIP154_IDX;
> > > +     mem_res->wndw_ram_off_mask = wndw_ram_off_mask;
> > > +
> > > +     /* PKA EIP154 MMIO base address. */
> > > +     mem_res->eip154_base = mlxbf_pka_dev->resource[acpi_mem_idx]-
> > >start;
> > > +     mem_res->eip154_size = mlxbf_pka_dev->resource[acpi_mem_idx]-
> > >end -
> > > +                            mem_res->eip154_base + 1;
> > 
> > resource_size(), please change all of them.
> > 
> > > +     acpi_mem_idx++;
> > > +
> > > +     /* PKA window RAM base address. */
> > > +     mem_res->wndw_ram_base = mlxbf_pka_dev-
> > >resource[acpi_mem_idx]->start;
> > > +     mem_res->wndw_ram_size = mlxbf_pka_dev->resource[acpi_mem_idx]-
> > >end -
> > > +                              mem_res->wndw_ram_base + 1;
> > > +     acpi_mem_idx++;
> > > +
> > > +     /*
> > > +      * PKA alternate window RAM base address.
> > > +      * Note: the size of all the alt window RAM is the same, depicted by
> > 'alt_wndw_ram_size'
> > > +      * variable. All alt window RAM resources are read here even though not
> > all of them are used
> > > +      * currently.
> > > +      */
> > > +     mem_res->alt_wndw_ram_0_base = mlxbf_pka_dev-
> > >resource[acpi_mem_idx]->start;
> > > +     mem_res->alt_wndw_ram_size   = mlxbf_pka_dev-
> > >resource[acpi_mem_idx]->end -
> > > +                                    mem_res->alt_wndw_ram_0_base + 1;
> > > +
> > > +     if (mem_res->alt_wndw_ram_size !=
> > MLXBF_PKA_WINDOW_RAM_REGION_SIZE)
> > > +             dev_err(mlxbf_pka_dev->device, "alternate Window RAM size from
> > ACPI is wrong.\n");
> > 
> > Should this return error and result in failing to register the device?
> > 
> Ron Answer: if the alternative Window RAM is not properly initialized, the PKA will fall back to contiguous
> Window RAM.
> Also, the alternative Window RAM is not available in BlueField DPU. This check will give warning,
> in case the DPU customer manually updated the ACIP table to use alternative Window RAM.

Please downgrade it into dev_warn() then.

Things like this might be nice to mention also into changelog so that it 
is permanently recorded somewhere. At least to me, the fallback doesn't 
look obvious from the code alone and one day it might be that neither of 
us is around to tell it when a 3rd person comes and has to figure this 
code out. :-)

> > This is extremely long and it would make reviewing it significantly easier
> > if it would be split into multiple, logical patches. Mixing two types of
> > devices in a single probe adds to the confusion, can like one of those be
> > introduced first and then the other?

I think you might have misunderstood this comment. I didn't mean to 
address my code style and kernel API comments in separate patches, those 
should be used right when new code is introduced.

It would be large benefit if this single big change could be split into 
multiple patches so that you introduce this support in multiple steps.
As noted above, one potential thing would be to add the types in different 
changes (if possible). If there are features which can be separated into 
own patches, that would also help to bring the lines added per patch down 
helping to review each patch, and allow writing more focused changelog 
entry for each change, etc.

Please do realize that by posting a large patch like this, you'll place 
this patch into a very disadvantaged position. Small patches are easy to 
review quickly, whereas large one like this take considerable time, so the 
chance for me or other reviewers finding time to go through it are much 
lower than if the series consists smaller patches.


-- 
 i.

Powered by blists - more mailing lists