Re: [RFC PATCH 1/2] drivers/misc: add silex multipk driver

["Gupta, Nipun" <nipun.gupta@amd.com>]

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@amd.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