Ubuntu Core enablement changes

[artiepoole]

In an ongoing attempt to enable fpga device functionality in Ubuntu Core, three issues have arisen in relation to the requirement for snaps to be strictly confined. In our case, we are attempting to redistribute dfx-mgr (and libdfx) as part of FPGAd. Once the concept of "softeners" is in place, this snap will enable downstream customers to use dfx-mgr as they expect to be able to, but with the caveat that FPGAd will be the gatekeeper for accessing the FPGA subsystems.

In these efforts, we found three challenges in the way dfx-mgr and libdfx operate:

1. both parts of this application write to a file called state.txt. In real operation, this file is created at /state.txt which is not allowed in a strictly confined snap, and no layout can be applied to files in the root directory, except as files inside known pre-defined directories.
2. files are freely moved in and our of /lib/firmware. In a snap's environment, this directory does not exist, so accesses to it fail. Repeatedly copying is bad for performance as well.
3. the device tree overlay subsystem's files (configfs) are remounted to /configfs. This mount call requires strong permissions, which the store team can't/wont to grant to the fpgad snap.

In order to tackle these three problems, I have edited dfx-mgr and libdfx with the following solutions:

1. for libdfx, state.txt is now moved to /etc/dfx/state.txt /run/dfx/state.txt. I would actually prefer to remove the "system(command)" calls to mitigate the need for this file at all, but that is out of scope here. C stdlib io operations are used to write into internal buffers instead of relying on state.txt, generated using system calls. Several of the functions are exported for use in dfx-mgr (and similar) to save duplication.
2. Before anything which will trigger the kernel to look for firmware, instead of copying files to /lib/firmware, the containing directory is written to /sys/module/firmware_class/parameters/path instead. This is exported for use by dfx-mgr to save duplication.
3. the calls to mount /configfs are removed, and all paths pointing to "/configfs/" are replaced by /sys/kernel/config/

Please see Xilinx/dfx-mgr#12 for the dfx-mgr changes

Please do not hesitate to suggest changes or ask for clarification.

[sivadur]

@artiepoole Thanks for reporting these, we will check and get back on these

[artiepoole]

@sivadur Thank you for the haste! I created this as a draft. There have been some tweaks since creation, so please take these into account during assessment. I have now tested it more thoroughly and have marked this as "ready for review"

[talhaHavadar]

do we have any update on review @sivadur ?

[artiepoole]

Hey @sivadur,

On the dfx-mgr side of things, it was suggested that we should remove the state.txt file completely and instead do the comparisons handled by state.txt using C, instead of using the shell via system commands. If you agree, I would like to make those changes in this PR as well.
The relevant commit is here

[arthokal]

Hi @artiepoole ,

Agree with removing dependency on state.txt file in libdfx and directly reading contents of /sys/class/fpga_manager/fpga0/state using file handling in C.

[artiepoole]

Thanks @arthokal, I'll go ahead and implement this then

Is there any objection to adding DFX_DBG and DFX_ERR equivalent macros to libdfx? Currently all messages are printed with printf.

[artiepoole]

To save you having to explore:

DFX_DBG example:
DFX_DBG("Found base design %s", base_designs[i].base_path); becomes DFX-MGRD> findBaseDesign_path():195 Found base design /lib/firmware/xilinx/k26-starter-kits

DFX_ERR example:
DFX_ERR("Failed to close `%s` after writing", path); became DFX-MGRD> ERROR:write_string_to_file():157 Failed to close `/sys/class/fpga_manager/fpga0/firmware` after writing: No such file or directory

Current libdfx print example:
printf("dfx_set_firmware_lookup_path: Setting lookup path for `%s`\n", file_path); of course prints like dfx_set_firmware_lookup_path: Setting lookup path for `/lib/firmware/xilinx/k24-starter-kits/k24_starter_kits.bit.bin

[arthokal]

@artiepoole ,

Regarding addition of DFX_DBG and DFX_ERR equivalent macros to libdfx, we can take these in a separate PR.

[artiepoole]

@arthokal @sivadur,

Please be aware that I am unable to test dfx_cfg_drivers_load since I do not have a package containing a driver to test with.

I have made the changes to remove the need for state.txt. I have tested to the best of my ability the functionality of the dfx-mgr's -load, -o, -b, remove and -listPackage commands. Please review/comment when possible.

P.S. dfx-mgr has been adjusted to use the functions I exposed in libdfx.h to save duplication

[artiepoole]

@arthokal

Thanks for spotting those mistakes and missing README changes.
I amended the commit to fix the docstrings and have created a new commit containing the README updates.
Please review the docs/README.txt when you can. I kept the usage examples very minimal to match the surrounding. If you would like more detailed examples, please let me know.

Best,
Artie

[arthokal]

Changes looks fine to me.
@NavaKishoreManne, please review and approve if changes looks fine.

[arthokal]

With these changes, we tested libdfx application from our end including dfx_cfg_drivers_load. Testing looks good.
Approving from my end. Let @sivadur review too and approve.

Thank you,
Aravind.

[talhaHavadar]

I see all parties agreed, do we know when we would have this merged?

[arthokal]

Hi @talhaHavadar
Before merging to GitHub, we follow an internal validation process to ensure consistency across our repositories. This typically takes a week. We'll update you as soon as it's complete