Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

On Thu, Dec 19, 2024 at 09:47:12AM +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > Sent: Tuesday, December 10, 2024 2:34 PM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> > linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> > FPGA programming
> > 
> > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> > > Hi Yilun,
> > >
> > > > -----Original Message-----
> > > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > > Sent: Wednesday, November 27, 2024 7:20 AM
> > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org;
> > > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> > > > robh@kernel.org; saravanak@google.com; linux-kernel@vger.kernel.org;
> > > > linux-fpga@vger.kernel.org; devicetree@vger.kernel.org
> > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface
> > > > for runtime FPGA programming
> > > >
> > > > > > > + * struct fpga_region_ops - ops for low level FPGA region ops
> > > > > > > +for device
> > > > > > > + * enumeration/removal
> > > > > > > + * @region_status: returns the FPGA region status
> > > > > > > + * @region_config_enumeration: Configure and enumerate the FPGA
> > region.
> > > > > > > + * @region_remove: Remove all devices within the FPGA region
> > > > > > > + * (which are added as part of the enumeration).
> > > > > > > + */
> > > > > > > +struct fpga_region_ops {
> > > > > > > +	int (*region_status)(struct fpga_region *region);
> > > > > > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > > > > > +					 struct fpga_region_config_info
> > *config_info);
> > > > > >
> > > > > > My current concern is still about this combined API, it just
> > > > > > offloads all work to low level, but we have some common flows.
> > > > > > That's why we introduce a common FPGA reprograming API.
> > > > > >
> > > > > > I didn't see issue about the vendor specific pre configuration.
> > > > > > They are generally needed to initialize the struct
> > > > > > fpga_image_info, which is a common structure for
> > fpga_region_program_fpga().
> > > > > >
> > > > > > For port IDs(AFU) inputs for DFL, I think it could also be
> > > > > > changed (Don't have to be implemented in this patchset).
> > > > > > Previously DFL provides an uAPI for the whole device, so it
> > > > > > needs a port_id input to position which fpga_region within the
> > > > > > device for programming. But now, we are introducing a per
> > > > > > fpga_region programming interface, IIUC port_id
> > > > should not be needed anymore.
> > > > > >
> > > > > > The combined API is truly simple for leveraging the existing
> > > > > > of-fpga-region overlay apply mechanism. But IMHO that flow
> > > > > > doesn't fit our new uAPI well. That flow is to adapt the generic
> > > > > > configfs overlay interface, which comes to a dead end as you mentioned.
> > > > > >
> > > > > > My gut feeling for the generic programing flow should be:
> > > > > >
> > > > > >  1. Program the image to HW.
> > > > > >  2. Enumerate the programmed image (apply the DT overlay)
> > > > > >
> > > > > > Why we have to:
> > > > > >
> > > > > >  1. Start enumeration.
> > > > > >  2. On pre enumeration, programe the image.
> > > > > >  3. Real enumeration.
> > > > > >
> > > > >
> > > > > I agree with the approach of leveraging vendor-specific callbacks
> > > > > to handle the distinct phases of the FPGA programming process.
> > > > > Here's the proposed flow.
> > > > >
> > > > > Pre-Configuration:
> > > > > A vendor-specific callback extracts the required pre-configuration
> > > > > details and initializes struct fpga_image_info. This ensures that
> > > > > all vendor-specific
> > > >
> > > > Since we need to construct the fpga_image_info, initialize multiple
> > > > field as needed, I'm wondering if configfs could be a solution for the uAPI?
> > > >
> > >
> > > A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL
> > flow.
> > > The POC code looks as follows.
> > 
> > I prefer more to configfs cause it provides standard FS way to create the
> > fpga_image_info object, e.g. which attributes are visible for OF/non-OF region, which
> > attributes come from image blob and can only be RO, etc.
> > 
> > Of couse ioctl() could achieve the same goal but would add much more specific rules
> > (maybe flags/types) for user to follow.
> > 
> 
> Agreed. Using ConfigFS is preferable because it provides a standardized filesystem
> interface for creating and managing the fpga_image_info object.
> 
> The proposed new user interface is outlined as follows:
> 
> # Mount ConfigFS filesystem
> mount -t configfs none /sys/kernel/config
> 
> # Upload Configuration and Load the Bitstream for the Targeted FPGA Region.
> 
> Configuration File Upload:
> Upload the configuration file containing the necessary metadata or settings required
> for configuring the FPGA region. This file may vary based on the vendor and includes
> important details specific to the vendor's requirements.
> 
> Vendor-Specific Callback: 
> A vendor-specific callback function extracts the relevant configuration data from the file.
> The format and contents of the configuration file can differ between vendors. The callback
> then initializes the struct fpga_image_info, ensuring all vendor-specific requirements are
> satisfied.
> 
> Device-Specific Considerations:
> For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config files.
> These .dtbo files contain all necessary information to populate the fpga_image_info.
> For non-OF devices, a vendor specific fpga.config files are used to provide the required
> data for initializing the fpga_image_info.

non-OF fpga images usually don't contain fpga_image_info data (e.g.
enable/disable_timeout_us). I think we don't have to force users embed
these data in fpga image, provide additional configfs attributes to
input these data is possible. For some FPGA regions (e.g. OF), these
attributes could be RO, some could be RW, depends on different FPGA
region drivers.

So I think we may have a Configuration File Upload interface, like:

  echo "config_file" > /sys/kernel/config/fpga/<region>/image

Some additional parameter interfaces, like:

  echo 10000 > /sys/kernel/config/fpga/<region>/enable_timeout
  ...

And a Configuration interface, like:

  # programming
  echo 1 > /sys/kernel/config/fpga/<region>/config
  # removing
  echo 0 > /sys/kernel/config/fpga/<region>/config

How do you think?

Thanks,
Yilun

> 
> FPGA Configuration:
> Once the configuration details are extracted and the fpga_image_info structure is initialized,
> the FPGA can be programmed accordingly.
> 
> echo "config_file" > /sys/kernel/config/fpga/<region>/config
> 
> 
> # Check the status of "region"
> cat /sys/kernel/config/fpga/<region>/status
> 
> # Remove "region"
> echo "remove" > /sys/kernel/config/fpga/<region>/remove
> 
> Looking forward to your feedback.
> 
> Regards,
> Navakishore.
> 

[RFC v2 0/1]Add user space interaction for FPGA programming

[Nava kishore Manne]

The existing FPGA manager subsystem didn't have any user space interface
(other than the status/state in sysfs) in Kernel. Basically, FPGAs are
semiconductor devices that can be reprogrammed for desired hardware
functionality.

FPGAs can be reprogrammed at runtime with different types of logic and IPs
as per user need and hence there is a need to use device tree overlays for
removing/updating/adding the devices at runtime for the IPs/controllers
that are present in FPGA. But we don't have any user interface in kernel
for updating the device tree at runtime.

Sometime back there was a series sent by Pantelis Antoniou
(https://lore.kernel.org/lkml/1414528565-10907-4-git-send-email-pantelis.antoniou@konsulko.com/).

This patch introduced a user interface configfs for Device Tree overlays,
a method of dynamically altering the kernel's live Device Tree. However,
this patch series was not accepted in mainline due to various concerns.
For more details refer to this link:
https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts#issues_and_what_needs_to_be_completed_--_Not_an_exhaustive_list

One of the major valid concerns that were raised with this configfs
interface was security. It provides a generic interface (Not specific
to the use cases) for modifying the live device tree.

In order to configure/program the FPGA devices, All the major vendors of
FPGA are using this configfs series as out-of-tree patch for configuring
the FPGAs and there was never an attempt to introduce a generic interface
to configure/program the FPGA in upstream and hence upstream kernel ended
up in not having proper support for FPGAs configure/program.
This series tries to address this gap of FPGA programmability by providing
a new IOCTL interface to the user.

The newly introduced IOCTL interface offers a generic and standardized
mechanism for configuring or reprogramming FPGAs at runtime. It supports
both Open Firmware (OF) and non-OF devices, utilizing vendor-specific
callbacks—such as configuration+enumeration, removal, and status checks.
To accommodate diverse device-specific configurations.

This solution enhances FPGA runtime management, supporting various device
types and vendors, while ensuring compatibility with the current FPGA
configuration flow.

Nava kishore Manne (1):
  fpga-region: Add generic IOCTL interface for runtime FPGA programming

 drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
 drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
 include/linux/fpga/fpga-region.h |  32 +++++++++
 include/uapi/linux/fpga-region.h |  51 ++++++++++++++
 4 files changed, 283 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/fpga-region.h

-- 
2.34.1

[RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Nava kishore Manne]

Introduces an IOCTL interface within the fpga-region subsystem,
providing a generic and standardized mechanism for configuring (or)
reprogramming FPGAs during runtime. The newly added interface supports
both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
callbacks (e.g., configuration + enumeration, removal, and status) to
accommodate a wide range of device specific configurations.

The IOCTL interface ensures compatibility with both OF and non-OF
devices, allowing for seamless FPGA reprogramming across diverse
platforms.

Vendor-specific callbacks are integrated into the interface, enabling
custom FPGA configuration + enumeration, removal, and status reporting
mechanisms, ensuring flexibility for vendor implementations.

This solution enhances FPGA runtime management, supporting various device
types and vendors, while ensuring compatibility with the current FPGA
configuration flow.

Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
---
Changes for v2:
 - As discussed with Yilun, the implementation has been modified to utilize a
 callback approach, enabling seamless handling of both OF and non-OF devices.

 - As suggested by Yilun in the POC code, we have moved away from using  void *args
 as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
 utilizing the fpga_region_config_info structure to gather user inputs. Currently,
 this structure is implemented to support only OF devices, but we intend to extend
 it by incorporating new members to accommodate non-OF devices in the future.

 drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
 drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
 include/linux/fpga/fpga-region.h |  32 +++++++++
 include/uapi/linux/fpga-region.h |  51 ++++++++++++++
 4 files changed, 283 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/fpga-region.h

diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
index 753cd142503e..c6bea3c99a69 100644
--- a/drivers/fpga/fpga-region.c
+++ b/drivers/fpga/fpga-region.c
@@ -8,6 +8,7 @@
 #include <linux/fpga/fpga-bridge.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-region.h>
+#include <linux/fpga-region.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
 };
 ATTRIBUTE_GROUPS(fpga_region);
 
+static int fpga_region_device_open(struct inode *inode, struct file *file)
+{
+	struct miscdevice *miscdev = file->private_data;
+	struct fpga_region *region = container_of(miscdev, struct fpga_region, miscdev);
+
+	file->private_data = region;
+
+	return 0;
+}
+
+static int fpga_region_device_release(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
+				     unsigned long arg)
+{
+	int err;
+	void __user *argp = (void __user *)arg;
+	struct fpga_region_config_info config_info;
+	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
+
+	switch (cmd) {
+	case FPGA_REGION_IOCTL_LOAD:
+		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
+			return -EFAULT;
+
+		err = region->region_ops->region_config_enumeration(region, &config_info);
+
+		break;
+	case FPGA_REGION_IOCTL_REMOVE:
+		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
+			return -EFAULT;
+
+		err = region->region_ops->region_remove(region, &config_info);
+
+		break;
+	case FPGA_REGION_IOCTL_STATUS:
+		unsigned int status;
+
+		status = region->region_ops->region_status(region);
+
+		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
+			err = -EFAULT;
+		break;
+	default:
+		err = -ENOTTY;
+	}
+
+	return err;
+}
+
+static const struct file_operations fpga_region_fops = {
+	.owner		= THIS_MODULE,
+	.open		= fpga_region_device_open,
+	.release	= fpga_region_device_release,
+	.unlocked_ioctl	= fpga_region_device_ioctl,
+	.compat_ioctl	= fpga_region_device_ioctl,
+};
+
 /**
  * __fpga_region_register_full - create and register an FPGA Region device
  * @parent: device parent
@@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
 	if (ret)
 		goto err_remove;
 
+	if (info->region_ops) {
+		region->region_ops = info->region_ops;
+		region->miscdev.minor = MISC_DYNAMIC_MINOR;
+		region->miscdev.name = kobject_name(&region->dev.kobj);
+		region->miscdev.fops = &fpga_region_fops;
+		ret = misc_register(&region->miscdev);
+		if (ret) {
+			pr_err("fpga-region: failed to register misc device.\n");
+			goto err_remove;
+		}
+	}
+
 	ret = device_register(&region->dev);
 	if (ret) {
+		misc_deregister(&region->miscdev);
 		put_device(&region->dev);
 		return ERR_PTR(ret);
 	}
@@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
 }
 EXPORT_SYMBOL_GPL(__fpga_region_register);
 
+/**
+ * __fpga_region_register_with_ops - create and register an FPGA Region device
+ * with user interface call-backs.
+ * @parent: device parent
+ * @mgr: manager that programs this region
+ * @region_ops: ops for low level FPGA region for device enumeration/removal
+ * @priv: of-fpga-region private data
+ * @get_bridges: optional function to get bridges to a list
+ * @owner: module containing the get_bridges function
+ *
+ * This simple version of the register function should be sufficient for most users.
+ * The fpga_region_register_full() function is available for users that need to
+ * pass additional, optional parameters.
+ *
+ * Return: struct fpga_region or ERR_PTR()
+ */
+struct fpga_region *
+__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
+				const struct fpga_region_ops *region_ops,
+				void *priv,
+				int (*get_bridges)(struct fpga_region *),
+				struct module *owner)
+{
+	struct fpga_region_info info = { 0 };
+
+	info.mgr = mgr;
+	info.priv = priv;
+	info.get_bridges = get_bridges;
+	info.region_ops = region_ops;
+
+	return __fpga_region_register_full(parent, &info, owner);
+}
+EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
+
 /**
  * fpga_region_unregister - unregister an FPGA region
  * @region: FPGA region
@@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
  */
 void fpga_region_unregister(struct fpga_region *region)
 {
+	misc_deregister(&region->miscdev);
 	device_unregister(&region->dev);
 }
 EXPORT_SYMBOL_GPL(fpga_region_unregister);
diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
index 8526a5a86f0c..63fe56e0466f 100644
--- a/drivers/fpga/of-fpga-region.c
+++ b/drivers/fpga/of-fpga-region.c
@@ -8,6 +8,8 @@
 #include <linux/fpga/fpga-bridge.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-region.h>
+#include <linux/firmware.h>
+#include <linux/fpga-region.h>
 #include <linux/idr.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -18,6 +20,20 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 
+/**
+ * struct of_fpga_region_priv - Private data structure
+ * image.
+ * @dev:	Device data structure
+ * @fw:		firmware of coeff table.
+ * @path:	path of FPGA overlay image firmware file.
+ * @ovcs_id:	overlay changeset id.
+ */
+struct of_fpga_region_priv {
+	struct device *dev;
+	const struct firmware *fw;
+	int ovcs_id;
+};
+
 static const struct of_device_id fpga_region_of_match[] = {
 	{ .compatible = "fpga-region", },
 	{},
@@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
 	.notifier_call = of_fpga_region_notify,
 };
 
+static int of_fpga_region_status(struct fpga_region *region)
+{
+	struct of_fpga_region_priv *ovcs = region->priv;
+
+	if (ovcs->ovcs_id)
+		return FPGA_REGION_HAS_PL;
+
+	return FPGA_REGION_EMPTY;
+}
+
+static int of_fpga_region_config_enumeration(struct fpga_region *region,
+					     struct fpga_region_config_info *config_info)
+{
+	struct of_fpga_region_priv *ovcs = region->priv;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (ovcs->ovcs_id)
+		return -EPERM;
+
+	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
+	if (err != 0)
+		goto out_err;
+
+	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
+				   &ovcs->ovcs_id, NULL);
+	if (err < 0) {
+		pr_err("%s: Failed to create overlay (err=%d)\n",
+		       __func__, err);
+		release_firmware(ovcs->fw);
+		goto out_err;
+	}
+
+	return 0;
+
+out_err:
+	ovcs->ovcs_id = 0;
+	ovcs->fw = NULL;
+
+	return err;
+}
+
+static int of_fpga_region_config_remove(struct fpga_region *region,
+					struct fpga_region_config_info *config_info)
+{
+	struct of_fpga_region_priv *ovcs = region->priv;
+
+	if (!ovcs->ovcs_id)
+		return -EPERM;
+
+	of_overlay_remove(&ovcs->ovcs_id);
+	release_firmware(ovcs->fw);
+
+	ovcs->ovcs_id = 0;
+	ovcs->fw = NULL;
+
+	return 0;
+}
+
+static const struct fpga_region_ops region_ops = {
+	.region_status = of_fpga_region_status,
+	.region_config_enumeration = of_fpga_region_config_enumeration,
+	.region_remove = of_fpga_region_config_remove,
+};
+
 static int of_fpga_region_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
+	struct of_fpga_region_priv *priv;
 	struct fpga_region *region;
 	struct fpga_manager *mgr;
 	int ret;
 
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+
 	/* Find the FPGA mgr specified by region or parent region. */
 	mgr = of_fpga_region_get_mgr(np);
 	if (IS_ERR(mgr))
 		return -EPROBE_DEFER;
 
