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@xxxxxxxxxxxxxxx>
> Sent: Tuesday, December 10, 2024 2:34 PM
> To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> Cc: git (AMD-Xilinx) <git@xxxxxxx>; mdf@xxxxxxxxxx; hao.wu@xxxxxxxxx;
> yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx; robh@xxxxxxxxxx; saravanak@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; linux-fpga@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx>
> > > Sent: Wednesday, November 27, 2024 7:20 AM
> > > To: Manne, Nava kishore <nava.kishore.manne@xxxxxxx>
> > > Cc: git (AMD-Xilinx) <git@xxxxxxx>; mdf@xxxxxxxxxx;
> > > hao.wu@xxxxxxxxx; yilun.xu@xxxxxxxxx; trix@xxxxxxxxxx;
> > > robh@xxxxxxxxxx; saravanak@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > > linux-fpga@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx
> > > 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.