From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id EE7452F8EA5 for ; Tue, 7 Jul 2026 10:12:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783419138; cv=none; b=N5V52ZmdeatQQJ/SvxyCVSpzQkdhb3qGzd/XNik1WqhrF9b/tXU1L0KMGct+9OEyNcjeGb36xDcjoPxwNXp+93gSyzNU77PtzxAI+0T8ALMQTM95noyhNvj38qbKJrca83G/ckgaP8P5Nadtwba0GZKLZBsjk+GzcUJyc7Sacts= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1783419138; c=relaxed/simple; bh=vWOP24ondHw/gw/XIDfo+j/GkazolrF1zP1ahWRqoB4=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=TbUfa5sZTL3LHoASB0X4TN1kkQHhwJBx2xKCAnCU0qEeo9LMguaunV7b6NONehef1/h8fZOYG8yAlIbHdN2+k6Y0pPRdLCK5t+EsApFH9e0lfG7SAtBf0nwMNpZkXqtrET1asnEnw0/79ZWHhz5gzqDQ6Uk0aQoEkKnvKLtcjJs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b=rU4gsA1Q; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.b="rU4gsA1Q" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 15F891F000E9; Tue, 7 Jul 2026 10:12:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linuxfoundation.org; s=korg; t=1783419136; bh=UjUUVLi5jwwCyhIEiAiPugwvcMqy8YZ/Sdx3Jsr6f04=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=rU4gsA1QuNECl+NSx6CW7vRrbS6QI3iu5jzp3PNGwvSU1QD7cI7b+0hoPF3PG4gPE AWQRC5awG96c9gb0Uxd6P139xWIYMYCWt0uzDpIsNcqlmThBtSKs5QLf3U/FTY2tWv IigCmij9qUhgQzF9adxYxJzJrKQRqaz/MiJENhek= Date: Tue, 7 Jul 2026 12:12:14 +0200 From: Greg KH To: Harsh Jain Cc: arnd@arndb.de, michal.simek@amd.com, linux-kernel@vger.kernel.org, sarat.chand.savitala@amd.com, nkowlaku@amd.com Subject: Re: [PATCH 2/2] misc: Add Xilinx PUF driver Message-ID: <2026070704-encrypt-pavilion-0549@gregkh> References: <20260707095620.2795456-1-h.jain@amd.com> <20260707095620.2795456-3-h.jain@amd.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260707095620.2795456-3-h.jain@amd.com> On Tue, Jul 07, 2026 at 03:26:20PM +0530, Harsh Jain wrote: > Versal devices contain a physically unclonable function (PUF) block > that derives device-unique secrets from intrinsic silicon > characteristics. The PUF generates a Key Encryption Key (KEK) used > internally by the AES engine for secure key storage, which is not > externally accessible. In addition, the PUF provides a readable > device-unique identifier derived from the same entropy source. So why isn't this tied into the normal crypto/keys/tee/whatever subsystem for this type of hardware? Why is a custom user/kernel api ok for this one specific piece of hardware? And where is the userspace code that interacts with this? > Add a misc character driver to support PUF registration and regeneration. > Registration generates a unique secret and outputs binary helper data > that can later be used during regeneration to reproduce the same > secrets. > > The driver also implements PUF_CLEAR_ID and PUF_CLEAR_KEY ioctls to clear > the PUF ID and PUF-derived AES key via firmware. They are disabled by default > unless puf_clear is set. Please wrap the changelog at 72 columns, checkpatch should have caught this, right? > > Signed-off-by: Harsh Jain > --- > MAINTAINERS | 6 + > drivers/misc/Kconfig | 11 + > drivers/misc/Makefile | 1 + > drivers/misc/xilinx_puf.c | 377 +++++++++++++++++++++++++++++++++ > include/uapi/misc/xilinx_puf.h | 82 +++++++ > 5 files changed, 477 insertions(+) > create mode 100644 drivers/misc/xilinx_puf.c > create mode 100644 include/uapi/misc/xilinx_puf.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 15011f5752a9..a46a34f163b4 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -29546,6 +29546,12 @@ L: netdev@vger.kernel.org > S: Orphan > F: drivers/net/ethernet/xilinx/ll_temac* > > +XILINX PUF DRIVER > +M: Harsh Jain > +S: Maintained You aren't paid to support this? AMD doesn't care? That's sad. > +F: drivers/misc/xilinx_puf.c > +F: include/uapi/misc/xilinx_puf.h > + > XILINX PWM DRIVER > M: Sean Anderson > S: Maintained > diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig > index 390256ed91f4..98ec00765647 100644 > --- a/drivers/misc/Kconfig > +++ b/drivers/misc/Kconfig > @@ -490,6 +490,17 @@ config XILINX_SDFEC > > If unsure, say N. > > +config XILINX_PUF > + tristate "Xilinx PUF driver" > + depends on ZYNQMP_FIRMWARE > + help > + This option enables support for the Xilinx Physically Unclonable Function > + (PUF) driver on AMD Versal devices. > + It is a configurable driver to generate PUF KEK source either by using > + PUF registration or regeneration command. > + > + If unsure, say N. Module name? > + > config MISC_RTSX > tristate > default MISC_RTSX_PCI || MISC_RTSX_USB > diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile > index fed47c7672b9..66d1ea5d1f13 100644 > --- a/drivers/misc/Makefile > +++ b/drivers/misc/Makefile > @@ -55,6 +55,7 @@ obj-$(CONFIG_BCM_VK) += bcm-vk/ > obj-y += cardreader/ > obj-$(CONFIG_PVPANIC) += pvpanic/ > obj-$(CONFIG_UACCE) += uacce/ > +obj-$(CONFIG_XILINX_PUF) += xilinx_puf.o > obj-$(CONFIG_XILINX_SDFEC) += xilinx_sdfec.o > obj-$(CONFIG_HISI_HIKEY_USB) += hisi_hikey_usb.o > obj-$(CONFIG_NTSYNC) += ntsync.o > diff --git a/drivers/misc/xilinx_puf.c b/drivers/misc/xilinx_puf.c > new file mode 100644 > index 000000000000..ad8ada58b105 > --- /dev/null > +++ b/drivers/misc/xilinx_puf.c > @@ -0,0 +1,377 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Driver for Xilinx PUF device. > + * > + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc. > + * > + * Description: > + * This driver is developed for PUF registration and regeneration support. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +static bool puf_clear; > +module_param(puf_clear, bool, 0600); > +MODULE_PARM_DESC(puf_clear, "Flag to enable clearing of PUF ID and key."); This is not the 1990's, please do not add module parameters for things that should be done other ways. Especially for a device-specific thing. > + > +/** > + * struct puf_params - parameters for PUF > + * @pufoperation: PUF registration or regeneration operation > + * @globalvarfilter: global variation filter > + * @readoption: option to read PUF data from efuse cache or ram address > + * @reserved: explicit padding to match the Versal PLM PUF parameter layout. > + * @shuttervalue: shutter value for PUF registration/regeneration > + * @readsyndromeaddr: address to store the syndrome data during registration > + * @chashaddr: CHASH address > + * @auxaddr: AUX address > + * @pufidaddr: PUF ID address > + * @writesyndromeaddr: address where syndrome data is present and it is passed to the user > + * @trimsyndataaddr: trimmed syndrome data will be stored > + */ > +struct puf_params { > + u8 pufoperation; > + u8 globalvarfilter; > + u8 readoption; > + u8 reserved; > + u32 shuttervalue; > + u64 readsyndromeaddr; > + u64 chashaddr; > + u64 auxaddr; > + u64 pufidaddr; > + u64 writesyndromeaddr; > + u64 trimsyndataaddr; > +}; > + > +/** > + * struct xpuf_dev - Driver data for PUF > + * @dev: pointer to device struct > + * @miscdev: misc device handle > + */ > +struct xpuf_dev { > + struct device *dev; > + struct miscdevice miscdev; > +}; > + > +static int xlnx_puf_regis(struct xpuf_dev *puf, struct puf_usrparams *pufreq) > +{ > + struct puf_params *pufin; > + struct pufdata *pufdat; > + dma_addr_t dma_addr_data; > + dma_addr_t dma_addr_in; > + u32 buflen; > + void *buf; > + int ret; > + > + if (pufreq->pufoperation != PUF_REGIS) > + return -EINVAL; > + > + if (pufreq->readoption != PUF_READ_FROM_RAM && > + pufreq->readoption != PUF_READ_FROM_EFUSE_CACHE) > + return -EINVAL; > + > + buflen = sizeof(struct puf_params) + sizeof(struct pufdata); > + buf = kzalloc(buflen, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + pufin = buf; > + pufin->pufoperation = pufreq->pufoperation; > + pufin->globalvarfilter = pufreq->globalvarfilter; > + pufin->readoption = pufreq->readoption; > + pufin->shuttervalue = pufreq->shuttervalue; > + > + pufdat = buf + sizeof(struct puf_params); > + dma_addr_in = dma_map_single(puf->dev, buf, buflen, DMA_BIDIRECTIONAL); > + if (dma_mapping_error(puf->dev, dma_addr_in)) { > + ret = -ENOMEM; > + goto cleanup; > + } > + > + dma_addr_data = dma_addr_in + sizeof(struct puf_params); > + pufin->readsyndromeaddr = (u64)dma_addr_data; > + pufin->chashaddr = (u64)(pufin->readsyndromeaddr + sizeof(pufdat->pufhd.syndata)); > + pufin->auxaddr = (u64)(pufin->chashaddr + sizeof(pufdat->pufhd.chash)); > + pufin->pufidaddr = (u64)(pufin->auxaddr + sizeof(pufdat->pufhd.aux)); > + pufin->trimsyndataaddr = (u64)(pufin->pufidaddr + sizeof(pufdat->pufid)); > + > + dma_sync_single_for_device(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL); > + ret = versal_pm_puf_registration(dma_addr_in); > + dma_unmap_single(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL); > + if (ret) > + goto cleanup; > + > + if (copy_to_user(u64_to_user_ptr(pufreq->pufdataaddr), pufdat, sizeof(struct pufdata))) { > + ret = -EFAULT; > + goto cleanup; > + } > + > +cleanup: > + kfree(buf); > + > + return ret; > +} > + > +static int xlnx_puf_regen_id(struct xpuf_dev *puf, struct puf_usrparams *pufreq) > +{ > + struct puf_helperdata *pufhd; > + struct puf_params *pufin; > + dma_addr_t dma_addr_data; > + dma_addr_t dma_addr_in; > + u32 buflen; > + void *buf; > + int ret; > + > + if (pufreq->pufoperation != PUF_REGEN && > + pufreq->pufoperation != PUF_REGEN_ID) > + return -EINVAL; > + > + if (pufreq->readoption != PUF_READ_FROM_RAM && > + pufreq->readoption != PUF_READ_FROM_EFUSE_CACHE) > + return -EINVAL; > + > + buflen = sizeof(struct puf_params) + sizeof(struct puf_helperdata) + > + PUF_ID_LEN_IN_BYTES; > + > + buf = kzalloc(buflen, GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + pufin = buf; > + pufin->pufoperation = pufreq->pufoperation; > + pufin->globalvarfilter = pufreq->globalvarfilter; > + pufin->shuttervalue = pufreq->shuttervalue; > + pufin->readoption = pufreq->readoption; > + > + pufhd = buf + sizeof(struct puf_params); > + if (copy_from_user(pufhd, u64_to_user_ptr(pufreq->pufdataaddr), > + sizeof(struct puf_helperdata))) { > + ret = -EFAULT; > + goto cleanup; > + } > + > + dma_addr_in = dma_map_single(puf->dev, buf, buflen, DMA_BIDIRECTIONAL); > + if (dma_mapping_error(puf->dev, dma_addr_in)) { > + ret = -ENOMEM; > + goto cleanup; > + } > + > + dma_addr_data = dma_addr_in + sizeof(struct puf_params); > + pufin->writesyndromeaddr = (u64)dma_addr_data; > + pufin->chashaddr = (u64)(pufin->writesyndromeaddr + sizeof(pufhd->syndata)); > + pufin->auxaddr = (u64)(pufin->chashaddr + sizeof(pufhd->chash)); > + pufin->pufidaddr = (u64)(pufin->auxaddr + sizeof(pufhd->aux)); > + > + dma_sync_single_for_device(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL); > + ret = versal_pm_puf_regeneration(dma_addr_in); > + dma_unmap_single(puf->dev, dma_addr_in, buflen, DMA_BIDIRECTIONAL); > + if (ret) > + goto cleanup; > + > + if (copy_to_user(u64_to_user_ptr(pufreq->pufidaddr), ((char *)pufhd + > + sizeof(struct puf_helperdata)), > + PUF_ID_LEN_IN_BYTES)) { > + ret = -EFAULT; > + goto cleanup; > + } > + > +cleanup: > + kfree(buf); > + > + return ret; > +} > + > +static long xlnx_puf_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > +{ > + struct xpuf_dev *puf = file->private_data; > + struct puf_usrparams pufreq; > + void __user *data = NULL; > + int ret; > + > + if (_IOC_TYPE(cmd) != PUF_IOC_MAGIC) > + return -ENOTTY; > + > + /* check if ioctl argument is present and valid */ > + if (_IOC_DIR(cmd) != _IOC_NONE) { > + data = (void __user *)arg; > + if (!data) > + return -EINVAL; > + > + if (copy_from_user(&pufreq, data, sizeof(struct puf_usrparams))) > + return -EFAULT; > + } > + > + switch (cmd) { > + case PUF_REGISTRATION: > + ret = xlnx_puf_regis(puf, &pufreq); > + break; > + case PUF_REGENERATION: > + case PUF_REGEN_ID_ONLY: > + ret = xlnx_puf_regen_id(puf, &pufreq); > + break; > + case PUF_CLEAR_ID: > + if (!puf_clear) { > + ret = -EOPNOTSUPP; > + break; > + } > + ret = versal_pm_puf_clear_id(); > + break; > + case PUF_CLEAR_KEY: > + if (!puf_clear) { > + ret = -EOPNOTSUPP; > + break; > + } > + ret = versal_pm_aes_init(); > + if (!ret) > + ret = versal_pm_puf_key_zero(); > + break; > + default: > + return -ENOTTY; > + } > + > + return ret; > +} > + > +/** > + * xlnx_puf_open - open puf device > + * @inode: inode object > + * @file: file object > + * > + * Return: 0 if successful; otherwise -errno > + */ > +static int xlnx_puf_open(struct inode *inode, struct file *file) > +{ > + struct xpuf_dev *xpuf; > + > + xpuf = container_of(file->private_data, struct xpuf_dev, miscdev); > + file->private_data = xpuf; > + > + dev_dbg(xpuf->dev, "device /dev/xpuf opened\n"); > + > + return 0; > +} > + > +/** > + * xlnx_puf_release - release puf resources > + * @inode: inode object > + * @file: file object > + * > + * Return: 0 if successful; otherwise -errno > + */ > +static int xlnx_puf_release(struct inode *inode, struct file *file) > +{ > + struct xpuf_dev *xpuf = file->private_data; > + > + dev_dbg(xpuf->dev, "device /dev/xpuf closed\n"); > + > + return 0; > +} This does nothing, why is it needed at all? > + > +static const struct file_operations xlnx_puf_fops = { > + .owner = THIS_MODULE, > + .open = xlnx_puf_open, > + .release = xlnx_puf_release, > + .unlocked_ioctl = xlnx_puf_ioctl, > + .compat_ioctl = compat_ptr_ioctl, > +}; > + > +/** > + * xlnx_puf_probe - probe puf device > + * @pdev: Pointer to puf platform device structure > + * > + * Return: 0 if successful; otherwise -errno > + */ > +static int xlnx_puf_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct xpuf_dev *xpuf; > + int ret; > + > + xpuf = devm_kzalloc(dev, sizeof(*xpuf), GFP_KERNEL); > + if (!xpuf) > + return -ENOMEM; > + > + xpuf->dev = dev; > + > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64)); > + if (ret < 0) { > + ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32)); > + if (ret < 0) { > + dev_err(dev, "no usable DMA configuration\n"); > + return ret; > + } > + } > + > + xpuf->miscdev.minor = MISC_DYNAMIC_MINOR; > + xpuf->miscdev.name = "xpuf"; > + xpuf->miscdev.fops = &xlnx_puf_fops; > + xpuf->miscdev.parent = dev; > + > + ret = misc_register(&xpuf->miscdev); > + if (ret) > + return ret; > + > + platform_set_drvdata(pdev, xpuf); > + > + dev_dbg(dev, "puf registered as /dev/xpuf successfully\n"); > + > + return 0; > +} > + > +/** > + * xlnx_puf_remove - clean up structures > + * @pdev: The structure containing the device's details > + */ > +static void xlnx_puf_remove(struct platform_device *pdev) > +{ > + struct xpuf_dev *xpuf = platform_get_drvdata(pdev); > + > + misc_deregister(&xpuf->miscdev); > + > + dev_dbg(xpuf->dev, "device /dev/xpuf removed\n"); ftrace is your friend :) > +} > + > +static struct platform_driver xlnx_puf_drv = { > + .probe = xlnx_puf_probe, > + .remove = xlnx_puf_remove, > + .driver = { > + .name = "xlnx-puf", > + }, > +}; > + > +static struct platform_device *xlnx_puf_pdev; > + > +static int __init xlnx_puf_driver_init(void) > +{ > + int ret; > + > + ret = platform_driver_register(&xlnx_puf_drv); > + if (ret) > + return ret; > + > + xlnx_puf_pdev = platform_device_register_simple(xlnx_puf_drv.driver.name, > + 0, NULL, 0); Why isn't this using the faux bus instead? This isn't a real platform device at all as you have no resources for it that describe it. > + if (IS_ERR(xlnx_puf_pdev)) { > + ret = PTR_ERR(xlnx_puf_pdev); > + platform_driver_unregister(&xlnx_puf_drv); > + } > + > + return ret; > +} > + > +static void __exit xlnx_puf_driver_exit(void) > +{ > + platform_device_unregister(xlnx_puf_pdev); > + platform_driver_unregister(&xlnx_puf_drv); > +} > + > +module_init(xlnx_puf_driver_init); > +module_exit(xlnx_puf_driver_exit); > + > +MODULE_AUTHOR("AMD"); > +MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("Xilinx Versal PUF driver"); > diff --git a/include/uapi/misc/xilinx_puf.h b/include/uapi/misc/xilinx_puf.h > new file mode 100644 > index 000000000000..32565a748df3 > --- /dev/null > +++ b/include/uapi/misc/xilinx_puf.h > @@ -0,0 +1,82 @@ > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > +/* > + * Driver for Xilinx PUF device. > + * > + * Copyright (C) 2022 - 2026, Advanced Micro Devices, Inc. > + * > + * Description: > + * This driver is developed for PUF registration and regeneration support. > + */ > + > +#ifndef _UAPI_MISC_XILINX_PUF_H_ > +#define _UAPI_MISC_XILINX_PUF_H_ > + > +#include > +#include > + > +#define PUF_MAX_SYNDROME_DATA_LEN_IN_WORDS 140 > +#define PUF_EFUSE_TRIM_SYN_DATA_IN_WORDS 127 > +#define PUF_ID_LEN_IN_WORDS 8 > +#define PUF_ID_LEN_IN_BYTES 32 > +#define PUF_REGIS 0 > +#define PUF_REGEN 1 > +#define PUF_REGEN_ID 2 > + > +/** > + * struct puf_usrparams - user parameters for PUF from user space. > + * @pufoperation: PUF registration or regeneration operation > + * @globalvarfilter: global variation filter > + * @readoption: option to read PUF data from efuse cache or ram address > + * @reserved: explicit padding to match the Versal PLM PUF parameter layout > + * @shuttervalue: shutter value for PUF registration/regeneration > + * @pufdataaddr: address to store/get the puf data during registration/regeneration > + * @pufidaddr: puf id will be stored either during registration/regeneration > + */ > +struct puf_usrparams { > + __u8 pufoperation; > + __u8 globalvarfilter; > + __u8 readoption; > + __u8 reserved; > + __u32 shuttervalue; > + __u64 pufdataaddr; > + __u64 pufidaddr; > +}; > + > +/** > + * struct puf_helperdata - parameters for puf helper data. > + * @syndata: PUF syndrome data > + * @chash: PUF chash > + * @aux: PUF aux > + */ > +struct puf_helperdata { > + __u32 syndata[PUF_MAX_SYNDROME_DATA_LEN_IN_WORDS]; > + __u32 chash; > + __u32 aux; > +}; > + > +/** > + * struct pufdata - parameters for puf data. > + * @pufhd: puf helper data of type struct puf_helperdata > + * @pufid: PUF id > + * @efusesyndata: PUF efuse syndrome data > + */ > +struct pufdata { > + struct puf_helperdata pufhd; > + __u32 pufid[PUF_ID_LEN_IN_WORDS]; > + __u32 efusesyndata[PUF_EFUSE_TRIM_SYN_DATA_IN_WORDS]; > +}; > + > +enum pufreadoption { > + PUF_READ_FROM_RAM = 0, > + PUF_READ_FROM_EFUSE_CACHE = 1 > +}; > + > +#define PUF_IOC_MAGIC 'P' Why are you allowed to use "P" for your ioctl? Isn't that already used by other drivers? thanks, greg k-h