-	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
+	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
+					       of_fpga_region_get_bridges);
 	if (IS_ERR(region)) {
 		ret = PTR_ERR(region);
 		goto eprobe_mgr_put;
diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
index 5fbc05fe70a6..3a3ba6dbb5e1 100644
--- a/include/linux/fpga/fpga-region.h
+++ b/include/linux/fpga/fpga-region.h
@@ -6,15 +6,35 @@
 #include <linux/device.h>
 #include <linux/fpga/fpga-mgr.h>
 #include <linux/fpga/fpga-bridge.h>
+#include <linux/fpga-region.h>
+#include <linux/miscdevice.h>
 
 struct fpga_region;
 
+/**
+ * struct fpga_region_ops - ops for low level FPGA region ops for device
+ * enumeration/removal
+ * @region_status: returns the FPGA region status
+ * @region_config_enumeration: Configure and enumerate the FPGA region.
+ * @region_remove: Remove all devices within the FPGA region
+ * (which are added as part of the enumeration).
+ */
+struct fpga_region_ops {
+	int (*region_status)(struct fpga_region *region);
+	int (*region_config_enumeration)(struct fpga_region *region,
+					 struct fpga_region_config_info *config_info);
+	int (*region_remove)(struct fpga_region *region,
+			     struct fpga_region_config_info *config_info);
+};
+
 /**
  * struct fpga_region_info - collection of parameters an FPGA Region
  * @mgr: fpga region manager
  * @compat_id: FPGA region id for compatibility check.
  * @priv: fpga region private data
  * @get_bridges: optional function to get bridges to a list
+ * @fpga_region_ops: ops for low level FPGA region ops for device
+ * enumeration/removal
  *
  * fpga_region_info contains parameters for the register_full function.
  * These are separated into an info structure because they some are optional
@@ -26,6 +46,7 @@ struct fpga_region_info {
 	struct fpga_compat_id *compat_id;
 	void *priv;
 	int (*get_bridges)(struct fpga_region *region);
+	const struct fpga_region_ops *region_ops;
 };
 
 /**
@@ -39,6 +60,8 @@ struct fpga_region_info {
  * @ops_owner: module containing the get_bridges function
  * @priv: private data
  * @get_bridges: optional function to get bridges to a list
+ * @fpga_region_ops: ops for low level FPGA region ops for device
+ * enumeration/removal
  */
 struct fpga_region {
 	struct device dev;
@@ -50,6 +73,8 @@ struct fpga_region {
 	struct module *ops_owner;
 	void *priv;
 	int (*get_bridges)(struct fpga_region *region);
+	const struct fpga_region_ops *region_ops;
+	struct miscdevice miscdev;
 };
 
 #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
@@ -71,6 +96,13 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
 struct fpga_region *
 __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
 		       int (*get_bridges)(struct fpga_region *), struct module *owner);
+#define fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges) \
+	__fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges, THIS_MODULE)
+struct fpga_region *
+__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
+				const struct fpga_region_ops *region_ops, void *priv,
+				int (*get_bridges)(struct fpga_region *),
+				struct module *owner);
 void fpga_region_unregister(struct fpga_region *region);
 
 #endif /* _FPGA_REGION_H */
diff --git a/include/uapi/linux/fpga-region.h b/include/uapi/linux/fpga-region.h
new file mode 100644
index 000000000000..88ade83daf61
--- /dev/null
+++ b/include/uapi/linux/fpga-region.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+/*
+ * Header File for FPGA Region User API
+ *
+ * Copyright (C) 2024 Advanced Micro Devices, Inc.
+ *
+ * Author: Manne, Nava kishore <nava.kishore.manne@amd.com>
+ */
+
+#ifndef _UAPI_LINUX_FPGA_REGION_H
+#define _UAPI_LINUX_FPGA_REGION_H
+
+#include <linux/ioctl.h>
+#include <linux/limits.h>
+#include <linux/types.h>
+
+/* IOCTLs for fpga region file descriptor */
+#define FPGA_REGION_MAGIC_NUMBER	'f'
+#define FPGA_REGION_BASE		0
+
+/**
+ * FPGA_REGION_IOCTL_LOAD - _IOW(FPGA_REGION_MAGIC, 0,
+ *                               struct fpga_region_config_info)
+ *
+ * FPGA_REGION_IOCTL_REMOVE - _IOW(FPGA_REGION_MAGIC, 1,
+ *                                 struct fpga_region_config_info)
+ *
+ * Driver does Configuration/Reconfiguration based on Region ID and
+ * Buffer (Image) provided by caller.
+ * Return: 0 on success, -errno on failure.
+ */
+struct fpga_region_config_info {	/* Input */
+	char firmware_name[NAME_MAX];   /* Firmware file name */
+};
+
+/*
+ * FPGA Region Control IOCTLs.
+ */
+#define FPGA_REGION_MAGIC	'f'
+#define FPGA_IOW(num, dtype)	_IOW(FPGA_REGION_MAGIC, num, dtype)
+#define FPGA_IOR(num, dtype)	_IOR(FPGA_REGION_MAGIC, num, dtype)
+
+#define FPGA_REGION_IOCTL_LOAD		FPGA_IOW(0, __u32)
+#define FPGA_REGION_IOCTL_REMOVE        FPGA_IOW(1, __u32)
+#define FPGA_REGION_IOCTL_STATUS        FPGA_IOR(2, __u32)
+
+/* Region status possibilities returned by FPGA_REGION_IOCTL_STATUS ioctl */
+#define FPGA_REGION_HAS_PL	0	/* if the region has PL logic */
+#define FPGA_REGION_EMPTY	1	/* If the region is empty */
+
+#endif /* _UAPI_LINUX_FPGA_REGION_H */
-- 
2.34.1

RE: [RFC v2 0/1]Add user space interaction for FPGA programming

[Manne, Nava kishore]

Ping!

