| 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: <b43b6051-238b-207f-f0c5-3071950c1a0f@amd.com>
Date: Thu, 13 Mar 2025 14:46:11 +0530
From: "Gupta, Nipun" <nipun.gupta@....com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: linux-kernel@...r.kernel.org, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, derek.kiernan@....com, dragan.cvetic@....com,
arnd@...db.de, praveen.jain@....com, harpreet.anand@....com,
nikhil.agarwal@....com, srivatsa@...il.mit.edu, code@...icks.com,
ptsm@...ux.microsoft.com
Subject: Re: [RFC PATCH 1/2] drivers/misc: add silex multipk driver
On 12-03-2025 15:48, Greg KH wrote:
> On Wed, Mar 12, 2025 at 03:24:20PM +0530, Nipun Gupta wrote:
>> Silex MultiPK device provides a char device interface to interact
>> with Silex device for offloading asymmetric crypto operations.
>>
>> Signed-off-by: Nipun Gupta <nipun.gupta@....com>
>> ---
>>
>> Silex MultiPK device is a PKI offload device, which has multiple PK engines to
>> perform asymmetric crypto operations like ECDSA, RSA, Point Multiplication etc.
>>
>> Following provides the brief overview of the device interface with the software:
>>
>> +------------------+
>> | Software |
>> +------------------+
>> | |
>> | v
>> | +-----------------------------------------------------------+
>> | | RAM |
>> | | +----------------------------+ +---------------------+ |
>> | | | RQ pages | | CQ pages | |
>> | | | +------------------------+ | | +-----------------+ | |
>> | | | | START (cmd) | | | | req_id | status | | |
>> | | | | TFRI (addr, sz)---+ | | | | req_id | status | | |
>> | | | | +-TFRO (addr, sz) | | | | | ... | | |
>> | | | | | NTFY (req_id) | | | | +-----------------+ | |
>> | | | +-|-------------------|--+ | | | |
>> | | | | v | +---------------------+ |
>> | | | | +-----------+ | |
>> | | | | | input | | |
>> | | | | | data | | |
>> | | | v +-----------+ | |
>> | | | +----------------+ | |
>> | | | | output data | | |
>> | | | +----------------+ | |
>> | | +----------------------------+ |
>> | | |
>> | +-----------------------------------------------------------+
>> |
>> |
>> +---|----------------------------------------------------+
>> | v Silex MultiPK device |
>> | +-------------------+ +------------------------+ |
>> | | New request FIFO | --> | PK engines | |
>> | +-------------------+ +------------------------+ |
>> +--------------------------------------------------------+
>>
>> To perform a crypto operation, the software writes a sequence of descriptors,
>> into the RQ memory. This includes input data and designated location for the
>> output data. After preparing the request, request offset (from the RQ memory
>> region) is written into the NEW_REQUEST register. Request is then stored in a
>> common hardware FIFO shared among all RQs. When a PK engine becomes available,
>> device pops the request from the FIFO and fetches the descriptors. It DMAs the
>> input data from RQ memory and executes the necessary computations. After
>> computation is complete, the device writes output data back to RAM via DMA.
>> Device then writes a new entry in CQ ring buffer in RAM, indicating completion
>> of the request. Device also generates an interrupt for notifying completion to
>> the software.
>>
>> As Crypto AF_ALG does not support offloading asymmetric operations from
>> user-space (which was attempted to be added earlier in Linux at:
>> https://lore.kernel.org/all/146672253157.23101.15291203749122389409.stgit@tstruk-mobl1.ra.intel.com/),
>> RQ memory is provided to the user-space via mmap, so that application can
>> directly write to the descriptors.
>>
>> P.S. Most of the above text is also added as part of silex driver file.
>
> All of that should be in the changelog text, don't put it down here
> where it will never be seen again.
Sure will add it in the change log.
>
>> +static ssize_t hardware_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>
> Where is the Documentation/ABI/ entries for your new sysfs files?
Yes, will add documentation for ABI entry/ies
>
>> + int v, hwv, cnt, maxtotalreqs, rqmaxpending, mults;
>> + struct multipk_dev *mpkdev = dev_get_drvdata(dev);
>> +
>> + v = (int)sx_rdreg(mpkdev->regs, REG_SEMVER);
>> + hwv = (int)sx_rdreg(mpkdev->regs, REG_HW_VERSION);
>> + cnt = (int)sx_rdreg(mpkdev->regs, REQ_CFG_REQ_QUEUES_CNT);
>> + maxtotalreqs = (int)sx_rdreg(mpkdev->regs, REQ_CFG_MAX_PENDING_REQ);
>> + rqmaxpending = (int)sx_rdreg(mpkdev->regs, REQ_CFG_MAX_REQ_QUEUE_ENTRIES);
>> + mults = (int)sx_rdreg(mpkdev->regs, REQ_CFG_PK_INST);
>> +
>> + return sprintf(buf,
>> + "Hardware interface version: %d.%d.%d\n"
>> + "Hardware implementation version: %d.%d.%d\n"
>> + "Count request queues: %d\n"
>> + "Total max pending requests: %d\n"
>> + "Max pending requests per request queue: %d\n"
>> + "Pkcores 64 multipliers: %d\n"
>> + "Pkcores 256 multipliers: %d\n",
>> + MPK_SEMVER_MAJOR(v), MPK_SEMVER_MINOR(v), MPK_SEMVER_PATCH(v),
>> + MPK_HWVER_MAJOR(hwv), MPK_HWVER_MINOR(hwv), MPK_HWVER_SVN(hwv),
>> + cnt, maxtotalreqs, rqmaxpending,
>> + mults >> 16, mults & 0xFFFF);
>
> No!
>
> sysfs is "one value per file", which this is not at all.
Will create separate entries for each.
>
> Also, did you run checkpatch.pl? It should have told you that you
> should not be calling sprintf() here either.
I did run checkpatch on patches (as well as separate files), but it did
not report it. Will use seq_printf instead.
>
>> +static int __init multipk_init(void)
>> +{
>> + dev_t devt;
>> + int ret;
>> +
>> + multipk_class = class_create("multipk");
>
> Why do you need a whole new class? Why not just use a misc device?
We will update to use misc device instead of creating new class.
>
>> + if (IS_ERR(multipk_class)) {
>> + ret = PTR_ERR(multipk_class);
>> + pr_err("can't register class\n");
>> + goto err;
>> + }
>> + ret = alloc_chrdev_region(&devt, 0, MULTIPK_MAX_DEVICES, "multipk");
>
> Again, why not a dynamic misc device per device in the system?
>
> No need to make this harder than it is.
>
> But again, this really should use the in-kernel apis we already have for
> this type of hardware, don't make a custom user/kernel api at all.
> That's not going to scale or be easy to maintain for any amount of time.
Agree to some extent, but as Crypto AF_ALG does not support offloading
asymmetric operations, we added this driver as misc driver. This device
is supported in AMD Versal series devices:
https://www.amd.com/en/products/adaptive-socs-and-fpgas/versal/premium-series.html.
We would maintain the driver with minimal possible user interface changes.
Thanks,
Nipun
>
> thanks,
>
> greg k-h
Powered by blists - more mailing lists