Message-ID:
 <DS7PR12MB607015CAB2781E16A4EC110ECD74A@DS7PR12MB6070.namprd12.prod.outlook.com>
Date: Thu, 12 Jun 2025 09:05:21 +0000
From: "Manne, Nava kishore" <nava.kishore.manne@....com>
To: Xu Yilun <yilun.xu@...ux.intel.com>
CC: "mdf@...nel.org" <mdf@...nel.org>, "hao.wu@...el.com" <hao.wu@...el.com>,
	"yilun.xu@...el.com" <yilun.xu@...el.com>, "trix@...hat.com"
	<trix@...hat.com>, "robh@...nel.org" <robh@...nel.org>,
	"saravanak@...gle.com" <saravanak@...gle.com>, "linux-fpga@...r.kernel.org"
	<linux-fpga@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "git (AMD-Xilinx)" <git@....com>
Subject: RE: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for
 runtime FPGA configuration

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Yilun,

        Thanks for quick response.
Please find my response inline.

> -----Original Message-----
> From: Xu Yilun <yilun.xu@...ux.intel.com>
> Sent: Sunday, June 1, 2025 8:32 PM
> To: Manne, Nava kishore <nava.kishore.manne@....com>
> Cc: mdf@...nel.org; hao.wu@...el.com; yilun.xu@...el.com; trix@...hat.com;
> robh@...nel.org; saravanak@...gle.com; linux-fpga@...r.kernel.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org; git (AMD-Xilinx)
> <git@....com>
> Subject: Re: [RFC v3 1/1] fpga-region: Introduce ConfigFS interface for runtime
> FPGA configuration
>
> On Mon, May 19, 2025 at 09:09:37AM +0530, Nava kishore Manne wrote:
> > Introduces an ConfigFS 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., pre_config, post_config, removal, and status) to
> > accommodate a wide range of device specific configurations.
> >
> > The ConfigFS 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 pre_config, post_config, 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@....com>
> > ---
> > Changes for v3:
> >  - As discussed with Yilun, the implementation continues to use a
> > callback-based  approach to seamlessly support both OF (Open Firmware)
> > and non-OF devices via  vendor-specific hooks. Additionally, the
> > earlier IOCTL-based interface has been  replaced with a more suitable
> > ConfigFS-based mechanism to enable runtime FPGA  configuration.
> >
> > 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       | 196 +++++++++++++
> >  drivers/fpga/of-fpga-region.c    | 474 +++++++++++++++++--------------
> >  include/linux/fpga/fpga-region.h |  34 +++
> >  3 files changed, 493 insertions(+), 211 deletions(-)
> >
> > diff --git a/drivers/fpga/fpga-region.c b/drivers/fpga/fpga-region.c
> > index 753cd142503e..d583fc22955b 100644
> > --- a/drivers/fpga/fpga-region.c
> > +++ b/drivers/fpga/fpga-region.c
> > @@ -5,6 +5,7 @@
> >   *  Copyright (C) 2013-2016 Altera Corporation
> >   *  Copyright (C) 2017 Intel Corporation
> >   */
> > +#include <linux/configfs.h>
> >  #include <linux/fpga/fpga-bridge.h>
> >  #include <linux/fpga/fpga-mgr.h>
> >  #include <linux/fpga/fpga-region.h>
> > @@ -180,6 +181,158 @@ static struct attribute *fpga_region_attrs[] = {
> > };  ATTRIBUTE_GROUPS(fpga_region);
> >
> > +static struct fpga_region *item_to_fpga_region(struct config_item
> > +*item) {
> > +   return container_of(to_configfs_subsystem(to_config_group(item)),
> > +                       struct fpga_region, subsys);
> > +}
> > +
> > +/**
> > + * fpga_region_image_store - Set firmware image name for FPGA region
> > + * This function sets the firmware image name for an FPGA region through
> configfs.
> > + * @item: Configfs item representing the FPGA region
> > + * @buf: Input buffer containing the firmware image name
> > + * @count: Size of the input buffer
> > + *
> > + * Return: Number of bytes written on success, or negative errno on failure.
> > + */
> > +static ssize_t fpga_region_image_store(struct config_item *item,
> > +const char *buf, size_t count) {
> > +   struct fpga_region *region = item_to_fpga_region(item);
> > +   struct device *dev = &region->dev;
> > +   struct fpga_image_info *info;
> > +   char firmware_name[NAME_MAX];
> > +   char *s;
> > +
> > +   if (region->info) {
> > +           dev_err(dev, "Region already has already configured.\n");
> > +           return -EINVAL;
> > +   }
> > +
> > +   info = fpga_image_info_alloc(dev);
> > +   if (!info)
> > +           return -ENOMEM;
> > +
> > +   /* copy to path buffer (and make sure it's always zero terminated */
> > +   count = snprintf(firmware_name, sizeof(firmware_name) - 1, "%s", buf);
> > +   firmware_name[sizeof(firmware_name) - 1] = '\0';
> > +
> > +   /* strip trailing newlines */
> > +   s = firmware_name + strlen(firmware_name);
> > +   while (s > firmware_name && *--s == '\n')
> > +           *s = '\0';
> > +
> > +   region->firmware_name = devm_kstrdup(dev, firmware_name,
> GFP_KERNEL);
> > +   if (!region->firmware_name)
> > +           return -ENOMEM;
> > +
> > +   region->info = info;
> > +
> > +   return count;
> > +}
> > +
> > +/**
> > + * fpga_region_config_store - Trigger FPGA configuration via configfs
> > + * @item: Configfs item representing the FPGA region
> > + * @buf: Input buffer containing the configuration command (expects
> > +"1" to program, "0" to remove)
> > + * @count: Size of the input buffer
> > + *
> > + * If the input is "1", this function performs:
> > + *   1. region_pre_config() (if defined)
>
> Please define explicit workflow, and explicit expectation for each callback, or this
> framework makes no sense. From your of-fpga-region implementation, seems
> pre_config() means "parse image", post_config() means "populate devices".
>
> > + *   2. Bitstream programming via fpga_region_program_fpga() (unless external
> config flag is set)
> > + *   3. region_post_config() (if defined)
> > + *
> > + * If the input is "0", it triggers region_remove() (if defined).
> > + *
> > + * Return: Number of bytes processed on success, or negative errno on failure.
>
> Please put the uAPI description in Documentation/ABI/testing. Then we could know
> the file path layout from userspace POV.
>
> > + */
> > +static ssize_t fpga_region_config_store(struct config_item *item,
> > +                                   const char *buf, size_t count)
> > +{
> > +   struct fpga_region *region = item_to_fpga_region(item);
> > +   int config_value, ret = 0;
> > +
> > +   /* Parse input: must be "0" or "1" */
> > +   if (kstrtoint(buf, 10, &config_value) || (config_value != 0 && config_value !=
> 1))
> > +           return -EINVAL;
> > +
> > +   /* Ensure fpga_image_info is available */
> > +   if (!region->info)
> > +           return -EINVAL;
> > +
> > +   if (config_value == 1) {
> > +           /* Pre-config */
> > +           if (region->region_ops->region_pre_config) {
> > +                   ret = region->region_ops->region_pre_config(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +
> > +           /* Program bitstream if not external */
> > +           if (!(region->info->flags & FPGA_MGR_EXTERNAL_CONFIG)) {
> > +                   ret = fpga_region_program_fpga(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +
> > +           /* Post-config */
> > +           if (region->region_ops->region_post_config) {
> > +                   ret = region->region_ops->region_post_config(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +
> > +   } else {
> > +           /* Remove configuration */
> > +           if (region->region_ops->region_remove) {
> > +                   ret = region->region_ops->region_remove(region);
> > +                   if (ret)
> > +                           return ret;
> > +           }
> > +   }
> > +
> > +   return count;
> > +}
> > +
> > +/* Define Attributes */
> > +CONFIGFS_ATTR_WO(fpga_region_, image);
> CONFIGFS_ATTR_WO(fpga_region_,
> > +config);
> > +
> > +/* Attribute List */
> > +static struct configfs_attribute *fpga_region_config_attrs[] = {
> > +   &fpga_region_attr_image,
> > +   &fpga_region_attr_config,
> > +   NULL,
> > +};
> > +
> > +/* ConfigFS Item Type */
> > +static const struct config_item_type fpga_region_item_type = {
> > +   .ct_attrs = fpga_region_config_attrs,
> > +   .ct_owner = THIS_MODULE,
> > +};
>
> I think this is still the sysfs methodology. My understanding from configfs.rst is, use
> userspace interfaces to control the lifecycle of a kernel object.
>
> Now for existing kernel reprogramming flow, the image object for fpga_region is the
> struct fpga_image_info. We need to associate the struct with a config_item: alloc the
> struct fpga_image_info instance by mkdir, expose necessary fields
> (enable_timeout_us, disable_timeout_us, firmware_name, and the most important for
> of-fpga-region - overlay blob ...) for user to fill/query via configfs attributes. And finally
> use a writeable attribute (e.g. load) to trigger fpga_region_program_fpga().
>

Thanks for the inputs. We've now implemented a ConfigFS-based reprogramming
interface for fpga_region that aligns with the intended lifecycle-driven model
described in configfs.rst.

New Interface Usage Instructions:
# 1. Mount configfs (if not already mounted)
mkdir -p /configfs
mount -t configfs configfs /configfs

# 2. Create a region directory (e.g., region0)
mkdir /configfs/fpga_regions/region0

# 3. Copy your bitstream or overlay files to firmware path
cp pl.dtbo /lib/firmware/

# 4. Set the firmware name (e.g., overlay .dtbo)
echo pl.dtbo > /configfs/fpga_regions/region0/image

# 5. Trigger Programming/reprogramming
echo 1 > /configfs/fpga_regions/region0/config

# 6. Unload/Reset the Programmable Logic
echo 0 > /configfs/fpga_regions/region0/config

Summary of Changes:
->Lifecycle Management:
    ->On mkdir /configfs/fpga_regions/regionX, we allocate and associate
       a struct fpga_image_info to the corresponding fpga_region.
    ->On rmdir, we clean up the associated image info via the new
       fpga_regions_drop_item() callback.
->Device Lookup:
    ->class_find_device_by_name() is now used in fpga_regions_make_item()
       to map a region name (e.g., region0) directly to a registered
       fpga_region device.

Please share your thoughts on this approach.

Regards,
Navakishore.

Powered by blists - more mailing lists