> -----Original Message-----
> From: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Sent: Tuesday, October 29, 2024 2:48 PM
> To: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> Manne, Nava kishore <nava.kishore.manne@amd.com>; linux-
> kernel@vger.kernel.org; linux-fpga@vger.kernel.org; devicetree@vger.kernel.org
> Subject: [RFC v2 0/1]Add user space interaction for FPGA programming
> 
> The existing FPGA manager subsystem didn't have any user space interface (other
> than the status/state in sysfs) in Kernel. Basically, FPGAs are semiconductor
> devices that can be reprogrammed for desired hardware functionality.
> 
> FPGAs can be reprogrammed at runtime with different types of logic and IPs as per
> user need and hence there is a need to use device tree overlays for
> removing/updating/adding the devices at runtime for the IPs/controllers that are
> present in FPGA. But we don't have any user interface in kernel for updating the
> device tree at runtime.
> 
> Sometime back there was a series sent by Pantelis Antoniou
> (https://lore.kernel.org/lkml/1414528565-10907-4-git-send-email-
> pantelis.antoniou@konsulko.com/).
> 
> This patch introduced a user interface configfs for Device Tree overlays, a method of
> dynamically altering the kernel's live Device Tree. However, this patch series was
> not accepted in mainline due to various concerns.
> For more details refer to this link:
> https://elinux.org/Frank%27s_Evolving_Overlay_Thoughts#issues_and_what_needs
> _to_be_completed_--_Not_an_exhaustive_list
> 
> One of the major valid concerns that were raised with this configfs interface was
> security. It provides a generic interface (Not specific to the use cases) for modifying
> the live device tree.
> 
> In order to configure/program the FPGA devices, All the major vendors of FPGA are
> using this configfs series as out-of-tree patch for configuring the FPGAs and there
> was never an attempt to introduce a generic interface to configure/program the
> FPGA in upstream and hence upstream kernel ended up in not having proper
> support for FPGAs configure/program.
> This series tries to address this gap of FPGA programmability by providing a new
> IOCTL interface to the user.
> 
> The newly introduced IOCTL interface offers a generic and standardized mechanism
> for configuring or reprogramming FPGAs at runtime. It supports both Open Firmware
> (OF) and non-OF devices, utilizing vendor-specific callbacks—such as
> configuration+enumeration, removal, and status checks.
> To accommodate diverse device-specific configurations.
> 
> This solution enhances FPGA runtime management, supporting various device types
> and vendors, while ensuring compatibility with the current FPGA configuration flow.
> 
> Nava kishore Manne (1):
>   fpga-region: Add generic IOCTL interface for runtime FPGA programming
> 
>  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
>  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
>  include/linux/fpga/fpga-region.h |  32 +++++++++  include/uapi/linux/fpga-region.h |
> 51 ++++++++++++++
>  4 files changed, 283 insertions(+), 1 deletion(-)  create mode 100644
> include/uapi/linux/fpga-region.h
> 
> --
> 2.34.1

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote:
> Introduces an IOCTL interface within the fpga-region subsystem,
> providing a generic and standardized mechanism for configuring (or)
> reprogramming FPGAs during runtime. The newly added interface supports
> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> callbacks (e.g., configuration + enumeration, removal, and status) to
> accommodate a wide range of device specific configurations.
> 
> The IOCTL interface ensures compatibility with both OF and non-OF
> devices, allowing for seamless FPGA reprogramming across diverse
> platforms.
> 
> Vendor-specific callbacks are integrated into the interface, enabling
> custom FPGA configuration + enumeration, removal, and status reporting
> mechanisms, ensuring flexibility for vendor implementations.
> 
> This solution enhances FPGA runtime management, supporting various device
> types and vendors, while ensuring compatibility with the current FPGA
> configuration flow.
> 
> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> ---
> Changes for v2:
>  - As discussed with Yilun, the implementation has been modified to utilize a
>  callback approach, enabling seamless handling of both OF and non-OF devices.
> 
>  - As suggested by Yilun in the POC code, we have moved away from using  void *args
>  as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
>  utilizing the fpga_region_config_info structure to gather user inputs. Currently,
>  this structure is implemented to support only OF devices, but we intend to extend
>  it by incorporating new members to accommodate non-OF devices in the future.
> 
>  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
>  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
>  include/linux/fpga/fpga-region.h |  32 +++++++++
>  include/uapi/linux/fpga-region.h |  51 ++++++++++++++
>  4 files changed, 283 insertions(+), 1 deletion(-)
>  create mode 100644 include/uapi/linux/fpga-region.h
> 
> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> index 753cd142503e..c6bea3c99a69 100644
> --- a/drivers/fpga/fpga-region.c
> +++ b/drivers/fpga/fpga-region.c
> @@ -8,6 +8,7 @@
>  #include <linux/fpga/fpga-bridge.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/fpga/fpga-region.h>
> +#include <linux/fpga-region.h>
>  #include <linux/idr.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
>  };
>  ATTRIBUTE_GROUPS(fpga_region);
>  
> +static int fpga_region_device_open(struct inode *inode, struct file *file)
> +{
> +	struct miscdevice *miscdev = file->private_data;
> +	struct fpga_region *region = container_of(miscdev, struct fpga_region, miscdev);
> +
> +	file->private_data = region;
> +
> +	return 0;
> +}
> +
> +static int fpga_region_device_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}
> +
> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
> +				     unsigned long arg)
> +{
> +	int err;
> +	void __user *argp = (void __user *)arg;
> +	struct fpga_region_config_info config_info;
> +	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
> +
> +	switch (cmd) {
> +	case FPGA_REGION_IOCTL_LOAD:
> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
> +			return -EFAULT;
> +
> +		err = region->region_ops->region_config_enumeration(region, &config_info);
> +
> +		break;
> +	case FPGA_REGION_IOCTL_REMOVE:
> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
> +			return -EFAULT;
> +
> +		err = region->region_ops->region_remove(region, &config_info);
> +
> +		break;
> +	case FPGA_REGION_IOCTL_STATUS:
> +		unsigned int status;
> +
> +		status = region->region_ops->region_status(region);
> +
> +		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
> +			err = -EFAULT;
> +		break;
> +	default:
> +		err = -ENOTTY;
> +	}
> +
> +	return err;
> +}
> +
> +static const struct file_operations fpga_region_fops = {
> +	.owner		= THIS_MODULE,
> +	.open		= fpga_region_device_open,
> +	.release	= fpga_region_device_release,
> +	.unlocked_ioctl	= fpga_region_device_ioctl,
> +	.compat_ioctl	= fpga_region_device_ioctl,
> +};
> +
>  /**
>   * __fpga_region_register_full - create and register an FPGA Region device
>   * @parent: device parent
> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
>  	if (ret)
>  		goto err_remove;
>  
> +	if (info->region_ops) {
> +		region->region_ops = info->region_ops;
> +		region->miscdev.minor = MISC_DYNAMIC_MINOR;
> +		region->miscdev.name = kobject_name(&region->dev.kobj);
> +		region->miscdev.fops = &fpga_region_fops;
> +		ret = misc_register(&region->miscdev);
> +		if (ret) {
> +			pr_err("fpga-region: failed to register misc device.\n");
> +			goto err_remove;
> +		}
> +	}
> +
>  	ret = device_register(&region->dev);
>  	if (ret) {
> +		misc_deregister(&region->miscdev);
>  		put_device(&region->dev);
>  		return ERR_PTR(ret);
>  	}
> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>  }
>  EXPORT_SYMBOL_GPL(__fpga_region_register);
>  
> +/**
> + * __fpga_region_register_with_ops - create and register an FPGA Region device
> + * with user interface call-backs.
> + * @parent: device parent
> + * @mgr: manager that programs this region
> + * @region_ops: ops for low level FPGA region for device enumeration/removal
> + * @priv: of-fpga-region private data
> + * @get_bridges: optional function to get bridges to a list
> + * @owner: module containing the get_bridges function
> + *
> + * This simple version of the register function should be sufficient for most users.
> + * The fpga_region_register_full() function is available for users that need to
> + * pass additional, optional parameters.
> + *
> + * Return: struct fpga_region or ERR_PTR()
> + */
> +struct fpga_region *
> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
> +				const struct fpga_region_ops *region_ops,
> +				void *priv,
> +				int (*get_bridges)(struct fpga_region *),
> +				struct module *owner)
> +{
> +	struct fpga_region_info info = { 0 };
> +
> +	info.mgr = mgr;
> +	info.priv = priv;
> +	info.get_bridges = get_bridges;
> +	info.region_ops = region_ops;
> +
> +	return __fpga_region_register_full(parent, &info, owner);
> +}
> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
> +
>  /**
>   * fpga_region_unregister - unregister an FPGA region
>   * @region: FPGA region
> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
>   */
>  void fpga_region_unregister(struct fpga_region *region)
>  {
> +	misc_deregister(&region->miscdev);
>  	device_unregister(&region->dev);
>  }
>  EXPORT_SYMBOL_GPL(fpga_region_unregister);
> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> index 8526a5a86f0c..63fe56e0466f 100644
> --- a/drivers/fpga/of-fpga-region.c
> +++ b/drivers/fpga/of-fpga-region.c
> @@ -8,6 +8,8 @@
>  #include <linux/fpga/fpga-bridge.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/fpga/fpga-region.h>
> +#include <linux/firmware.h>
> +#include <linux/fpga-region.h>
>  #include <linux/idr.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -18,6 +20,20 @@
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  
> +/**
> + * struct of_fpga_region_priv - Private data structure
> + * image.
> + * @dev:	Device data structure
> + * @fw:		firmware of coeff table.
> + * @path:	path of FPGA overlay image firmware file.
> + * @ovcs_id:	overlay changeset id.
> + */
> +struct of_fpga_region_priv {
> +	struct device *dev;
> +	const struct firmware *fw;
> +	int ovcs_id;
> +};
> +
>  static const struct of_device_id fpga_region_of_match[] = {
>  	{ .compatible = "fpga-region", },
>  	{},
> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
>  	.notifier_call = of_fpga_region_notify,
>  };
>  
> +static int of_fpga_region_status(struct fpga_region *region)
> +{
> +	struct of_fpga_region_priv *ovcs = region->priv;
> +
> +	if (ovcs->ovcs_id)
> +		return FPGA_REGION_HAS_PL;

Could you help specify what is PL?

> +
> +	return FPGA_REGION_EMPTY;
> +}
> +
> +static int of_fpga_region_config_enumeration(struct fpga_region *region,
> +					     struct fpga_region_config_info *config_info)
> +{
> +	struct of_fpga_region_priv *ovcs = region->priv;
> +	int err;
> +
> +	/* if it's set do not allow changes */
> +	if (ovcs->ovcs_id)
> +		return -EPERM;
> +
> +	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
> +	if (err != 0)
> +		goto out_err;
> +
> +	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
> +				   &ovcs->ovcs_id, NULL);
> +	if (err < 0) {
> +		pr_err("%s: Failed to create overlay (err=%d)\n",
> +		       __func__, err);
> +		release_firmware(ovcs->fw);
> +		goto out_err;
> +	}
> +
> +	return 0;
> +
> +out_err:
> +	ovcs->ovcs_id = 0;
> +	ovcs->fw = NULL;
> +
> +	return err;
> +}
> +
> +static int of_fpga_region_config_remove(struct fpga_region *region,
> +					struct fpga_region_config_info *config_info)
> +{
> +	struct of_fpga_region_priv *ovcs = region->priv;
> +
> +	if (!ovcs->ovcs_id)
> +		return -EPERM;
> +
> +	of_overlay_remove(&ovcs->ovcs_id);
> +	release_firmware(ovcs->fw);
> +
> +	ovcs->ovcs_id = 0;
> +	ovcs->fw = NULL;
> +
> +	return 0;
> +}
> +
> +static const struct fpga_region_ops region_ops = {
> +	.region_status = of_fpga_region_status,
> +	.region_config_enumeration = of_fpga_region_config_enumeration,
> +	.region_remove = of_fpga_region_config_remove,
> +};
> +
>  static int of_fpga_region_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
> +	struct of_fpga_region_priv *priv;
>  	struct fpga_region *region;
>  	struct fpga_manager *mgr;
>  	int ret;
>  
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +
>  	/* Find the FPGA mgr specified by region or parent region. */
>  	mgr = of_fpga_region_get_mgr(np);
>  	if (IS_ERR(mgr))
>  		return -EPROBE_DEFER;
>  
> -	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
> +	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
> +					       of_fpga_region_get_bridges);
>  	if (IS_ERR(region)) {
>  		ret = PTR_ERR(region);
>  		goto eprobe_mgr_put;
> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> index 5fbc05fe70a6..3a3ba6dbb5e1 100644
> --- a/include/linux/fpga/fpga-region.h
> +++ b/include/linux/fpga/fpga-region.h
> @@ -6,15 +6,35 @@
>  #include <linux/device.h>
>  #include <linux/fpga/fpga-mgr.h>
>  #include <linux/fpga/fpga-bridge.h>
> +#include <linux/fpga-region.h>
> +#include <linux/miscdevice.h>
>  
>  struct fpga_region;
>  
> +/**
> + * struct fpga_region_ops - ops for low level FPGA region ops for device
> + * enumeration/removal
> + * @region_status: returns the FPGA region status
> + * @region_config_enumeration: Configure and enumerate the FPGA region.
> + * @region_remove: Remove all devices within the FPGA region
> + * (which are added as part of the enumeration).
> + */
> +struct fpga_region_ops {
> +	int (*region_status)(struct fpga_region *region);
> +	int (*region_config_enumeration)(struct fpga_region *region,
> +					 struct fpga_region_config_info *config_info);

My current concern is still about this combined API, it just offloads
all work to low level, but we have some common flows. That's why we
introduce a common FPGA reprograming API.

I didn't see issue about the vendor specific pre configuration. They
are generally needed to initialize the struct fpga_image_info, which
is a common structure for fpga_region_program_fpga().

For port IDs(AFU) inputs for DFL, I think it could also be changed
(Don't have to be implemented in this patchset). Previously DFL
provides an uAPI for the whole device, so it needs a port_id input to
position which fpga_region within the device for programming. But now,
we are introducing a per fpga_region programming interface, IIUC port_id
should not be needed anymore.

The combined API is truly simple for leveraging the existing
of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit
our new uAPI well. That flow is to adapt the generic configfs overlay
interface, which comes to a dead end as you mentioned.

My gut feeling for the generic programing flow should be:

 1. Program the image to HW.
 2. Enumerate the programmed image (apply the DT overlay)

Why we have to:

 1. Start enumeration.
 2. On pre enumeration, programe the image.
 3. Real enumeration.

Thanks,
Yilun

> +	int (*region_remove)(struct fpga_region *region,
> +			     struct fpga_region_config_info *config_info);
> +};
> +
>  /**
>   * struct fpga_region_info - collection of parameters an FPGA Region
>   * @mgr: fpga region manager
>   * @compat_id: FPGA region id for compatibility check.
>   * @priv: fpga region private data
>   * @get_bridges: optional function to get bridges to a list
> + * @fpga_region_ops: ops for low level FPGA region ops for device
> + * enumeration/removal
>   *
>   * fpga_region_info contains parameters for the register_full function.
>   * These are separated into an info structure because they some are optional
> @@ -26,6 +46,7 @@ struct fpga_region_info {
>  	struct fpga_compat_id *compat_id;
>  	void *priv;
>  	int (*get_bridges)(struct fpga_region *region);
> +	const struct fpga_region_ops *region_ops;
>  };
>  
>  /**
> @@ -39,6 +60,8 @@ struct fpga_region_info {
>   * @ops_owner: module containing the get_bridges function
>   * @priv: private data
>   * @get_bridges: optional function to get bridges to a list
> + * @fpga_region_ops: ops for low level FPGA region ops for device
> + * enumeration/removal
>   */
>  struct fpga_region {
>  	struct device dev;
> @@ -50,6 +73,8 @@ struct fpga_region {
>  	struct module *ops_owner;
>  	void *priv;
>  	int (*get_bridges)(struct fpga_region *region);
> +	const struct fpga_region_ops *region_ops;
> +	struct miscdevice miscdev;
>  };
>  
>  #define to_fpga_region(d) container_of(d, struct fpga_region, dev)
> @@ -71,6 +96,13 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
>  struct fpga_region *
>  __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>  		       int (*get_bridges)(struct fpga_region *), struct module *owner);
> +#define fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges) \
> +	__fpga_region_register_with_ops(parent, mgr, region_ops, priv, get_bridges, THIS_MODULE)
> +struct fpga_region *
> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
> +				const struct fpga_region_ops *region_ops, void *priv,
> +				int (*get_bridges)(struct fpga_region *),
> +				struct module *owner);
>  void fpga_region_unregister(struct fpga_region *region);
>  
>  #endif /* _FPGA_REGION_H */
> diff --git a/include/uapi/linux/fpga-region.h b/include/uapi/linux/fpga-region.h
> new file mode 100644
> index 000000000000..88ade83daf61
> --- /dev/null
> +++ b/include/uapi/linux/fpga-region.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Header File for FPGA Region User API
> + *
> + * Copyright (C) 2024 Advanced Micro Devices, Inc.
> + *
> + * Author: Manne, Nava kishore <nava.kishore.manne@amd.com>
> + */
> +
> +#ifndef _UAPI_LINUX_FPGA_REGION_H
> +#define _UAPI_LINUX_FPGA_REGION_H
> +
> +#include <linux/ioctl.h>
> +#include <linux/limits.h>
> +#include <linux/types.h>
> +
> +/* IOCTLs for fpga region file descriptor */
> +#define FPGA_REGION_MAGIC_NUMBER	'f'
> +#define FPGA_REGION_BASE		0
> +
> +/**
> + * FPGA_REGION_IOCTL_LOAD - _IOW(FPGA_REGION_MAGIC, 0,
> + *                               struct fpga_region_config_info)
> + *
> + * FPGA_REGION_IOCTL_REMOVE - _IOW(FPGA_REGION_MAGIC, 1,
> + *                                 struct fpga_region_config_info)
> + *
> + * Driver does Configuration/Reconfiguration based on Region ID and
> + * Buffer (Image) provided by caller.
> + * Return: 0 on success, -errno on failure.
> + */
> +struct fpga_region_config_info {	/* Input */
> +	char firmware_name[NAME_MAX];   /* Firmware file name */
> +};
> +
> +/*
> + * FPGA Region Control IOCTLs.
> + */
> +#define FPGA_REGION_MAGIC	'f'
> +#define FPGA_IOW(num, dtype)	_IOW(FPGA_REGION_MAGIC, num, dtype)
> +#define FPGA_IOR(num, dtype)	_IOR(FPGA_REGION_MAGIC, num, dtype)
> +
> +#define FPGA_REGION_IOCTL_LOAD		FPGA_IOW(0, __u32)
> +#define FPGA_REGION_IOCTL_REMOVE        FPGA_IOW(1, __u32)
> +#define FPGA_REGION_IOCTL_STATUS        FPGA_IOR(2, __u32)
> +
> +/* Region status possibilities returned by FPGA_REGION_IOCTL_STATUS ioctl */
> +#define FPGA_REGION_HAS_PL	0	/* if the region has PL logic */
> +#define FPGA_REGION_EMPTY	1	/* If the region is empty */
> +
> +#endif /* _UAPI_LINUX_FPGA_REGION_H */
> -- 
> 2.34.1
> 
> 

RE: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Manne, Nava kishore]

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Tuesday, November 19, 2024 9:45 AM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
> 
> On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote:
> > Introduces an IOCTL interface within the fpga-region subsystem,
> > providing a generic and standardized mechanism for configuring (or)
> > reprogramming FPGAs during runtime. The newly added interface supports
> > both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> > callbacks (e.g., configuration + enumeration, removal, and status) to
> > accommodate a wide range of device specific configurations.
> >
> > The IOCTL interface ensures compatibility with both OF and non-OF
> > devices, allowing for seamless FPGA reprogramming across diverse
> > platforms.
> >
> > Vendor-specific callbacks are integrated into the interface, enabling
> > custom FPGA configuration + enumeration, removal, and status reporting
> > mechanisms, ensuring flexibility for vendor implementations.
> >
> > This solution enhances FPGA runtime management, supporting various
> > device types and vendors, while ensuring compatibility with the
> > current FPGA configuration flow.
> >
> > Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> > ---
> > Changes for v2:
> >  - As discussed with Yilun, the implementation has been modified to
> > utilize a  callback approach, enabling seamless handling of both OF and non-OF
> devices.
> >
> >  - As suggested by Yilun in the POC code, we have moved away from
> > using  void *args  as a parameter for ICOTL inputs to obtain the
> > required user inputs. Instead, we are  utilizing the
> > fpga_region_config_info structure to gather user inputs. Currently,
> > this structure is implemented to support only OF devices, but we intend to extend
> it by incorporating new members to accommodate non-OF devices in the future.
> >
> >  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
> >  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
> >  include/linux/fpga/fpga-region.h |  32 +++++++++
> > include/uapi/linux/fpga-region.h |  51 ++++++++++++++
> >  4 files changed, 283 insertions(+), 1 deletion(-)  create mode 100644
> > include/uapi/linux/fpga-region.h
> >
> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> > index 753cd142503e..c6bea3c99a69 100644
> > --- a/drivers/fpga/fpga-region.c
> > +++ b/drivers/fpga/fpga-region.c
> > @@ -8,6 +8,7 @@
> >  #include <linux/fpga/fpga-bridge.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-region.h>
> > +#include <linux/fpga-region.h>
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
> > };  ATTRIBUTE_GROUPS(fpga_region);
> >
> > +static int fpga_region_device_open(struct inode *inode, struct file
> > +*file) {
> > +	struct miscdevice *miscdev = file->private_data;
> > +	struct fpga_region *region = container_of(miscdev, struct
> > +fpga_region, miscdev);
> > +
> > +	file->private_data = region;
> > +
> > +	return 0;
> > +}
> > +
> > +static int fpga_region_device_release(struct inode *inode, struct
> > +file *file) {
> > +	return 0;
> > +}
> > +
> > +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
> > +				     unsigned long arg)
> > +{
> > +	int err;
> > +	void __user *argp = (void __user *)arg;
> > +	struct fpga_region_config_info config_info;
> > +	struct fpga_region *region =  (struct fpga_region
> > +*)(file->private_data);
> > +
> > +	switch (cmd) {
> > +	case FPGA_REGION_IOCTL_LOAD:
> > +		if (copy_from_user(&config_info, argp, sizeof(struct
> fpga_region_config_info)))
> > +			return -EFAULT;
> > +
> > +		err = region->region_ops->region_config_enumeration(region,
> > +&config_info);
> > +
> > +		break;
> > +	case FPGA_REGION_IOCTL_REMOVE:
> > +		if (copy_from_user(&config_info, argp, sizeof(struct
> fpga_region_config_info)))
> > +			return -EFAULT;
> > +
> > +		err = region->region_ops->region_remove(region, &config_info);
> > +
> > +		break;
> > +	case FPGA_REGION_IOCTL_STATUS:
> > +		unsigned int status;
> > +
> > +		status = region->region_ops->region_status(region);
> > +
> > +		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
> > +			err = -EFAULT;
> > +		break;
> > +	default:
> > +		err = -ENOTTY;
> > +	}
> > +
> > +	return err;
> > +}
> > +
> > +static const struct file_operations fpga_region_fops = {
> > +	.owner		= THIS_MODULE,
> > +	.open		= fpga_region_device_open,
> > +	.release	= fpga_region_device_release,
> > +	.unlocked_ioctl	= fpga_region_device_ioctl,
> > +	.compat_ioctl	= fpga_region_device_ioctl,
> > +};
> > +
> >  /**
> >   * __fpga_region_register_full - create and register an FPGA Region device
> >   * @parent: device parent
> > @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const
> struct fpga_region_info
> >  	if (ret)
> >  		goto err_remove;
> >
> > +	if (info->region_ops) {
> > +		region->region_ops = info->region_ops;
> > +		region->miscdev.minor = MISC_DYNAMIC_MINOR;
> > +		region->miscdev.name = kobject_name(&region->dev.kobj);
> > +		region->miscdev.fops = &fpga_region_fops;
> > +		ret = misc_register(&region->miscdev);
> > +		if (ret) {
> > +			pr_err("fpga-region: failed to register misc device.\n");
> > +			goto err_remove;
> > +		}
> > +	}
> > +
> >  	ret = device_register(&region->dev);
> >  	if (ret) {
> > +		misc_deregister(&region->miscdev);
> >  		put_device(&region->dev);
> >  		return ERR_PTR(ret);
> >  	}
> > @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent,
> > struct fpga_manager *mgr,  }
> > EXPORT_SYMBOL_GPL(__fpga_region_register);
> >
> > +/**
> > + * __fpga_region_register_with_ops - create and register an FPGA
> > +Region device
> > + * with user interface call-backs.
> > + * @parent: device parent
> > + * @mgr: manager that programs this region
> > + * @region_ops: ops for low level FPGA region for device
> > +enumeration/removal
> > + * @priv: of-fpga-region private data
> > + * @get_bridges: optional function to get bridges to a list
> > + * @owner: module containing the get_bridges function
> > + *
> > + * This simple version of the register function should be sufficient for most users.
> > + * The fpga_region_register_full() function is available for users
> > +that need to
> > + * pass additional, optional parameters.
> > + *
> > + * Return: struct fpga_region or ERR_PTR()  */ struct fpga_region *
> > +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager
> *mgr,
> > +				const struct fpga_region_ops *region_ops,
> > +				void *priv,
> > +				int (*get_bridges)(struct fpga_region *),
> > +				struct module *owner)
> > +{
> > +	struct fpga_region_info info = { 0 };
> > +
> > +	info.mgr = mgr;
> > +	info.priv = priv;
> > +	info.get_bridges = get_bridges;
> > +	info.region_ops = region_ops;
> > +
> > +	return __fpga_region_register_full(parent, &info, owner); }
> > +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
> > +
> >  /**
> >   * fpga_region_unregister - unregister an FPGA region
> >   * @region: FPGA region
> > @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
> >   */
> >  void fpga_region_unregister(struct fpga_region *region)  {
> > +	misc_deregister(&region->miscdev);
> >  	device_unregister(&region->dev);
> >  }
> >  EXPORT_SYMBOL_GPL(fpga_region_unregister);
> > diff --git a/drivers/fpga/of-fpga-region.c
> > b/drivers/fpga/of-fpga-region.c index 8526a5a86f0c..63fe56e0466f
> > 100644
> > --- a/drivers/fpga/of-fpga-region.c
> > +++ b/drivers/fpga/of-fpga-region.c
> > @@ -8,6 +8,8 @@
> >  #include <linux/fpga/fpga-bridge.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-region.h>
> > +#include <linux/firmware.h>
> > +#include <linux/fpga-region.h>
> >  #include <linux/idr.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -18,6 +20,20 @@
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >
> > +/**
> > + * struct of_fpga_region_priv - Private data structure
> > + * image.
> > + * @dev:	Device data structure
> > + * @fw:		firmware of coeff table.
> > + * @path:	path of FPGA overlay image firmware file.
> > + * @ovcs_id:	overlay changeset id.
> > + */
> > +struct of_fpga_region_priv {
> > +	struct device *dev;
> > +	const struct firmware *fw;
> > +	int ovcs_id;
> > +};
> > +
> >  static const struct of_device_id fpga_region_of_match[] = {
> >  	{ .compatible = "fpga-region", },
> >  	{},
> > @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
> >  	.notifier_call = of_fpga_region_notify,  };
> >
> > +static int of_fpga_region_status(struct fpga_region *region) {
> > +	struct of_fpga_region_priv *ovcs = region->priv;
> > +
> > +	if (ovcs->ovcs_id)
> > +		return FPGA_REGION_HAS_PL;
> 
> Could you help specify what is PL?

We will replace "PL" (Programmable Logic) with FPGA_REGION_HAS_CONFIGURED
for better clarity.

> > +
> > +	return FPGA_REGION_EMPTY;
> > +}
> > +
> > +static int of_fpga_region_config_enumeration(struct fpga_region *region,
> > +					     struct fpga_region_config_info *config_info)
> {
> > +	struct of_fpga_region_priv *ovcs = region->priv;
> > +	int err;
> > +
> > +	/* if it's set do not allow changes */
> > +	if (ovcs->ovcs_id)
> > +		return -EPERM;
> > +
> > +	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
> > +	if (err != 0)
> > +		goto out_err;
> > +
> > +	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
> > +				   &ovcs->ovcs_id, NULL);
> > +	if (err < 0) {
> > +		pr_err("%s: Failed to create overlay (err=%d)\n",
> > +		       __func__, err);
> > +		release_firmware(ovcs->fw);
> > +		goto out_err;
> > +	}
> > +
> > +	return 0;
> > +
> > +out_err:
> > +	ovcs->ovcs_id = 0;
> > +	ovcs->fw = NULL;
> > +
> > +	return err;
> > +}
> > +
> > +static int of_fpga_region_config_remove(struct fpga_region *region,
> > +					struct fpga_region_config_info *config_info) {
> > +	struct of_fpga_region_priv *ovcs = region->priv;
> > +
> > +	if (!ovcs->ovcs_id)
> > +		return -EPERM;
> > +
> > +	of_overlay_remove(&ovcs->ovcs_id);
> > +	release_firmware(ovcs->fw);
> > +
> > +	ovcs->ovcs_id = 0;
> > +	ovcs->fw = NULL;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct fpga_region_ops region_ops = {
> > +	.region_status = of_fpga_region_status,
> > +	.region_config_enumeration = of_fpga_region_config_enumeration,
> > +	.region_remove = of_fpga_region_config_remove, };
> > +
> >  static int of_fpga_region_probe(struct platform_device *pdev)  {
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> > +	struct of_fpga_region_priv *priv;
> >  	struct fpga_region *region;
> >  	struct fpga_manager *mgr;
> >  	int ret;
> >
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +
> >  	/* Find the FPGA mgr specified by region or parent region. */
> >  	mgr = of_fpga_region_get_mgr(np);
> >  	if (IS_ERR(mgr))
> >  		return -EPROBE_DEFER;
> >
> > -	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
> > +	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
> > +					       of_fpga_region_get_bridges);
> >  	if (IS_ERR(region)) {
> >  		ret = PTR_ERR(region);
> >  		goto eprobe_mgr_put;
> > diff --git a/include/linux/fpga/fpga-region.h
> > b/include/linux/fpga/fpga-region.h
> > index 5fbc05fe70a6..3a3ba6dbb5e1 100644
> > --- a/include/linux/fpga/fpga-region.h
> > +++ b/include/linux/fpga/fpga-region.h
> > @@ -6,15 +6,35 @@
> >  #include <linux/device.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-bridge.h>
> > +#include <linux/fpga-region.h>
> > +#include <linux/miscdevice.h>
> >
> >  struct fpga_region;
> >
> > +/**
> > + * struct fpga_region_ops - ops for low level FPGA region ops for
> > +device
> > + * enumeration/removal
> > + * @region_status: returns the FPGA region status
> > + * @region_config_enumeration: Configure and enumerate the FPGA region.
> > + * @region_remove: Remove all devices within the FPGA region
> > + * (which are added as part of the enumeration).
> > + */
> > +struct fpga_region_ops {
> > +	int (*region_status)(struct fpga_region *region);
> > +	int (*region_config_enumeration)(struct fpga_region *region,
> > +					 struct fpga_region_config_info *config_info);
> 
> My current concern is still about this combined API, it just offloads all work to low
> level, but we have some common flows. That's why we introduce a common FPGA
> reprograming API.
> 
> I didn't see issue about the vendor specific pre configuration. They are generally
> needed to initialize the struct fpga_image_info, which is a common structure for
> fpga_region_program_fpga().
> 
> For port IDs(AFU) inputs for DFL, I think it could also be changed (Don't have to be
> implemented in this patchset). Previously DFL provides an uAPI for the whole
> device, so it needs a port_id input to position which fpga_region within the device for
> programming. But now, we are introducing a per fpga_region programming interface,
> IIUC port_id should not be needed anymore.
> 
> The combined API is truly simple for leveraging the existing of-fpga-region overlay
> apply mechanism. But IMHO that flow doesn't fit our new uAPI well. That flow is to
> adapt the generic configfs overlay interface, which comes to a dead end as you
> mentioned.
> 
> My gut feeling for the generic programing flow should be:
> 
>  1. Program the image to HW.
>  2. Enumerate the programmed image (apply the DT overlay)
> 
> Why we have to:
> 
>  1. Start enumeration.
>  2. On pre enumeration, programe the image.
>  3. Real enumeration.
> 

I agree with the approach of leveraging vendor-specific callbacks to handle
the distinct phases of the FPGA programming process. 
Here's the proposed flow.
 
Pre-Configuration:
A vendor-specific callback extracts the required pre-configuration details
and initializes struct fpga_image_info. This ensures that all vendor-specific
requirements are handled before proceeding to the programming phase.
 
Programming:
The common API fpga_region_program_fpga() is used to program the image
to hardware. This standardizes the programming logic, minimizing duplication
and ensuring consistency across implementations.
 
Enumeration:
A vendor-specific callback is used for real enumeration, enabling hardware
specific customization while keeping the flow flexible and adaptable

This approach provides a clean separation of responsibilities, with
vendor-specific logic confined to the pre-configuration and enumeration
phases, while the programming phase leverages a common API.
It simplifies maintenance and aligns well with the Program -> Enumerate flow.
 
Looking forward to your feedback.


Regards,
Navakishore.

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Marco Pagani]

On 2024-11-19 05:14, Xu Yilun wrote:
> On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote:
>> Introduces an IOCTL interface within the fpga-region subsystem,
>> providing a generic and standardized mechanism for configuring (or)
>> reprogramming FPGAs during runtime. The newly added interface supports
>> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
>> callbacks (e.g., configuration + enumeration, removal, and status) to
>> accommodate a wide range of device specific configurations.
>>
>> The IOCTL interface ensures compatibility with both OF and non-OF
>> devices, allowing for seamless FPGA reprogramming across diverse
>> platforms.
>>
>> Vendor-specific callbacks are integrated into the interface, enabling
>> custom FPGA configuration + enumeration, removal, and status reporting
>> mechanisms, ensuring flexibility for vendor implementations.
>>
>> This solution enhances FPGA runtime management, supporting various device
>> types and vendors, while ensuring compatibility with the current FPGA
>> configuration flow.
>>
>> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
>> ---
>> Changes for v2:
>>  - As discussed with Yilun, the implementation has been modified to utilize a
>>  callback approach, enabling seamless handling of both OF and non-OF devices.
>>
>>  - As suggested by Yilun in the POC code, we have moved away from using  void *args
>>  as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
>>  utilizing the fpga_region_config_info structure to gather user inputs. Currently,
>>  this structure is implemented to support only OF devices, but we intend to extend
>>  it by incorporating new members to accommodate non-OF devices in the future.
>>
>>  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
>>  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
>>  include/linux/fpga/fpga-region.h |  32 +++++++++
>>  include/uapi/linux/fpga-region.h |  51 ++++++++++++++
>>  4 files changed, 283 insertions(+), 1 deletion(-)
>>  create mode 100644 include/uapi/linux/fpga-region.h
>>
>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>> index 753cd142503e..c6bea3c99a69 100644
>> --- a/drivers/fpga/fpga-region.c
>> +++ b/drivers/fpga/fpga-region.c
>> @@ -8,6 +8,7 @@
>>  #include <linux/fpga/fpga-bridge.h>
>>  #include <linux/fpga/fpga-mgr.h>
>>  #include <linux/fpga/fpga-region.h>
>> +#include <linux/fpga-region.h>
>>  #include <linux/idr.h>
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
>>  };
>>  ATTRIBUTE_GROUPS(fpga_region);
>>  
>> +static int fpga_region_device_open(struct inode *inode, struct file *file)
>> +{
>> +	struct miscdevice *miscdev = file->private_data;
>> +	struct fpga_region *region = container_of(miscdev, struct fpga_region, miscdev);
>> +
>> +	file->private_data = region;
>> +
>> +	return 0;
>> +}
>> +
>> +static int fpga_region_device_release(struct inode *inode, struct file *file)
>> +{
>> +	return 0;
>> +}
>> +
>> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
>> +				     unsigned long arg)
>> +{
>> +	int err;
>> +	void __user *argp = (void __user *)arg;
>> +	struct fpga_region_config_info config_info;
>> +	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
>> +
>> +	switch (cmd) {
>> +	case FPGA_REGION_IOCTL_LOAD:
>> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
>> +			return -EFAULT;
>> +
>> +		err = region->region_ops->region_config_enumeration(region, &config_info);
>> +
>> +		break;
>> +	case FPGA_REGION_IOCTL_REMOVE:
>> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
>> +			return -EFAULT;
>> +
>> +		err = region->region_ops->region_remove(region, &config_info);
>> +
>> +		break;
>> +	case FPGA_REGION_IOCTL_STATUS:
>> +		unsigned int status;
>> +
>> +		status = region->region_ops->region_status(region);
>> +
>> +		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
>> +			err = -EFAULT;
>> +		break;
>> +	default:
>> +		err = -ENOTTY;
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static const struct file_operations fpga_region_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.open		= fpga_region_device_open,
>> +	.release	= fpga_region_device_release,
>> +	.unlocked_ioctl	= fpga_region_device_ioctl,
>> +	.compat_ioctl	= fpga_region_device_ioctl,
>> +};
>> +
>>  /**
>>   * __fpga_region_register_full - create and register an FPGA Region device
>>   * @parent: device parent
>> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
>>  	if (ret)
>>  		goto err_remove;
>>  
>> +	if (info->region_ops) {
>> +		region->region_ops = info->region_ops;
>> +		region->miscdev.minor = MISC_DYNAMIC_MINOR;
>> +		region->miscdev.name = kobject_name(&region->dev.kobj);
>> +		region->miscdev.fops = &fpga_region_fops;
>> +		ret = misc_register(&region->miscdev);
>> +		if (ret) {
>> +			pr_err("fpga-region: failed to register misc device.\n");
>> +			goto err_remove;
>> +		}
>> +	}
>> +
>>  	ret = device_register(&region->dev);
>>  	if (ret) {
>> +		misc_deregister(&region->miscdev);
>>  		put_device(&region->dev);
>>  		return ERR_PTR(ret);
>>  	}
>> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>>  }
>>  EXPORT_SYMBOL_GPL(__fpga_region_register);
>>  
>> +/**
>> + * __fpga_region_register_with_ops - create and register an FPGA Region device
>> + * with user interface call-backs.
>> + * @parent: device parent
>> + * @mgr: manager that programs this region
>> + * @region_ops: ops for low level FPGA region for device enumeration/removal
>> + * @priv: of-fpga-region private data
>> + * @get_bridges: optional function to get bridges to a list
>> + * @owner: module containing the get_bridges function
>> + *
>> + * This simple version of the register function should be sufficient for most users.
>> + * The fpga_region_register_full() function is available for users that need to
>> + * pass additional, optional parameters.
>> + *
>> + * Return: struct fpga_region or ERR_PTR()
>> + */
>> +struct fpga_region *
>> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
>> +				const struct fpga_region_ops *region_ops,
>> +				void *priv,
>> +				int (*get_bridges)(struct fpga_region *),
>> +				struct module *owner)
>> +{
>> +	struct fpga_region_info info = { 0 };
>> +
>> +	info.mgr = mgr;
>> +	info.priv = priv;
>> +	info.get_bridges = get_bridges;
>> +	info.region_ops = region_ops;
>> +
>> +	return __fpga_region_register_full(parent, &info, owner);
>> +}
>> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
>> +
>>  /**
>>   * fpga_region_unregister - unregister an FPGA region
>>   * @region: FPGA region
>> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
>>   */
>>  void fpga_region_unregister(struct fpga_region *region)
>>  {
>> +	misc_deregister(&region->miscdev);
>>  	device_unregister(&region->dev);
>>  }
>>  EXPORT_SYMBOL_GPL(fpga_region_unregister);
>> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
>> index 8526a5a86f0c..63fe56e0466f 100644
>> --- a/drivers/fpga/of-fpga-region.c
>> +++ b/drivers/fpga/of-fpga-region.c
>> @@ -8,6 +8,8 @@
>>  #include <linux/fpga/fpga-bridge.h>
>>  #include <linux/fpga/fpga-mgr.h>
>>  #include <linux/fpga/fpga-region.h>
>> +#include <linux/firmware.h>
>> +#include <linux/fpga-region.h>
>>  #include <linux/idr.h>
>>  #include <linux/kernel.h>
>>  #include <linux/list.h>
>> @@ -18,6 +20,20 @@
>>  #include <linux/slab.h>
>>  #include <linux/spinlock.h>
>>  
>> +/**
>> + * struct of_fpga_region_priv - Private data structure
>> + * image.
>> + * @dev:	Device data structure
>> + * @fw:		firmware of coeff table.
>> + * @path:	path of FPGA overlay image firmware file.
>> + * @ovcs_id:	overlay changeset id.
>> + */
>> +struct of_fpga_region_priv {
>> +	struct device *dev;
>> +	const struct firmware *fw;
>> +	int ovcs_id;
>> +};
>> +
>>  static const struct of_device_id fpga_region_of_match[] = {
>>  	{ .compatible = "fpga-region", },
>>  	{},
>> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
>>  	.notifier_call = of_fpga_region_notify,
>>  };
>>  
>> +static int of_fpga_region_status(struct fpga_region *region)
>> +{
>> +	struct of_fpga_region_priv *ovcs = region->priv;
>> +
>> +	if (ovcs->ovcs_id)
>> +		return FPGA_REGION_HAS_PL;
> 
> Could you help specify what is PL?
> 
>> +
>> +	return FPGA_REGION_EMPTY;
>> +}
>> +
>> +static int of_fpga_region_config_enumeration(struct fpga_region *region,
>> +					     struct fpga_region_config_info *config_info)
>> +{
>> +	struct of_fpga_region_priv *ovcs = region->priv;
>> +	int err;
>> +
>> +	/* if it's set do not allow changes */
>> +	if (ovcs->ovcs_id)
>> +		return -EPERM;
>> +
>> +	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
>> +	if (err != 0)
>> +		goto out_err;
>> +
>> +	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
>> +				   &ovcs->ovcs_id, NULL);
>> +	if (err < 0) {
>> +		pr_err("%s: Failed to create overlay (err=%d)\n",
>> +		       __func__, err);
>> +		release_firmware(ovcs->fw);
>> +		goto out_err;
>> +	}
>> +
>> +	return 0;
>> +
>> +out_err:
>> +	ovcs->ovcs_id = 0;
>> +	ovcs->fw = NULL;
>> +
>> +	return err;
>> +}
>> +
>> +static int of_fpga_region_config_remove(struct fpga_region *region,
>> +					struct fpga_region_config_info *config_info)
>> +{
>> +	struct of_fpga_region_priv *ovcs = region->priv;
>> +
>> +	if (!ovcs->ovcs_id)
>> +		return -EPERM;
>> +
>> +	of_overlay_remove(&ovcs->ovcs_id);
>> +	release_firmware(ovcs->fw);
>> +
>> +	ovcs->ovcs_id = 0;
>> +	ovcs->fw = NULL;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct fpga_region_ops region_ops = {
>> +	.region_status = of_fpga_region_status,
>> +	.region_config_enumeration = of_fpga_region_config_enumeration,
>> +	.region_remove = of_fpga_region_config_remove,
>> +};
>> +
>>  static int of_fpga_region_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>>  	struct device_node *np = dev->of_node;
>> +	struct of_fpga_region_priv *priv;
>>  	struct fpga_region *region;
>>  	struct fpga_manager *mgr;
>>  	int ret;
>>  
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->dev = dev;
>> +
>>  	/* Find the FPGA mgr specified by region or parent region. */
>>  	mgr = of_fpga_region_get_mgr(np);
>>  	if (IS_ERR(mgr))
>>  		return -EPROBE_DEFER;
>>  
>> -	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
>> +	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
>> +					       of_fpga_region_get_bridges);
>>  	if (IS_ERR(region)) {
>>  		ret = PTR_ERR(region);
>>  		goto eprobe_mgr_put;
>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>> index 5fbc05fe70a6..3a3ba6dbb5e1 100644
>> --- a/include/linux/fpga/fpga-region.h
>> +++ b/include/linux/fpga/fpga-region.h
>> @@ -6,15 +6,35 @@
>>  #include <linux/device.h>
>>  #include <linux/fpga/fpga-mgr.h>
>>  #include <linux/fpga/fpga-bridge.h>
>> +#include <linux/fpga-region.h>
>> +#include <linux/miscdevice.h>
>>  
>>  struct fpga_region;
>>  
>> +/**
>> + * struct fpga_region_ops - ops for low level FPGA region ops for device
>> + * enumeration/removal
>> + * @region_status: returns the FPGA region status
>> + * @region_config_enumeration: Configure and enumerate the FPGA region.
>> + * @region_remove: Remove all devices within the FPGA region
>> + * (which are added as part of the enumeration).
>> + */
>> +struct fpga_region_ops {
>> +	int (*region_status)(struct fpga_region *region);
>> +	int (*region_config_enumeration)(struct fpga_region *region,
>> +					 struct fpga_region_config_info *config_info);
> 
> My current concern is still about this combined API, it just offloads
> all work to low level, but we have some common flows. That's why we
> introduce a common FPGA reprograming API.
> 
> I didn't see issue about the vendor specific pre configuration. They
> are generally needed to initialize the struct fpga_image_info, which
> is a common structure for fpga_region_program_fpga().
> 
> For port IDs(AFU) inputs for DFL, I think it could also be changed
> (Don't have to be implemented in this patchset). Previously DFL
> provides an uAPI for the whole device, so it needs a port_id input to
> position which fpga_region within the device for programming. But now,
> we are introducing a per fpga_region programming interface, IIUC port_id
> should not be needed anymore.
> 
> The combined API is truly simple for leveraging the existing
> of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit
> our new uAPI well. That flow is to adapt the generic configfs overlay
> interface, which comes to a dead end as you mentioned.
> 
> My gut feeling for the generic programing flow should be:
> 
>  1. Program the image to HW.
>  2. Enumerate the programmed image (apply the DT overlay)
> 
> Why we have to:
> 
>  1. Start enumeration.
>  2. On pre enumeration, programe the image.
>  3. Real enumeration.

I'm currently working on an RFC to propose a rework of the fpga
subsystem in order to make it more aligned with the device model. One of
the ideas I'm experimenting with is having a bus (struct bus_type) for
fpga regions (devices) so that we can have region drivers that could
handle internal device enumeration/management whenever a new region is
configured on the fabric. Does this make sense in your opinions?

Concerning the reconfiguration, wouldn't it be cleaner to use a
per-region sysfs interface at fpga-region level? It would still work
for both OF & non-OF cases.

Thanks,
Marco

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

> > > + * struct fpga_region_ops - ops for low level FPGA region ops for
> > > +device
> > > + * enumeration/removal
> > > + * @region_status: returns the FPGA region status
> > > + * @region_config_enumeration: Configure and enumerate the FPGA region.
> > > + * @region_remove: Remove all devices within the FPGA region
> > > + * (which are added as part of the enumeration).
> > > + */
> > > +struct fpga_region_ops {
> > > +	int (*region_status)(struct fpga_region *region);
> > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > +					 struct fpga_region_config_info *config_info);
> > 
> > My current concern is still about this combined API, it just offloads all work to low
> > level, but we have some common flows. That's why we introduce a common FPGA
> > reprograming API.
> > 
> > I didn't see issue about the vendor specific pre configuration. They are generally
> > needed to initialize the struct fpga_image_info, which is a common structure for
> > fpga_region_program_fpga().
> > 
> > For port IDs(AFU) inputs for DFL, I think it could also be changed (Don't have to be
> > implemented in this patchset). Previously DFL provides an uAPI for the whole
> > device, so it needs a port_id input to position which fpga_region within the device for
> > programming. But now, we are introducing a per fpga_region programming interface,
> > IIUC port_id should not be needed anymore.
> > 
> > The combined API is truly simple for leveraging the existing of-fpga-region overlay
> > apply mechanism. But IMHO that flow doesn't fit our new uAPI well. That flow is to
> > adapt the generic configfs overlay interface, which comes to a dead end as you
> > mentioned.
> > 
> > My gut feeling for the generic programing flow should be:
> > 
> >  1. Program the image to HW.
> >  2. Enumerate the programmed image (apply the DT overlay)
> > 
> > Why we have to:
> > 
> >  1. Start enumeration.
> >  2. On pre enumeration, programe the image.
> >  3. Real enumeration.
> > 
> 
> I agree with the approach of leveraging vendor-specific callbacks to handle
> the distinct phases of the FPGA programming process. 
> Here's the proposed flow.
>  
> Pre-Configuration:
> A vendor-specific callback extracts the required pre-configuration details
> and initializes struct fpga_image_info. This ensures that all vendor-specific

Since we need to construct the fpga_image_info, initialize multiple
field as needed, I'm wondering if configfs could be a solution for the
uAPI?

> requirements are handled before proceeding to the programming phase.
>  
> Programming:
> The common API fpga_region_program_fpga() is used to program the image
> to hardware. This standardizes the programming logic, minimizing duplication
> and ensuring consistency across implementations.
>  
> Enumeration:
> A vendor-specific callback is used for real enumeration, enabling hardware
> specific customization while keeping the flow flexible and adaptable
> 
> This approach provides a clean separation of responsibilities, with
> vendor-specific logic confined to the pre-configuration and enumeration
> phases, while the programming phase leverages a common API.
> It simplifies maintenance and aligns well with the Program -> Enumerate flow.

Generally I'm good to this flow.

Thanks,
Yilun

>  
> Looking forward to your feedback.
> 
> 
> Regards,
> Navakishore.

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

On Mon, Nov 25, 2024 at 12:26:06PM +0100, Marco Pagani wrote:
> 
> 
> On 2024-11-19 05:14, Xu Yilun wrote:
> > On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote:
> >> Introduces an IOCTL interface within the fpga-region subsystem,
> >> providing a generic and standardized mechanism for configuring (or)
> >> reprogramming FPGAs during runtime. The newly added interface supports
> >> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
> >> callbacks (e.g., configuration + enumeration, removal, and status) to
> >> accommodate a wide range of device specific configurations.
> >>
> >> The IOCTL interface ensures compatibility with both OF and non-OF
> >> devices, allowing for seamless FPGA reprogramming across diverse
> >> platforms.
> >>
> >> Vendor-specific callbacks are integrated into the interface, enabling
> >> custom FPGA configuration + enumeration, removal, and status reporting
> >> mechanisms, ensuring flexibility for vendor implementations.
> >>
> >> This solution enhances FPGA runtime management, supporting various device
> >> types and vendors, while ensuring compatibility with the current FPGA
> >> configuration flow.
> >>
> >> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
> >> ---
> >> Changes for v2:
> >>  - As discussed with Yilun, the implementation has been modified to utilize a
> >>  callback approach, enabling seamless handling of both OF and non-OF devices.
> >>
> >>  - As suggested by Yilun in the POC code, we have moved away from using  void *args
> >>  as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
> >>  utilizing the fpga_region_config_info structure to gather user inputs. Currently,
> >>  this structure is implemented to support only OF devices, but we intend to extend
> >>  it by incorporating new members to accommodate non-OF devices in the future.
> >>
> >>  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
> >>  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
> >>  include/linux/fpga/fpga-region.h |  32 +++++++++
> >>  include/uapi/linux/fpga-region.h |  51 ++++++++++++++
> >>  4 files changed, 283 insertions(+), 1 deletion(-)
> >>  create mode 100644 include/uapi/linux/fpga-region.h
> >>
> >> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> >> index 753cd142503e..c6bea3c99a69 100644
> >> --- a/drivers/fpga/fpga-region.c
> >> +++ b/drivers/fpga/fpga-region.c
> >> @@ -8,6 +8,7 @@
> >>  #include <linux/fpga/fpga-bridge.h>
> >>  #include <linux/fpga/fpga-mgr.h>
> >>  #include <linux/fpga/fpga-region.h>
> >> +#include <linux/fpga-region.h>
> >>  #include <linux/idr.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/list.h>
> >> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
> >>  };
> >>  ATTRIBUTE_GROUPS(fpga_region);
> >>  
> >> +static int fpga_region_device_open(struct inode *inode, struct file *file)
> >> +{
> >> +	struct miscdevice *miscdev = file->private_data;
> >> +	struct fpga_region *region = container_of(miscdev, struct fpga_region, miscdev);
> >> +
> >> +	file->private_data = region;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int fpga_region_device_release(struct inode *inode, struct file *file)
> >> +{
> >> +	return 0;
> >> +}
> >> +
> >> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
> >> +				     unsigned long arg)
> >> +{
> >> +	int err;
> >> +	void __user *argp = (void __user *)arg;
> >> +	struct fpga_region_config_info config_info;
> >> +	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
> >> +
> >> +	switch (cmd) {
> >> +	case FPGA_REGION_IOCTL_LOAD:
> >> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
> >> +			return -EFAULT;
> >> +
> >> +		err = region->region_ops->region_config_enumeration(region, &config_info);
> >> +
> >> +		break;
> >> +	case FPGA_REGION_IOCTL_REMOVE:
> >> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
> >> +			return -EFAULT;
> >> +
> >> +		err = region->region_ops->region_remove(region, &config_info);
> >> +
> >> +		break;
> >> +	case FPGA_REGION_IOCTL_STATUS:
> >> +		unsigned int status;
> >> +
> >> +		status = region->region_ops->region_status(region);
> >> +
> >> +		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
> >> +			err = -EFAULT;
> >> +		break;
> >> +	default:
> >> +		err = -ENOTTY;
> >> +	}
> >> +
> >> +	return err;
> >> +}
> >> +
> >> +static const struct file_operations fpga_region_fops = {
> >> +	.owner		= THIS_MODULE,
> >> +	.open		= fpga_region_device_open,
> >> +	.release	= fpga_region_device_release,
> >> +	.unlocked_ioctl	= fpga_region_device_ioctl,
> >> +	.compat_ioctl	= fpga_region_device_ioctl,
> >> +};
> >> +
> >>  /**
> >>   * __fpga_region_register_full - create and register an FPGA Region device
> >>   * @parent: device parent
> >> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
> >>  	if (ret)
> >>  		goto err_remove;
> >>  
> >> +	if (info->region_ops) {
> >> +		region->region_ops = info->region_ops;
> >> +		region->miscdev.minor = MISC_DYNAMIC_MINOR;
> >> +		region->miscdev.name = kobject_name(&region->dev.kobj);
> >> +		region->miscdev.fops = &fpga_region_fops;
> >> +		ret = misc_register(&region->miscdev);
> >> +		if (ret) {
> >> +			pr_err("fpga-region: failed to register misc device.\n");
> >> +			goto err_remove;
> >> +		}
> >> +	}
> >> +
> >>  	ret = device_register(&region->dev);
> >>  	if (ret) {
> >> +		misc_deregister(&region->miscdev);
> >>  		put_device(&region->dev);
> >>  		return ERR_PTR(ret);
> >>  	}
> >> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
> >>  }
> >>  EXPORT_SYMBOL_GPL(__fpga_region_register);
> >>  
> >> +/**
> >> + * __fpga_region_register_with_ops - create and register an FPGA Region device
> >> + * with user interface call-backs.
> >> + * @parent: device parent
> >> + * @mgr: manager that programs this region
> >> + * @region_ops: ops for low level FPGA region for device enumeration/removal
> >> + * @priv: of-fpga-region private data
> >> + * @get_bridges: optional function to get bridges to a list
> >> + * @owner: module containing the get_bridges function
> >> + *
> >> + * This simple version of the register function should be sufficient for most users.
> >> + * The fpga_region_register_full() function is available for users that need to
> >> + * pass additional, optional parameters.
> >> + *
> >> + * Return: struct fpga_region or ERR_PTR()
> >> + */
> >> +struct fpga_region *
> >> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
> >> +				const struct fpga_region_ops *region_ops,
> >> +				void *priv,
> >> +				int (*get_bridges)(struct fpga_region *),
> >> +				struct module *owner)
> >> +{
> >> +	struct fpga_region_info info = { 0 };
> >> +
> >> +	info.mgr = mgr;
> >> +	info.priv = priv;
> >> +	info.get_bridges = get_bridges;
> >> +	info.region_ops = region_ops;
> >> +
> >> +	return __fpga_region_register_full(parent, &info, owner);
> >> +}
> >> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
> >> +
> >>  /**
> >>   * fpga_region_unregister - unregister an FPGA region
> >>   * @region: FPGA region
> >> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
> >>   */
> >>  void fpga_region_unregister(struct fpga_region *region)
> >>  {
> >> +	misc_deregister(&region->miscdev);
> >>  	device_unregister(&region->dev);
> >>  }
> >>  EXPORT_SYMBOL_GPL(fpga_region_unregister);
> >> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
> >> index 8526a5a86f0c..63fe56e0466f 100644
> >> --- a/drivers/fpga/of-fpga-region.c
> >> +++ b/drivers/fpga/of-fpga-region.c
> >> @@ -8,6 +8,8 @@
> >>  #include <linux/fpga/fpga-bridge.h>
> >>  #include <linux/fpga/fpga-mgr.h>
> >>  #include <linux/fpga/fpga-region.h>
> >> +#include <linux/firmware.h>
> >> +#include <linux/fpga-region.h>
> >>  #include <linux/idr.h>
> >>  #include <linux/kernel.h>
> >>  #include <linux/list.h>
> >> @@ -18,6 +20,20 @@
> >>  #include <linux/slab.h>
> >>  #include <linux/spinlock.h>
> >>  
> >> +/**
> >> + * struct of_fpga_region_priv - Private data structure
> >> + * image.
> >> + * @dev:	Device data structure
> >> + * @fw:		firmware of coeff table.
> >> + * @path:	path of FPGA overlay image firmware file.
> >> + * @ovcs_id:	overlay changeset id.
> >> + */
> >> +struct of_fpga_region_priv {
> >> +	struct device *dev;
> >> +	const struct firmware *fw;
> >> +	int ovcs_id;
> >> +};
> >> +
> >>  static const struct of_device_id fpga_region_of_match[] = {
> >>  	{ .compatible = "fpga-region", },
> >>  	{},
> >> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
> >>  	.notifier_call = of_fpga_region_notify,
> >>  };
> >>  
> >> +static int of_fpga_region_status(struct fpga_region *region)
> >> +{
> >> +	struct of_fpga_region_priv *ovcs = region->priv;
> >> +
> >> +	if (ovcs->ovcs_id)
> >> +		return FPGA_REGION_HAS_PL;
> > 
> > Could you help specify what is PL?
> > 
> >> +
> >> +	return FPGA_REGION_EMPTY;
> >> +}
> >> +
> >> +static int of_fpga_region_config_enumeration(struct fpga_region *region,
> >> +					     struct fpga_region_config_info *config_info)
> >> +{
> >> +	struct of_fpga_region_priv *ovcs = region->priv;
> >> +	int err;
> >> +
> >> +	/* if it's set do not allow changes */
> >> +	if (ovcs->ovcs_id)
> >> +		return -EPERM;
> >> +
> >> +	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
> >> +	if (err != 0)
> >> +		goto out_err;
> >> +
> >> +	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
> >> +				   &ovcs->ovcs_id, NULL);
> >> +	if (err < 0) {
> >> +		pr_err("%s: Failed to create overlay (err=%d)\n",
> >> +		       __func__, err);
> >> +		release_firmware(ovcs->fw);
> >> +		goto out_err;
> >> +	}
> >> +
> >> +	return 0;
> >> +
> >> +out_err:
> >> +	ovcs->ovcs_id = 0;
> >> +	ovcs->fw = NULL;
> >> +
> >> +	return err;
> >> +}
> >> +
> >> +static int of_fpga_region_config_remove(struct fpga_region *region,
> >> +					struct fpga_region_config_info *config_info)
> >> +{
> >> +	struct of_fpga_region_priv *ovcs = region->priv;
> >> +
> >> +	if (!ovcs->ovcs_id)
> >> +		return -EPERM;
> >> +
> >> +	of_overlay_remove(&ovcs->ovcs_id);
> >> +	release_firmware(ovcs->fw);
> >> +
> >> +	ovcs->ovcs_id = 0;
> >> +	ovcs->fw = NULL;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static const struct fpga_region_ops region_ops = {
> >> +	.region_status = of_fpga_region_status,
> >> +	.region_config_enumeration = of_fpga_region_config_enumeration,
> >> +	.region_remove = of_fpga_region_config_remove,
> >> +};
> >> +
> >>  static int of_fpga_region_probe(struct platform_device *pdev)
> >>  {
> >>  	struct device *dev = &pdev->dev;
> >>  	struct device_node *np = dev->of_node;
> >> +	struct of_fpga_region_priv *priv;
> >>  	struct fpga_region *region;
> >>  	struct fpga_manager *mgr;
> >>  	int ret;
> >>  
> >> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> +	if (!priv)
> >> +		return -ENOMEM;
> >> +
> >> +	priv->dev = dev;
> >> +
> >>  	/* Find the FPGA mgr specified by region or parent region. */
> >>  	mgr = of_fpga_region_get_mgr(np);
> >>  	if (IS_ERR(mgr))
> >>  		return -EPROBE_DEFER;
> >>  
> >> -	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
> >> +	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
> >> +					       of_fpga_region_get_bridges);
> >>  	if (IS_ERR(region)) {
> >>  		ret = PTR_ERR(region);
> >>  		goto eprobe_mgr_put;
> >> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
> >> index 5fbc05fe70a6..3a3ba6dbb5e1 100644
> >> --- a/include/linux/fpga/fpga-region.h
> >> +++ b/include/linux/fpga/fpga-region.h
> >> @@ -6,15 +6,35 @@
> >>  #include <linux/device.h>
> >>  #include <linux/fpga/fpga-mgr.h>
> >>  #include <linux/fpga/fpga-bridge.h>
> >> +#include <linux/fpga-region.h>
> >> +#include <linux/miscdevice.h>
> >>  
> >>  struct fpga_region;
> >>  
> >> +/**
> >> + * struct fpga_region_ops - ops for low level FPGA region ops for device
> >> + * enumeration/removal
> >> + * @region_status: returns the FPGA region status
> >> + * @region_config_enumeration: Configure and enumerate the FPGA region.
> >> + * @region_remove: Remove all devices within the FPGA region
> >> + * (which are added as part of the enumeration).
> >> + */
> >> +struct fpga_region_ops {
> >> +	int (*region_status)(struct fpga_region *region);
> >> +	int (*region_config_enumeration)(struct fpga_region *region,
> >> +					 struct fpga_region_config_info *config_info);
> > 
> > My current concern is still about this combined API, it just offloads
> > all work to low level, but we have some common flows. That's why we
> > introduce a common FPGA reprograming API.
> > 
> > I didn't see issue about the vendor specific pre configuration. They
> > are generally needed to initialize the struct fpga_image_info, which
> > is a common structure for fpga_region_program_fpga().
> > 
> > For port IDs(AFU) inputs for DFL, I think it could also be changed
> > (Don't have to be implemented in this patchset). Previously DFL
> > provides an uAPI for the whole device, so it needs a port_id input to
> > position which fpga_region within the device for programming. But now,
> > we are introducing a per fpga_region programming interface, IIUC port_id
> > should not be needed anymore.
> > 
> > The combined API is truly simple for leveraging the existing
> > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit
> > our new uAPI well. That flow is to adapt the generic configfs overlay
> > interface, which comes to a dead end as you mentioned.
> > 
> > My gut feeling for the generic programing flow should be:
> > 
> >  1. Program the image to HW.
> >  2. Enumerate the programmed image (apply the DT overlay)
> > 
> > Why we have to:
> > 
> >  1. Start enumeration.
> >  2. On pre enumeration, programe the image.
> >  3. Real enumeration.
> 
> I'm currently working on an RFC to propose a rework of the fpga
> subsystem in order to make it more aligned with the device model. One of
> the ideas I'm experimenting with is having a bus (struct bus_type) for
> fpga regions (devices) so that we can have region drivers that could
> handle internal device enumeration/management whenever a new region is
> configured on the fabric. Does this make sense in your opinions?

mm.. I didn't fully understand the need to have a region driver, what's
the issue to solve?

Thanks,
Yilun

> 
> Concerning the reconfiguration, wouldn't it be cleaner to use a
> per-region sysfs interface at fpga-region level? It would still work
> for both OF & non-OF cases.
> 
> Thanks,
> Marco
> 
> 

RE: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Manne, Nava kishore]

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Wednesday, November 27, 2024 7:20 AM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
> 
> > > > + * struct fpga_region_ops - ops for low level FPGA region ops for
> > > > +device
> > > > + * enumeration/removal
> > > > + * @region_status: returns the FPGA region status
> > > > + * @region_config_enumeration: Configure and enumerate the FPGA region.
> > > > + * @region_remove: Remove all devices within the FPGA region
> > > > + * (which are added as part of the enumeration).
> > > > + */
> > > > +struct fpga_region_ops {
> > > > +	int (*region_status)(struct fpga_region *region);
> > > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > > +					 struct fpga_region_config_info *config_info);
> > >
> > > My current concern is still about this combined API, it just
> > > offloads all work to low level, but we have some common flows.
> > > That's why we introduce a common FPGA reprograming API.
> > >
> > > I didn't see issue about the vendor specific pre configuration. They
> > > are generally needed to initialize the struct fpga_image_info, which
> > > is a common structure for fpga_region_program_fpga().
> > >
> > > For port IDs(AFU) inputs for DFL, I think it could also be changed
> > > (Don't have to be implemented in this patchset). Previously DFL
> > > provides an uAPI for the whole device, so it needs a port_id input
> > > to position which fpga_region within the device for programming. But
> > > now, we are introducing a per fpga_region programming interface, IIUC port_id
> should not be needed anymore.
> > >
> > > The combined API is truly simple for leveraging the existing
> > > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't
> > > fit our new uAPI well. That flow is to adapt the generic configfs
> > > overlay interface, which comes to a dead end as you mentioned.
> > >
> > > My gut feeling for the generic programing flow should be:
> > >
> > >  1. Program the image to HW.
> > >  2. Enumerate the programmed image (apply the DT overlay)
> > >
> > > Why we have to:
> > >
> > >  1. Start enumeration.
> > >  2. On pre enumeration, programe the image.
> > >  3. Real enumeration.
> > >
> >
> > I agree with the approach of leveraging vendor-specific callbacks to
> > handle the distinct phases of the FPGA programming process.
> > Here's the proposed flow.
> >
> > Pre-Configuration:
> > A vendor-specific callback extracts the required pre-configuration
> > details and initializes struct fpga_image_info. This ensures that all
> > vendor-specific
> 
> Since we need to construct the fpga_image_info, initialize multiple field as needed,
> I'm wondering if configfs could be a solution for the uAPI?
> 

A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL flow. 
The POC code looks as follows.

static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
	struct fpga_region_config_info config_info;
	void __user *argp = (void __user *)arg;
	struct device *dev = &region->dev;
	struct fpga_image_info *info;
	int err;

	switch (cmd) {
	case FPGA_REGION_IOCTL_LOAD:
		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
		return -EFAULT;

		info = fpga_image_info_alloc(dev);
		if (!info)
			return ERR_PTR(-ENOMEM);

		/* A vendor-specific callback extracts the required pre-configuration
		 * details and initializes struct fpga_image_info. This ensures that all
		 * vendor-specific requirements are handled before proceeding to
		 * the programming phase.
		 */
		err = region->region_ops->region_preconfig(region, &config_info, info);
		if (err)
			return err;

		/* The common API fpga_region_program_fpga() is used to program
		 * the image to hardware.
		 */
		region->info = info;
		err = fpga_region_program_fpga(region);
		if (err) {
			fpga_image_info_free(info);
			region->info = NULL;
		}

		/* A vendor-specific callback is used for real enumeration, enabling
		 * hardware specific customization.
		 */
		err = region->region_ops->region_enumeration(region, &config_info);

		break;

	case FPGA_REGION_IOCTL_REMOVE:
		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
			return -EFAULT;

		err = region->region_ops->region_remove(region, &config_info);
		if (err)
			return err;

		fpga_image_info_free(region->info);

		break;

	case FPGA_REGION_IOCTL_STATUS:
		unsigned int status;

		status = region->region_ops->region_status(region);

		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
			err = -EFAULT;

		break;

	default:
		err = -ENOTTY;
	}

	return err;
}

Regards,
Navakishore.

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> Hi Yilun,
> 
> > -----Original Message-----
> > From: Xu Yilun <yilun.xu@linux.intel.com>
> > Sent: Wednesday, November 27, 2024 7:20 AM
> > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> > yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> > linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> > devicetree@vger.kernel.org
> > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> > FPGA programming
> > 
> > > > > + * struct fpga_region_ops - ops for low level FPGA region ops for
> > > > > +device
> > > > > + * enumeration/removal
> > > > > + * @region_status: returns the FPGA region status
> > > > > + * @region_config_enumeration: Configure and enumerate the FPGA region.
> > > > > + * @region_remove: Remove all devices within the FPGA region
> > > > > + * (which are added as part of the enumeration).
> > > > > + */
> > > > > +struct fpga_region_ops {
> > > > > +	int (*region_status)(struct fpga_region *region);
> > > > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > > > +					 struct fpga_region_config_info *config_info);
> > > >
> > > > My current concern is still about this combined API, it just
> > > > offloads all work to low level, but we have some common flows.
> > > > That's why we introduce a common FPGA reprograming API.
> > > >
> > > > I didn't see issue about the vendor specific pre configuration. They
> > > > are generally needed to initialize the struct fpga_image_info, which
> > > > is a common structure for fpga_region_program_fpga().
> > > >
> > > > For port IDs(AFU) inputs for DFL, I think it could also be changed
> > > > (Don't have to be implemented in this patchset). Previously DFL
> > > > provides an uAPI for the whole device, so it needs a port_id input
> > > > to position which fpga_region within the device for programming. But
> > > > now, we are introducing a per fpga_region programming interface, IIUC port_id
> > should not be needed anymore.
> > > >
> > > > The combined API is truly simple for leveraging the existing
> > > > of-fpga-region overlay apply mechanism. But IMHO that flow doesn't
> > > > fit our new uAPI well. That flow is to adapt the generic configfs
> > > > overlay interface, which comes to a dead end as you mentioned.
> > > >
> > > > My gut feeling for the generic programing flow should be:
> > > >
> > > >  1. Program the image to HW.
> > > >  2. Enumerate the programmed image (apply the DT overlay)
> > > >
> > > > Why we have to:
> > > >
> > > >  1. Start enumeration.
> > > >  2. On pre enumeration, programe the image.
> > > >  3. Real enumeration.
> > > >
> > >
> > > I agree with the approach of leveraging vendor-specific callbacks to
> > > handle the distinct phases of the FPGA programming process.
> > > Here's the proposed flow.
> > >
> > > Pre-Configuration:
> > > A vendor-specific callback extracts the required pre-configuration
> > > details and initializes struct fpga_image_info. This ensures that all
> > > vendor-specific
> > 
> > Since we need to construct the fpga_image_info, initialize multiple field as needed,
> > I'm wondering if configfs could be a solution for the uAPI?
> > 
> 
> A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL flow. 
> The POC code looks as follows.

I prefer more to configfs cause it provides standard FS way to create
the fpga_image_info object, e.g. which attributes are visible for
OF/non-OF region, which attributes come from image blob and can only be
RO, etc.

Of couse ioctl() could achieve the same goal but would add much more
specific rules (maybe flags/types) for user to follow.

Thanks,
Yilun

> 
> static long fpga_region_device_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> 	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
> 	struct fpga_region_config_info config_info;
> 	void __user *argp = (void __user *)arg;
> 	struct device *dev = &region->dev;
> 	struct fpga_image_info *info;
> 	int err;
> 
> 	switch (cmd) {
> 	case FPGA_REGION_IOCTL_LOAD:
> 		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
> 		return -EFAULT;
> 
> 		info = fpga_image_info_alloc(dev);
> 		if (!info)
> 			return ERR_PTR(-ENOMEM);
> 
> 		/* A vendor-specific callback extracts the required pre-configuration
> 		 * details and initializes struct fpga_image_info. This ensures that all
> 		 * vendor-specific requirements are handled before proceeding to
> 		 * the programming phase.
> 		 */
> 		err = region->region_ops->region_preconfig(region, &config_info, info);
> 		if (err)
> 			return err;
> 
> 		/* The common API fpga_region_program_fpga() is used to program
> 		 * the image to hardware.
> 		 */
> 		region->info = info;
> 		err = fpga_region_program_fpga(region);
> 		if (err) {
> 			fpga_image_info_free(info);
> 			region->info = NULL;
> 		}
> 
> 		/* A vendor-specific callback is used for real enumeration, enabling
> 		 * hardware specific customization.
> 		 */
> 		err = region->region_ops->region_enumeration(region, &config_info);
> 
> 		break;
> 
> 	case FPGA_REGION_IOCTL_REMOVE:
> 		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
> 			return -EFAULT;
> 
> 		err = region->region_ops->region_remove(region, &config_info);
> 		if (err)
> 			return err;
> 
> 		fpga_image_info_free(region->info);
> 
> 		break;
> 
> 	case FPGA_REGION_IOCTL_STATUS:
> 		unsigned int status;
> 
> 		status = region->region_ops->region_status(region);
> 
> 		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
> 			err = -EFAULT;
> 
> 		break;
> 
> 	default:
> 		err = -ENOTTY;
> 	}
> 
> 	return err;
> }
> 
> Regards,
> Navakishore.

RE: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Manne, Nava kishore]

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Tuesday, December 10, 2024 2:34 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
> 
> On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> > Hi Yilun,
> >
> > > -----Original Message-----
> > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > Sent: Wednesday, November 27, 2024 7:20 AM
> > > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org;
> > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> > > robh@kernel.org; saravanak@google.com; linux-kernel@vger.kernel.org;
> > > linux-fpga@vger.kernel.org; devicetree@vger.kernel.org
> > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface
> > > for runtime FPGA programming
> > >
> > > > > > + * struct fpga_region_ops - ops for low level FPGA region ops
> > > > > > +for device
> > > > > > + * enumeration/removal
> > > > > > + * @region_status: returns the FPGA region status
> > > > > > + * @region_config_enumeration: Configure and enumerate the FPGA
> region.
> > > > > > + * @region_remove: Remove all devices within the FPGA region
> > > > > > + * (which are added as part of the enumeration).
> > > > > > + */
> > > > > > +struct fpga_region_ops {
> > > > > > +	int (*region_status)(struct fpga_region *region);
> > > > > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > > > > +					 struct fpga_region_config_info
> *config_info);
> > > > >
> > > > > My current concern is still about this combined API, it just
> > > > > offloads all work to low level, but we have some common flows.
> > > > > That's why we introduce a common FPGA reprograming API.
> > > > >
> > > > > I didn't see issue about the vendor specific pre configuration.
> > > > > They are generally needed to initialize the struct
> > > > > fpga_image_info, which is a common structure for
> fpga_region_program_fpga().
> > > > >
> > > > > For port IDs(AFU) inputs for DFL, I think it could also be
> > > > > changed (Don't have to be implemented in this patchset).
> > > > > Previously DFL provides an uAPI for the whole device, so it
> > > > > needs a port_id input to position which fpga_region within the
> > > > > device for programming. But now, we are introducing a per
> > > > > fpga_region programming interface, IIUC port_id
> > > should not be needed anymore.
> > > > >
> > > > > The combined API is truly simple for leveraging the existing
> > > > > of-fpga-region overlay apply mechanism. But IMHO that flow
> > > > > doesn't fit our new uAPI well. That flow is to adapt the generic
> > > > > configfs overlay interface, which comes to a dead end as you mentioned.
> > > > >
> > > > > My gut feeling for the generic programing flow should be:
> > > > >
> > > > >  1. Program the image to HW.
> > > > >  2. Enumerate the programmed image (apply the DT overlay)
> > > > >
> > > > > Why we have to:
> > > > >
> > > > >  1. Start enumeration.
> > > > >  2. On pre enumeration, programe the image.
> > > > >  3. Real enumeration.
> > > > >
> > > >
> > > > I agree with the approach of leveraging vendor-specific callbacks
> > > > to handle the distinct phases of the FPGA programming process.
> > > > Here's the proposed flow.
> > > >
> > > > Pre-Configuration:
> > > > A vendor-specific callback extracts the required pre-configuration
> > > > details and initializes struct fpga_image_info. This ensures that
> > > > all vendor-specific
> > >
> > > Since we need to construct the fpga_image_info, initialize multiple
> > > field as needed, I'm wondering if configfs could be a solution for the uAPI?
> > >
> >
> > A configfs uAPI isn't necessary, we can manage this using the proposed IOCTL
> flow.
> > The POC code looks as follows.
> 
> I prefer more to configfs cause it provides standard FS way to create the
> fpga_image_info object, e.g. which attributes are visible for OF/non-OF region, which
> attributes come from image blob and can only be RO, etc.
> 
> Of couse ioctl() could achieve the same goal but would add much more specific rules
> (maybe flags/types) for user to follow.
> 

Agreed. Using ConfigFS is preferable because it provides a standardized filesystem
interface for creating and managing the fpga_image_info object.

The proposed new user interface is outlined as follows:

# Mount ConfigFS filesystem
mount -t configfs none /sys/kernel/config

# Upload Configuration and Load the Bitstream for the Targeted FPGA Region.

Configuration File Upload:
Upload the configuration file containing the necessary metadata or settings required
for configuring the FPGA region. This file may vary based on the vendor and includes
important details specific to the vendor's requirements.

Vendor-Specific Callback: 
A vendor-specific callback function extracts the relevant configuration data from the file.
The format and contents of the configuration file can differ between vendors. The callback
then initializes the struct fpga_image_info, ensuring all vendor-specific requirements are
satisfied.

Device-Specific Considerations:
For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config files.
These .dtbo files contain all necessary information to populate the fpga_image_info.
For non-OF devices, a vendor specific fpga.config files are used to provide the required
data for initializing the fpga_image_info.

FPGA Configuration:
Once the configuration details are extracted and the fpga_image_info structure is initialized,
the FPGA can be programmed accordingly.

echo "config_file" > /sys/kernel/config/fpga/<region>/config


# Check the status of "region"
cat /sys/kernel/config/fpga/<region>/status

# Remove "region"
echo "remove" > /sys/kernel/config/fpga/<region>/remove

Looking forward to your feedback.

Regards,
Navakishore.

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Marco Pagani]

On 2024-11-28 02:34, Xu Yilun wrote:
> On Mon, Nov 25, 2024 at 12:26:06PM +0100, Marco Pagani wrote:
>> On 2024-11-19 05:14, Xu Yilun wrote:
>>> On Tue, Oct 29, 2024 at 02:47:34PM +0530, Nava kishore Manne wrote:
>>>> Introduces an IOCTL interface within the fpga-region subsystem,
>>>> providing a generic and standardized mechanism for configuring (or)
>>>> reprogramming FPGAs during runtime. The newly added interface supports
>>>> both OF (Open Firmware) and non-OF devices, leveraging vendor-specific
>>>> callbacks (e.g., configuration + enumeration, removal, and status) to
>>>> accommodate a wide range of device specific configurations.
>>>>
>>>> The IOCTL interface ensures compatibility with both OF and non-OF
>>>> devices, allowing for seamless FPGA reprogramming across diverse
>>>> platforms.
>>>>
>>>> Vendor-specific callbacks are integrated into the interface, enabling
>>>> custom FPGA configuration + enumeration, removal, and status reporting
>>>> mechanisms, ensuring flexibility for vendor implementations.
>>>>
>>>> This solution enhances FPGA runtime management, supporting various device
>>>> types and vendors, while ensuring compatibility with the current FPGA
>>>> configuration flow.
>>>>
>>>> Signed-off-by: Nava kishore Manne <nava.kishore.manne@amd.com>
>>>> ---
>>>> Changes for v2:
>>>>  - As discussed with Yilun, the implementation has been modified to utilize a
>>>>  callback approach, enabling seamless handling of both OF and non-OF devices.
>>>>
>>>>  - As suggested by Yilun in the POC code, we have moved away from using  void *args
>>>>  as a parameter for ICOTL inputs to obtain the required user inputs. Instead, we are
>>>>  utilizing the fpga_region_config_info structure to gather user inputs. Currently,
>>>>  this structure is implemented to support only OF devices, but we intend to extend
>>>>  it by incorporating new members to accommodate non-OF devices in the future.
>>>>
>>>>  drivers/fpga/fpga-region.c       | 110 +++++++++++++++++++++++++++++++
>>>>  drivers/fpga/of-fpga-region.c    |  91 ++++++++++++++++++++++++-
>>>>  include/linux/fpga/fpga-region.h |  32 +++++++++
>>>>  include/uapi/linux/fpga-region.h |  51 ++++++++++++++
>>>>  4 files changed, 283 insertions(+), 1 deletion(-)
>>>>  create mode 100644 include/uapi/linux/fpga-region.h
>>>>
>>>> diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
>>>> index 753cd142503e..c6bea3c99a69 100644
>>>> --- a/drivers/fpga/fpga-region.c
>>>> +++ b/drivers/fpga/fpga-region.c
>>>> @@ -8,6 +8,7 @@
>>>>  #include <linux/fpga/fpga-bridge.h>
>>>>  #include <linux/fpga/fpga-mgr.h>
>>>>  #include <linux/fpga/fpga-region.h>
>>>> +#include <linux/fpga-region.h>
>>>>  #include <linux/idr.h>
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/list.h>
>>>> @@ -180,6 +181,67 @@ static struct attribute *fpga_region_attrs[] = {
>>>>  };
>>>>  ATTRIBUTE_GROUPS(fpga_region);
>>>>  
>>>> +static int fpga_region_device_open(struct inode *inode, struct file *file)
>>>> +{
>>>> +	struct miscdevice *miscdev = file->private_data;
>>>> +	struct fpga_region *region = container_of(miscdev, struct fpga_region, miscdev);
>>>> +
>>>> +	file->private_data = region;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int fpga_region_device_release(struct inode *inode, struct file *file)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static long fpga_region_device_ioctl(struct file *file, unsigned int cmd,
>>>> +				     unsigned long arg)
>>>> +{
>>>> +	int err;
>>>> +	void __user *argp = (void __user *)arg;
>>>> +	struct fpga_region_config_info config_info;
>>>> +	struct fpga_region *region =  (struct fpga_region *)(file->private_data);
>>>> +
>>>> +	switch (cmd) {
>>>> +	case FPGA_REGION_IOCTL_LOAD:
>>>> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
>>>> +			return -EFAULT;
>>>> +
>>>> +		err = region->region_ops->region_config_enumeration(region, &config_info);
>>>> +
>>>> +		break;
>>>> +	case FPGA_REGION_IOCTL_REMOVE:
>>>> +		if (copy_from_user(&config_info, argp, sizeof(struct fpga_region_config_info)))
>>>> +			return -EFAULT;
>>>> +
>>>> +		err = region->region_ops->region_remove(region, &config_info);
>>>> +
>>>> +		break;
>>>> +	case FPGA_REGION_IOCTL_STATUS:
>>>> +		unsigned int status;
>>>> +
>>>> +		status = region->region_ops->region_status(region);
>>>> +
>>>> +		if (copy_to_user((void __user *)arg, &status, sizeof(status)))
>>>> +			err = -EFAULT;
>>>> +		break;
>>>> +	default:
>>>> +		err = -ENOTTY;
>>>> +	}
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +static const struct file_operations fpga_region_fops = {
>>>> +	.owner		= THIS_MODULE,
>>>> +	.open		= fpga_region_device_open,
>>>> +	.release	= fpga_region_device_release,
>>>> +	.unlocked_ioctl	= fpga_region_device_ioctl,
>>>> +	.compat_ioctl	= fpga_region_device_ioctl,
>>>> +};
>>>> +
>>>>  /**
>>>>   * __fpga_region_register_full - create and register an FPGA Region device
>>>>   * @parent: device parent
>>>> @@ -229,8 +291,21 @@ __fpga_region_register_full(struct device *parent, const struct fpga_region_info
>>>>  	if (ret)
>>>>  		goto err_remove;
>>>>  
>>>> +	if (info->region_ops) {
>>>> +		region->region_ops = info->region_ops;
>>>> +		region->miscdev.minor = MISC_DYNAMIC_MINOR;
>>>> +		region->miscdev.name = kobject_name(&region->dev.kobj);
>>>> +		region->miscdev.fops = &fpga_region_fops;
>>>> +		ret = misc_register(&region->miscdev);
>>>> +		if (ret) {
>>>> +			pr_err("fpga-region: failed to register misc device.\n");
>>>> +			goto err_remove;
>>>> +		}
>>>> +	}
>>>> +
>>>>  	ret = device_register(&region->dev);
>>>>  	if (ret) {
>>>> +		misc_deregister(&region->miscdev);
>>>>  		put_device(&region->dev);
>>>>  		return ERR_PTR(ret);
>>>>  	}
>>>> @@ -272,6 +347,40 @@ __fpga_region_register(struct device *parent, struct fpga_manager *mgr,
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(__fpga_region_register);
>>>>  
>>>> +/**
>>>> + * __fpga_region_register_with_ops - create and register an FPGA Region device
>>>> + * with user interface call-backs.
>>>> + * @parent: device parent
>>>> + * @mgr: manager that programs this region
>>>> + * @region_ops: ops for low level FPGA region for device enumeration/removal
>>>> + * @priv: of-fpga-region private data
>>>> + * @get_bridges: optional function to get bridges to a list
>>>> + * @owner: module containing the get_bridges function
>>>> + *
>>>> + * This simple version of the register function should be sufficient for most users.
>>>> + * The fpga_region_register_full() function is available for users that need to
>>>> + * pass additional, optional parameters.
>>>> + *
>>>> + * Return: struct fpga_region or ERR_PTR()
>>>> + */
>>>> +struct fpga_region *
>>>> +__fpga_region_register_with_ops(struct device *parent, struct fpga_manager *mgr,
>>>> +				const struct fpga_region_ops *region_ops,
>>>> +				void *priv,
>>>> +				int (*get_bridges)(struct fpga_region *),
>>>> +				struct module *owner)
>>>> +{
>>>> +	struct fpga_region_info info = { 0 };
>>>> +
>>>> +	info.mgr = mgr;
>>>> +	info.priv = priv;
>>>> +	info.get_bridges = get_bridges;
>>>> +	info.region_ops = region_ops;
>>>> +
>>>> +	return __fpga_region_register_full(parent, &info, owner);
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(__fpga_region_register_with_ops);
>>>> +
>>>>  /**
>>>>   * fpga_region_unregister - unregister an FPGA region
>>>>   * @region: FPGA region
>>>> @@ -280,6 +389,7 @@ EXPORT_SYMBOL_GPL(__fpga_region_register);
>>>>   */
>>>>  void fpga_region_unregister(struct fpga_region *region)
>>>>  {
>>>> +	misc_deregister(&region->miscdev);
>>>>  	device_unregister(&region->dev);
>>>>  }
>>>>  EXPORT_SYMBOL_GPL(fpga_region_unregister);
>>>> diff --git a/drivers/fpga/of-fpga-region.c b/drivers/fpga/of-fpga-region.c
>>>> index 8526a5a86f0c..63fe56e0466f 100644
>>>> --- a/drivers/fpga/of-fpga-region.c
>>>> +++ b/drivers/fpga/of-fpga-region.c
>>>> @@ -8,6 +8,8 @@
>>>>  #include <linux/fpga/fpga-bridge.h>
>>>>  #include <linux/fpga/fpga-mgr.h>
>>>>  #include <linux/fpga/fpga-region.h>
>>>> +#include <linux/firmware.h>
>>>> +#include <linux/fpga-region.h>
>>>>  #include <linux/idr.h>
>>>>  #include <linux/kernel.h>
>>>>  #include <linux/list.h>
>>>> @@ -18,6 +20,20 @@
>>>>  #include <linux/slab.h>
>>>>  #include <linux/spinlock.h>
>>>>  
>>>> +/**
>>>> + * struct of_fpga_region_priv - Private data structure
>>>> + * image.
>>>> + * @dev:	Device data structure
>>>> + * @fw:		firmware of coeff table.
>>>> + * @path:	path of FPGA overlay image firmware file.
>>>> + * @ovcs_id:	overlay changeset id.
>>>> + */
>>>> +struct of_fpga_region_priv {
>>>> +	struct device *dev;
>>>> +	const struct firmware *fw;
>>>> +	int ovcs_id;
>>>> +};
>>>> +
>>>>  static const struct of_device_id fpga_region_of_match[] = {
>>>>  	{ .compatible = "fpga-region", },
>>>>  	{},
>>>> @@ -394,20 +410,93 @@ static struct notifier_block fpga_region_of_nb = {
>>>>  	.notifier_call = of_fpga_region_notify,
>>>>  };
>>>>  
>>>> +static int of_fpga_region_status(struct fpga_region *region)
>>>> +{
>>>> +	struct of_fpga_region_priv *ovcs = region->priv;
>>>> +
>>>> +	if (ovcs->ovcs_id)
>>>> +		return FPGA_REGION_HAS_PL;
>>>
>>> Could you help specify what is PL?
>>>
>>>> +
>>>> +	return FPGA_REGION_EMPTY;
>>>> +}
>>>> +
>>>> +static int of_fpga_region_config_enumeration(struct fpga_region *region,
>>>> +					     struct fpga_region_config_info *config_info)
>>>> +{
>>>> +	struct of_fpga_region_priv *ovcs = region->priv;
>>>> +	int err;
>>>> +
>>>> +	/* if it's set do not allow changes */
>>>> +	if (ovcs->ovcs_id)
>>>> +		return -EPERM;
>>>> +
>>>> +	err = request_firmware(&ovcs->fw, config_info->firmware_name, NULL);
>>>> +	if (err != 0)
>>>> +		goto out_err;
>>>> +
>>>> +	err = of_overlay_fdt_apply((void *)ovcs->fw->data, ovcs->fw->size,
>>>> +				   &ovcs->ovcs_id, NULL);
>>>> +	if (err < 0) {
>>>> +		pr_err("%s: Failed to create overlay (err=%d)\n",
>>>> +		       __func__, err);
>>>> +		release_firmware(ovcs->fw);
>>>> +		goto out_err;
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +
>>>> +out_err:
>>>> +	ovcs->ovcs_id = 0;
>>>> +	ovcs->fw = NULL;
>>>> +
>>>> +	return err;
>>>> +}
>>>> +
>>>> +static int of_fpga_region_config_remove(struct fpga_region *region,
>>>> +					struct fpga_region_config_info *config_info)
>>>> +{
>>>> +	struct of_fpga_region_priv *ovcs = region->priv;
>>>> +
>>>> +	if (!ovcs->ovcs_id)
>>>> +		return -EPERM;
>>>> +
>>>> +	of_overlay_remove(&ovcs->ovcs_id);
>>>> +	release_firmware(ovcs->fw);
>>>> +
>>>> +	ovcs->ovcs_id = 0;
>>>> +	ovcs->fw = NULL;
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static const struct fpga_region_ops region_ops = {
>>>> +	.region_status = of_fpga_region_status,
>>>> +	.region_config_enumeration = of_fpga_region_config_enumeration,
>>>> +	.region_remove = of_fpga_region_config_remove,
>>>> +};
>>>> +
>>>>  static int of_fpga_region_probe(struct platform_device *pdev)
>>>>  {
>>>>  	struct device *dev = &pdev->dev;
>>>>  	struct device_node *np = dev->of_node;
>>>> +	struct of_fpga_region_priv *priv;
>>>>  	struct fpga_region *region;
>>>>  	struct fpga_manager *mgr;
>>>>  	int ret;
>>>>  
>>>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +	if (!priv)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	priv->dev = dev;
>>>> +
>>>>  	/* Find the FPGA mgr specified by region or parent region. */
>>>>  	mgr = of_fpga_region_get_mgr(np);
>>>>  	if (IS_ERR(mgr))
>>>>  		return -EPROBE_DEFER;
>>>>  
>>>> -	region = fpga_region_register(dev, mgr, of_fpga_region_get_bridges);
>>>> +	region = fpga_region_register_with_ops(dev, mgr, &region_ops, priv,
>>>> +					       of_fpga_region_get_bridges);
>>>>  	if (IS_ERR(region)) {
>>>>  		ret = PTR_ERR(region);
>>>>  		goto eprobe_mgr_put;
>>>> diff --git a/include/linux/fpga/fpga-region.h b/include/linux/fpga/fpga-region.h
>>>> index 5fbc05fe70a6..3a3ba6dbb5e1 100644
>>>> --- a/include/linux/fpga/fpga-region.h
>>>> +++ b/include/linux/fpga/fpga-region.h
>>>> @@ -6,15 +6,35 @@
>>>>  #include <linux/device.h>
>>>>  #include <linux/fpga/fpga-mgr.h>
>>>>  #include <linux/fpga/fpga-bridge.h>
>>>> +#include <linux/fpga-region.h>
>>>> +#include <linux/miscdevice.h>
>>>>  
>>>>  struct fpga_region;
>>>>  
>>>> +/**
>>>> + * struct fpga_region_ops - ops for low level FPGA region ops for device
>>>> + * enumeration/removal
>>>> + * @region_status: returns the FPGA region status
>>>> + * @region_config_enumeration: Configure and enumerate the FPGA region.
>>>> + * @region_remove: Remove all devices within the FPGA region
>>>> + * (which are added as part of the enumeration).
>>>> + */
>>>> +struct fpga_region_ops {
>>>> +	int (*region_status)(struct fpga_region *region);
>>>> +	int (*region_config_enumeration)(struct fpga_region *region,
>>>> +					 struct fpga_region_config_info *config_info);
>>>
>>> My current concern is still about this combined API, it just offloads
>>> all work to low level, but we have some common flows. That's why we
>>> introduce a common FPGA reprograming API.
>>>
>>> I didn't see issue about the vendor specific pre configuration. They
>>> are generally needed to initialize the struct fpga_image_info, which
>>> is a common structure for fpga_region_program_fpga().
>>>
>>> For port IDs(AFU) inputs for DFL, I think it could also be changed
>>> (Don't have to be implemented in this patchset). Previously DFL
>>> provides an uAPI for the whole device, so it needs a port_id input to
>>> position which fpga_region within the device for programming. But now,
>>> we are introducing a per fpga_region programming interface, IIUC port_id
>>> should not be needed anymore.
>>>
>>> The combined API is truly simple for leveraging the existing
>>> of-fpga-region overlay apply mechanism. But IMHO that flow doesn't fit
>>> our new uAPI well. That flow is to adapt the generic configfs overlay
>>> interface, which comes to a dead end as you mentioned.
>>>
>>> My gut feeling for the generic programing flow should be:
>>>
>>>  1. Program the image to HW.
>>>  2. Enumerate the programmed image (apply the DT overlay)
>>>
>>> Why we have to:
>>>
>>>  1. Start enumeration.
>>>  2. On pre enumeration, programe the image.
>>>  3. Real enumeration.
>>
>> I'm currently working on an RFC to propose a rework of the fpga
>> subsystem in order to make it more aligned with the device model. One of
>> the ideas I'm experimenting with is having a bus (struct bus_type) for
>> fpga regions (devices) so that we can have region drivers that could
>> handle internal device enumeration/management whenever a new region is
>> configured on the fabric. Does this make sense in your opinions?
> 
> mm.. I didn't fully understand the need to have a region driver, what's
> the issue to solve?
> 

Sorry for the late reply. The general idea is to handle regions in a way
that is more aligned with the device model without having to resort to
extra ops and additional devices.

Having an fpga bus would allow us to handle enumeration using proper
region drivers (in the device model sense of the term, i.e., struct
device_driver) instead of derived region devices.

On second thought, I think having a reconfiguration interface at the
fpga manager level is sounder than having it at the region level (one
for each region).

With that in place, the fpga manager could request a firmware image,
parse it, write the content into the fpga configuration memory, and then
instantiate the region devices and add them to its fpga bus. Then, if
there is a match, a specific region driver can handle the enumeration
within the new region.

What do you think?

Thanks,
Marco

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

> >> I'm currently working on an RFC to propose a rework of the fpga
> >> subsystem in order to make it more aligned with the device model. One of
> >> the ideas I'm experimenting with is having a bus (struct bus_type) for
> >> fpga regions (devices) so that we can have region drivers that could
> >> handle internal device enumeration/management whenever a new region is
> >> configured on the fabric. Does this make sense in your opinions?
> > 
> > mm.. I didn't fully understand the need to have a region driver, what's
> > the issue to solve?
> > 
> 
> Sorry for the late reply. The general idea is to handle regions in a way
> that is more aligned with the device model without having to resort to
> extra ops and additional devices.
> 
> Having an fpga bus would allow us to handle enumeration using proper
> region drivers (in the device model sense of the term, i.e., struct
> device_driver) instead of derived region devices.
> 
> On second thought, I think having a reconfiguration interface at the
> fpga manager level is sounder than having it at the region level (one
> for each region).

I don't think so. A firmware image may contain enumeration info, e.g.
of-fpga-region. And I think the fpga-region should parse these
enumeration info rather than fpga manager. fpga manager should only deal
with content writing stuff and not be exposed to user.

> 
> With that in place, the fpga manager could request a firmware image,
> parse it, write the content into the fpga configuration memory, and then
> instantiate the region devices and add them to its fpga bus. Then, if

I think an fpga-region is always there no matter it is cleared, being
reprogrammed, or working. So I don't think an fpga-region needs to be
re-instantated. The sub devices inside fpga-region needs
re-instantating. That's also why I'm hesitating to fpga bus.

Thanks,
Yilun

> there is a match, a specific region driver can handle the enumeration
> within the new region.
> 
> What do you think?
> 
> Thanks,
> Marco
> 

RE: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Manne, Nava kishore]

Hi Yilun,

> -----Original Message-----
> From: Xu Yilun <yilun.xu@linux.intel.com>
> Sent: Sunday, March 19, 2023 9:08 PM
> To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org; hao.wu@intel.com;
> yilun.xu@intel.com; trix@redhat.com; robh@kernel.org; saravanak@google.com;
> linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> devicetree@vger.kernel.org
> Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime
> FPGA programming
> 
> On Thu, Dec 19, 2024 at 09:47:12AM +0000, Manne, Nava kishore wrote:
> > Hi Yilun,
> >
> > > -----Original Message-----
> > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > Sent: Tuesday, December 10, 2024 2:34 PM
> > > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org;
> > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> > > robh@kernel.org; saravanak@google.com; linux-kernel@vger.kernel.org;
> > > linux-fpga@vger.kernel.org; devicetree@vger.kernel.org
> > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface
> > > for runtime FPGA programming
> > >
> > > On Wed, Dec 04, 2024 at 06:40:18AM +0000, Manne, Nava kishore wrote:
> > > > Hi Yilun,
> > > >
> > > > > -----Original Message-----
> > > > > From: Xu Yilun <yilun.xu@linux.intel.com>
> > > > > Sent: Wednesday, November 27, 2024 7:20 AM
> > > > > To: Manne, Nava kishore <nava.kishore.manne@amd.com>
> > > > > Cc: git (AMD-Xilinx) <git@amd.com>; mdf@kernel.org;
> > > > > hao.wu@intel.com; yilun.xu@intel.com; trix@redhat.com;
> > > > > robh@kernel.org; saravanak@google.com;
> > > > > linux-kernel@vger.kernel.org; linux-fpga@vger.kernel.org;
> > > > > devicetree@vger.kernel.org
> > > > > Subject: Re: [RFC v2 1/1] fpga-region: Add generic IOCTL
> > > > > interface for runtime FPGA programming
> > > > >
> > > > > > > > + * struct fpga_region_ops - ops for low level FPGA region
> > > > > > > > +ops for device
> > > > > > > > + * enumeration/removal
> > > > > > > > + * @region_status: returns the FPGA region status
> > > > > > > > + * @region_config_enumeration: Configure and enumerate
> > > > > > > > +the FPGA
> > > region.
> > > > > > > > + * @region_remove: Remove all devices within the FPGA
> > > > > > > > +region
> > > > > > > > + * (which are added as part of the enumeration).
> > > > > > > > + */
> > > > > > > > +struct fpga_region_ops {
> > > > > > > > +	int (*region_status)(struct fpga_region *region);
> > > > > > > > +	int (*region_config_enumeration)(struct fpga_region *region,
> > > > > > > > +					 struct fpga_region_config_info
> > > *config_info);
> > > > > > >
> > > > > > > My current concern is still about this combined API, it just
> > > > > > > offloads all work to low level, but we have some common flows.
> > > > > > > That's why we introduce a common FPGA reprograming API.
> > > > > > >
> > > > > > > I didn't see issue about the vendor specific pre configuration.
> > > > > > > They are generally needed to initialize the struct
> > > > > > > fpga_image_info, which is a common structure for
> > > fpga_region_program_fpga().
> > > > > > >
> > > > > > > For port IDs(AFU) inputs for DFL, I think it could also be
> > > > > > > changed (Don't have to be implemented in this patchset).
> > > > > > > Previously DFL provides an uAPI for the whole device, so it
> > > > > > > needs a port_id input to position which fpga_region within
> > > > > > > the device for programming. But now, we are introducing a
> > > > > > > per fpga_region programming interface, IIUC port_id
> > > > > should not be needed anymore.
> > > > > > >
> > > > > > > The combined API is truly simple for leveraging the existing
> > > > > > > of-fpga-region overlay apply mechanism. But IMHO that flow
> > > > > > > doesn't fit our new uAPI well. That flow is to adapt the
> > > > > > > generic configfs overlay interface, which comes to a dead end as you
> mentioned.
> > > > > > >
> > > > > > > My gut feeling for the generic programing flow should be:
> > > > > > >
> > > > > > >  1. Program the image to HW.
> > > > > > >  2. Enumerate the programmed image (apply the DT overlay)
> > > > > > >
> > > > > > > Why we have to:
> > > > > > >
> > > > > > >  1. Start enumeration.
> > > > > > >  2. On pre enumeration, programe the image.
> > > > > > >  3. Real enumeration.
> > > > > > >
> > > > > >
> > > > > > I agree with the approach of leveraging vendor-specific
> > > > > > callbacks to handle the distinct phases of the FPGA programming process.
> > > > > > Here's the proposed flow.
> > > > > >
> > > > > > Pre-Configuration:
> > > > > > A vendor-specific callback extracts the required
> > > > > > pre-configuration details and initializes struct
> > > > > > fpga_image_info. This ensures that all vendor-specific
> > > > >
> > > > > Since we need to construct the fpga_image_info, initialize
> > > > > multiple field as needed, I'm wondering if configfs could be a solution for the
> uAPI?
> > > > >
> > > >
> > > > A configfs uAPI isn't necessary, we can manage this using the
> > > > proposed IOCTL
> > > flow.
> > > > The POC code looks as follows.
> > >
> > > I prefer more to configfs cause it provides standard FS way to
> > > create the fpga_image_info object, e.g. which attributes are visible
> > > for OF/non-OF region, which attributes come from image blob and can only be
> RO, etc.
> > >
> > > Of couse ioctl() could achieve the same goal but would add much more
> > > specific rules (maybe flags/types) for user to follow.
> > >
> >
> > Agreed. Using ConfigFS is preferable because it provides a
> > standardized filesystem interface for creating and managing the fpga_image_info
> object.
> >
> > The proposed new user interface is outlined as follows:
> >
> > # Mount ConfigFS filesystem
> > mount -t configfs none /sys/kernel/config
> >
> > # Upload Configuration and Load the Bitstream for the Targeted FPGA Region.
> >
> > Configuration File Upload:
> > Upload the configuration file containing the necessary metadata or
> > settings required for configuring the FPGA region. This file may vary
> > based on the vendor and includes important details specific to the vendor's
> requirements.
> >
> > Vendor-Specific Callback:
> > A vendor-specific callback function extracts the relevant configuration data from the
> file.
> > The format and contents of the configuration file can differ between
> > vendors. The callback then initializes the struct fpga_image_info,
> > ensuring all vendor-specific requirements are satisfied.
> >
> > Device-Specific Considerations:
> > For Open Firmware (OF) devices, fpga.dtbo files are used instead of fpga_config
> files.
> > These .dtbo files contain all necessary information to populate the
> fpga_image_info.
> > For non-OF devices, a vendor specific fpga.config files are used to
> > provide the required data for initializing the fpga_image_info.
> 
> non-OF fpga images usually don't contain fpga_image_info data (e.g.
> enable/disable_timeout_us). I think we don't have to force users embed these data in
> fpga image, provide additional configfs attributes to input these data is possible. For
> some FPGA regions (e.g. OF), these attributes could be RO, some could be RW,
> depends on different FPGA region drivers.
> 
> So I think we may have a Configuration File Upload interface, like:
> 
>   echo "config_file" > /sys/kernel/config/fpga/<region>/image
> 
> Some additional parameter interfaces, like:
> 
>   echo 10000 > /sys/kernel/config/fpga/<region>/enable_timeout
>   ...
> 
> And a Configuration interface, like:
> 
>   # programming
>   echo 1 > /sys/kernel/config/fpga/<region>/config
>   # removing
>   echo 0 > /sys/kernel/config/fpga/<region>/config
> 
> How do you think?
> 

I agree and am actively working on the POC changes.
I will submit the RFC patches at the earliest.

Regards,
Navakishore.

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Marco Pagani]

On 06/02/25 07:04, Xu Yilun wrote:
>>>> I'm currently working on an RFC to propose a rework of the fpga
>>>> subsystem in order to make it more aligned with the device model. One of
>>>> the ideas I'm experimenting with is having a bus (struct bus_type) for
>>>> fpga regions (devices) so that we can have region drivers that could
>>>> handle internal device enumeration/management whenever a new region is
>>>> configured on the fabric. Does this make sense in your opinions?
>>>
>>> mm.. I didn't fully understand the need to have a region driver, what's
>>> the issue to solve?
>>>
>>
>> Sorry for the late reply. The general idea is to handle regions in a way
>> that is more aligned with the device model without having to resort to
>> extra ops and additional devices.
>>
>> Having an fpga bus would allow us to handle enumeration using proper
>> region drivers (in the device model sense of the term, i.e., struct
>> device_driver) instead of derived region devices.
>>
>> On second thought, I think having a reconfiguration interface at the
>> fpga manager level is sounder than having it at the region level (one
>> for each region).
> 
> I don't think so. A firmware image may contain enumeration info, e.g.
> of-fpga-region. And I think the fpga-region should parse these
> enumeration info rather than fpga manager. fpga manager should only deal
> with content writing stuff and not be exposed to user.

I agree with that. In my proposal, the fpga manager should be
responsible only for writing the image into the configuration memory
and allocating region devices. In-region enumeration should be handled by
the region drivers.

My worry with having one reconfiguration interface for each region is
that it does not reflect how the hardware works. To my knowledge, all
major FPGA implementations use a DMA engine (controlled by the fpga
manager) that performs the reconfiguration through a single port. So,
having one interface per region might be conceptually confusing and give
the impression that it is possible to configure regions independently in
parallel.

>> With that in place, the fpga manager could request a firmware image,
>> parse it, write the content into the fpga configuration memory, and then
>> instantiate the region devices and add them to its fpga bus. Then, if
> 
> I think an fpga-region is always there no matter it is cleared, being
> reprogrammed, or working. So I don't think an fpga-region needs to be
> re-instantated. The sub devices inside fpga-region needs
> re-instantating. That's also why I'm hesitating to fpga bus.

I think one of the issues with the current subsystem architecture is
that it coalesces two cases: full and partial images. With partial
images, it makes sense to keep the region devices and rerun the internal
enumeration. With full images, I believe it makes sense to clear and
reallocate new devices to set a new region tree.
 
> Thanks,
> Yilun
> 
>> there is a match, a specific region driver can handle the enumeration
>> within the new region.
>>
>> What do you think?

Thanks,
Marco

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

On Mon, Feb 17, 2025 at 04:18:36PM +0100, Marco Pagani wrote:
> 
> 
> On 06/02/25 07:04, Xu Yilun wrote:
> >>>> I'm currently working on an RFC to propose a rework of the fpga
> >>>> subsystem in order to make it more aligned with the device model. One of
> >>>> the ideas I'm experimenting with is having a bus (struct bus_type) for
> >>>> fpga regions (devices) so that we can have region drivers that could
> >>>> handle internal device enumeration/management whenever a new region is
> >>>> configured on the fabric. Does this make sense in your opinions?
> >>>
> >>> mm.. I didn't fully understand the need to have a region driver, what's
> >>> the issue to solve?
> >>>
> >>
> >> Sorry for the late reply. The general idea is to handle regions in a way
> >> that is more aligned with the device model without having to resort to
> >> extra ops and additional devices.
> >>
> >> Having an fpga bus would allow us to handle enumeration using proper
> >> region drivers (in the device model sense of the term, i.e., struct
> >> device_driver) instead of derived region devices.
> >>
> >> On second thought, I think having a reconfiguration interface at the
> >> fpga manager level is sounder than having it at the region level (one
> >> for each region).
> > 
> > I don't think so. A firmware image may contain enumeration info, e.g.
> > of-fpga-region. And I think the fpga-region should parse these
> > enumeration info rather than fpga manager. fpga manager should only deal
> > with content writing stuff and not be exposed to user.
> 
> I agree with that. In my proposal, the fpga manager should be
> responsible only for writing the image into the configuration memory
> and allocating region devices. In-region enumeration should be handled by
> the region drivers.
> 
> My worry with having one reconfiguration interface for each region is
> that it does not reflect how the hardware works. To my knowledge, all
> major FPGA implementations use a DMA engine (controlled by the fpga
> manager) that performs the reconfiguration through a single port. So,
> having one interface per region might be conceptually confusing and give
> the impression that it is possible to configure regions independently in
> parallel.

One interface per region means the regions could be independently
reprogrammed, i.e. reprogramming of one region won't affect the working
of another region. But they don't have to be reprogrammed in parallel.
If it cannot be reprogrammed now, the interface call could fail.

> 
> >> With that in place, the fpga manager could request a firmware image,
> >> parse it, write the content into the fpga configuration memory, and then
> >> instantiate the region devices and add them to its fpga bus. Then, if
> > 
> > I think an fpga-region is always there no matter it is cleared, being
> > reprogrammed, or working. So I don't think an fpga-region needs to be
> > re-instantated. The sub devices inside fpga-region needs
> > re-instantating. That's also why I'm hesitating to fpga bus.
> 
> I think one of the issues with the current subsystem architecture is
> that it coalesces two cases: full and partial images. With partial
> images, it makes sense to keep the region devices and rerun the internal
> enumeration. With full images, I believe it makes sense to clear and
> reallocate new devices to set a new region tree.

MM.. I don't actually understand what's the fundamental differences
between full & partial. If a full region supports full & partial
reconfiguration, the full region contains partial regions as
sub-devices. The partial reconfiguration reallocates it sub-devices and
won't change partial region itself. The full reconfiguration also
reallocates it sub-devices including partial regions, and won't change
full region itself.

Thanks,
Yilun

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Marco Pagani]

On 2025-03-01 10:27, Xu Yilun wrote:
> On Mon, Feb 17, 2025 at 04:18:36PM +0100, Marco Pagani wrote:
>>
>>
>> On 06/02/25 07:04, Xu Yilun wrote:
>>>>>> I'm currently working on an RFC to propose a rework of the fpga
>>>>>> subsystem in order to make it more aligned with the device model. One of
>>>>>> the ideas I'm experimenting with is having a bus (struct bus_type) for
>>>>>> fpga regions (devices) so that we can have region drivers that could
>>>>>> handle internal device enumeration/management whenever a new region is
>>>>>> configured on the fabric. Does this make sense in your opinions?
>>>>>
>>>>> mm.. I didn't fully understand the need to have a region driver, what's
>>>>> the issue to solve?
>>>>>
>>>>
>>>> Sorry for the late reply. The general idea is to handle regions in a way
>>>> that is more aligned with the device model without having to resort to
>>>> extra ops and additional devices.
>>>>
>>>> Having an fpga bus would allow us to handle enumeration using proper
>>>> region drivers (in the device model sense of the term, i.e., struct
>>>> device_driver) instead of derived region devices.
>>>>
>>>> On second thought, I think having a reconfiguration interface at the
>>>> fpga manager level is sounder than having it at the region level (one
>>>> for each region).
>>>
>>> I don't think so. A firmware image may contain enumeration info, e.g.
>>> of-fpga-region. And I think the fpga-region should parse these
>>> enumeration info rather than fpga manager. fpga manager should only deal
>>> with content writing stuff and not be exposed to user.
>>
>> I agree with that. In my proposal, the fpga manager should be
>> responsible only for writing the image into the configuration memory
>> and allocating region devices. In-region enumeration should be handled by
>> the region drivers.
>>
>> My worry with having one reconfiguration interface for each region is
>> that it does not reflect how the hardware works. To my knowledge, all
>> major FPGA implementations use a DMA engine (controlled by the fpga
>> manager) that performs the reconfiguration through a single port. So,
>> having one interface per region might be conceptually confusing and give
>> the impression that it is possible to configure regions independently in
>> parallel.
> 
> One interface per region means the regions could be independently
> reprogrammed, i.e. reprogramming of one region won't affect the working
> of another region. But they don't have to be reprogrammed in parallel.
> If it cannot be reprogrammed now, the interface call could fail.

Good point. However, I still have some other practical concerns. To the
best of my knowledge, reconfigurable images/bitstreams are statically
built for a specific reconfigurable region in current FPGA
implementations. So, what should happen if the user feeds the wrong
image (e.g., an image targeting another region) into a reconfigurable
region programming interface? I don't think the fpga manager could and
should detect these mistakes since the kernel has no visibility of the
FPGA configuration memory.

>>>> With that in place, the fpga manager could request a firmware image,
>>>> parse it, write the content into the fpga configuration memory, and then
>>>> instantiate the region devices and add them to its fpga bus. Then, if
>>>
>>> I think an fpga-region is always there no matter it is cleared, being
>>> reprogrammed, or working. So I don't think an fpga-region needs to be
>>> re-instantated. The sub devices inside fpga-region needs
>>> re-instantating. That's also why I'm hesitating to fpga bus.
>>
>> I think one of the issues with the current subsystem architecture is
>> that it coalesces two cases: full and partial images. With partial
>> images, it makes sense to keep the region devices and rerun the internal
>> enumeration. With full images, I believe it makes sense to clear and
>> reallocate new devices to set a new region tree.
> 
> MM.. I don't actually understand what's the fundamental differences
> between full & partial. If a full region supports full & partial
> reconfiguration, the full region contains partial regions as
> sub-devices. The partial reconfiguration reallocates it sub-devices and
> won't change partial region itself. The full reconfiguration also
> reallocates it sub-devices including partial regions, and won't change
> full region itself.

What I had in mind was re-instating a new region device in case of
partial reconfiguration to re-trigger the enumeration by a region driver
of the devices hosted in the region.

At this point, I believe I should send an RFC to have something concrete
to discuss. Unfortunately, it may take me some time due to a job change.

Thanks,
Marco

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Xu Yilun]

On Sun, Mar 16, 2025 at 10:55:07PM +0100, Marco Pagani wrote:
> 
> On 2025-03-01 10:27, Xu Yilun wrote:
> > On Mon, Feb 17, 2025 at 04:18:36PM +0100, Marco Pagani wrote:
> >>
> >>
> >> On 06/02/25 07:04, Xu Yilun wrote:
> >>>>>> I'm currently working on an RFC to propose a rework of the fpga
> >>>>>> subsystem in order to make it more aligned with the device model. One of
> >>>>>> the ideas I'm experimenting with is having a bus (struct bus_type) for
> >>>>>> fpga regions (devices) so that we can have region drivers that could
> >>>>>> handle internal device enumeration/management whenever a new region is
> >>>>>> configured on the fabric. Does this make sense in your opinions?
> >>>>>
> >>>>> mm.. I didn't fully understand the need to have a region driver, what's
> >>>>> the issue to solve?
> >>>>>
> >>>>
> >>>> Sorry for the late reply. The general idea is to handle regions in a way
> >>>> that is more aligned with the device model without having to resort to
> >>>> extra ops and additional devices.
> >>>>
> >>>> Having an fpga bus would allow us to handle enumeration using proper
> >>>> region drivers (in the device model sense of the term, i.e., struct
> >>>> device_driver) instead of derived region devices.
> >>>>
> >>>> On second thought, I think having a reconfiguration interface at the
> >>>> fpga manager level is sounder than having it at the region level (one
> >>>> for each region).
> >>>
> >>> I don't think so. A firmware image may contain enumeration info, e.g.
> >>> of-fpga-region. And I think the fpga-region should parse these
> >>> enumeration info rather than fpga manager. fpga manager should only deal
> >>> with content writing stuff and not be exposed to user.
> >>
> >> I agree with that. In my proposal, the fpga manager should be
> >> responsible only for writing the image into the configuration memory
> >> and allocating region devices. In-region enumeration should be handled by
> >> the region drivers.
> >>
> >> My worry with having one reconfiguration interface for each region is
> >> that it does not reflect how the hardware works. To my knowledge, all
> >> major FPGA implementations use a DMA engine (controlled by the fpga
> >> manager) that performs the reconfiguration through a single port. So,
> >> having one interface per region might be conceptually confusing and give
> >> the impression that it is possible to configure regions independently in
> >> parallel.
> > 
> > One interface per region means the regions could be independently
> > reprogrammed, i.e. reprogramming of one region won't affect the working
> > of another region. But they don't have to be reprogrammed in parallel.
> > If it cannot be reprogrammed now, the interface call could fail.
> 
> Good point. However, I still have some other practical concerns. To the
> best of my knowledge, reconfigurable images/bitstreams are statically
> built for a specific reconfigurable region in current FPGA
> implementations. So, what should happen if the user feeds the wrong
> image (e.g., an image targeting another region) into a reconfigurable
> region programming interface? I don't think the fpga manager could and
> should detect these mistakes since the kernel has no visibility of the

I think fpga manager could do something with the helper of HW and image
builder. If the region identifier could be read out from some HW
registers on fpga_region enumeration, and it is also built into the
image header, the fpga manager could check the validity.

Some reprograming engine even enforce the validation by requiring SW
input region identifier along with the image data.

Without the help of HW, I cannot see any SW solution could help, one
interface per region or one interface per engine.

Thanks,
Yilun

Re: [RFC v2 1/1] fpga-region: Add generic IOCTL interface for runtime FPGA programming

[Arnd Bergmann]

> + * FPGA Region Control IOCTLs.
> + */
> +#define FPGA_REGION_MAGIC	'f'
> +#define FPGA_IOW(num, dtype)	_IOW(FPGA_REGION_MAGIC, num, dtype)
> +#define FPGA_IOR(num, dtype)	_IOR(FPGA_REGION_MAGIC, num, dtype)
> +
> +#define FPGA_REGION_IOCTL_LOAD		FPGA_IOW(0, __u32)
> +#define FPGA_REGION_IOCTL_REMOVE        FPGA_IOW(1, __u32)
> +#define FPGA_REGION_IOCTL_STATUS        FPGA_IOR(2, __u32)

The definition does not appear to match the usage in the driver,
since you don't pass a __u32 structure but instead a 
fpga_region_config_info.

Please also remove the extra FPGA_IOW/FPGA_IOR macros and just use
_IOW/IOR directly so it is possible to process the headers when
identifying ioctl command codes.

The 'f' range seems to be rather overloaded already with filesystem
ioctls:

'f'   00-1F  linux/ext2_fs.h                                         conflict!
'f'   00-1F  linux/ext3_fs.h                                         conflict!
'f'   00-0F  fs/jfs/jfs_dinode.h                                     conflict!
'f'   00-0F  fs/ext4/ext4.h                                          conflict!
'f'   00-0F  linux/fs.h                                              conflict!
'f'   00-0F  fs/ocfs2/ocfs2_fs.h                                     conflict!

In particular, the numbers you have defined are very similar to these:
some of these:

#define FS_IOC_GETFLAGS                 _IOR('f', 1, long)
#define FS_IOC_SETFLAGS                 _IOW('f', 2, long)

      Arnd