| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<DS7PR12MB6070BC635C899C637EDE71F3CD7CA@DS7PR12MB6070.namprd12.prod.outlook.com>
Date: Fri, 20 Jun 2025 11:15:11 +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,
> -----Original Message-----
> From: Xu Yilun <yilun.xu@...ux.intel.com>
> Sent: Friday, June 13, 2025 3:40 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 Thu, Jun 12, 2025 at 09:05:21AM +0000, Manne, Nava kishore wrote:
> > [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 = ®ion->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
>
> FPGA region is the device driver generated kernel object, so this per fpga_region
> group should be created by kernel, not userspace. User should create fpga_image
> config_item under region group.
>
> mkdir /configfs/fpga_region/region0/my_image
>
> >
> > # 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
>
> My idea is, when an image item is first created, it is not activated, user needs to
> read/write its attributes to initialize it, then we could have an attribute (e.g. enable) to
> activate/reprograme the image.
>
> For example:
>
> echo 1 > /configfs/fpga_regions/region0/my_image/enable
>
> >
> > # 6. Unload/Reset the Programmable Logic echo 0 >
> > /configfs/fpga_regions/region0/config
>
> Maybe we could just delete the image item for unloading
>
> rmdir /configfs/fpga_region/region0/my_image
>
I’ve implemented the FPGA Region ConfigFS interface with the following hierarchy:
/configfs
└── fpga_regions ← Registered via configfs_register_subsystem()
└── region0 ← Added using configfs_add_default_group()
└── my_image ← Created via mkdir from userspace
├── firmware ← Write firmware name here
└── config ← Trigger programming/unloading
Observation:
If configfs is not mounted before configfs_add_default_group() is invoked
(e.g., when regions are registered early via base DTB), the path
/configfs/fpga_regions/region0 does not appear in userspace,
even though it’s properly initialized in the kernel.
This appears to be due to how default groups function.
they require the configfs filesystem to be mounted prior to the group
addition in order to be visible. As a result, the mount order becomes
a strict dependency, which may affect or break early-boot FPGA flows
where regions are created before configfs is available.
Proposal:
Use configfs_register_subsystem(®ion->cfg_subsys) for each FPGA region
instead of relying on configfs_add_default_group().
This approach places each FPGA region directly under /configfs/region0,
avoiding the timing issues associated with default groups.
The interface becomes available as soon as configfs is mounted.
regardless of when the region was registered
(boot time via base DTB or dynamically via overlays).
New user hierarchy:
/configfs
└── region0 ← Region appears as its own root node
└── my_image ← Created via mkdir from userspace
├── firmware ← Write firmware name here
└── config ← Trigger programming/unloading
Would like to know if this approach looks good, or if there are better
suggestions to handle this scenario?
Regards,
Navakishore.
Powered by blists - more mailing lists