Ubuntu Core enablement changes by artiepoole · Pull Request #12 · Xilinx/dfx-mgr

artiepoole · 2025-11-13T15:47:42Z

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:

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.
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.
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:

for dfx-mgr, state.txt is now moved to ~~/etc/dfx-mgrd/state.txt~~ /run/dfx-mgrd/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.
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. The implementation is in libdfx.c/.h to avoid code duplication.
the calls to mount /configfs are removed, and all paths pointing to "/configfs/" are replaced by /sys/kernel/config/

Please see Xilinx/libdfx#6 for the libdfx changes

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

tjaysukh · 2025-11-14T04:46:33Z

@artiepoole We’ll check this and get back in a week.

artiepoole · 2025-11-17T12:30:57Z

@tjaysukh, 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"

tjaysukh · 2025-12-03T09:38:51Z

@artiepoole,
Thank you for suggesting these changes and for the detailed explanation. We appreciate the effort you’ve put into this. Here are my thoughts on your commits:
1.Move /state.txt to /run/dfx-mgrd/state.txt - Since we are already modifying the code, let's remove state.txt altogether and replace the system(command) calls with direct file I/O operations as you suggested.
2.Configfs no longer being remounted - I agree with your solution and we can adopt this approach.
3.Stops copying files into /lib/firmware - Since this commit is dependent on libdfx (specifically the dfx_set_firmware_lookup_path() function). We can incorporate this change only after the updates are integrated into libdfx.

Please let me know your thoughts.

artiepoole · 2025-12-03T15:45:21Z

@tjaysukh

Thanks for the feedback.

On point 1), yes, I am happy to implement the direct I/O. I'll do in a follow-up commit. I will also adopt that for libdfx as well, if the responsible team agrees.

I also agree with point 3) about waiting for libdfx before merging.

I'll mark this PR as draft until the direct IO is implemented and update you here when I think it is ready for review.
Please don't hesitate to suggest any other modifications to my suggested changes.

Best,
Artie

artiepoole · 2025-12-05T10:02:11Z

Hey @tjaysukh

Please see the latest commit for changes regarding removing system calls. Please note that I did not touch any parts of the repository which I am not familiar with (e.g. contents of rpu.c or the update_env function).

Edit: If I am to remove the state.txt from libdfx, several of these functions could be moved to libdfx to reduce duplication. This got me thinking - why are they separate projects - are there other projects also using libdfx?
Do you agree that these functions should/could be moved to libdfx?
Specifically:

read_single_line
write_string_to_file
fpga_state
check_overlay_was_applied
write_path_to_overlay
write_to_fpga_firmware/write_to_fpga_flags
remove_overlay_dir
strip_trailing

talhaHavadar · 2025-12-09T10:54:44Z

@artiepoole , I think libdfx should only expose dfx related symbols not the generic utility functions, any further change to ABI of those functions would(should) trigger a library transition for the tools depending libdfx.

Ideally I would like to see those common utility functions sourced somewhere common for both libdfx and dfx-mgr but not tied to the library itself.

talhaHavadar · 2025-12-09T11:02:20Z

src/daemon_helper.c

+ * Strips trailing needle from haystack - e.g. `\n` from file read
+ * results or `/` from paths before concatenating.
+ */
+void strip_trailing(char *haystack, const char needle)


can we make this static as well?

src/daemon_helper.c

talhaHavadar · 2025-12-09T11:19:48Z

src/daemon_helper.c

+ * Return: 0 if the overlay status "applied" and path matches requested_path,
+ *        -1 on error or if either assertion is false
+ */
+static int check_overlay_was_applied(char *overlay_dir, char *requested_path)


this function modifies overlay_dir so it is not just a check. can we make it so it doesnt do that and use const in arguments to indicate this.

or at worst document this in function header so people are aware that this will change the arguments sent.

For requested_path, I think we are safe to add const easily.

talhaHavadar · 2025-12-09T11:20:55Z

src/daemon_helper.c

+    char read_buf[128];
+    const char *state_applied = "applied";
+
+    strip_trailing(overlay_dir, '/');


we might not need this at all, it is best to verify behaviour for //path though

I will test. usually works for bash cmds otherwise I will copy the buf before modifying

I decided to make a copy into full_path. strip trailing and assemble the full path in 3 steps instead. This functionality is now moving into libdfx, however, so have made those changes there

talhaHavadar · 2025-12-09T11:21:38Z

src/daemon_helper.c

+ * Return: 0 on success,
+ *        -1 on error (e.g., failed to open, write, or close the file)
+ */
+static int write_path_to_overlay(char *overlay_dir, const char *requested_path)


const overlay_dir

src/daemon_helper.c

talhaHavadar · 2025-12-09T11:26:57Z

src/daemon_helper.c

+    // ignore bits >= 1, only care about partial or full.
+    if (write_to_fpga_flags(flag & 1)) {
+        DFX_ERR("Failed to set flags");
+        goto ret;
+    }


I dont see it errors and jumps to the ret on the original file, it could be missed in the original implementation but just wanted to point out the behaviour change here

talhaHavadar · 2025-12-09T11:36:51Z

I see DFX_ERR already handles strerror so please ignore my comments about strerror: https://github.com/Xilinx/dfx-mgr/blob/master/src/print.h#L43-L47

artiepoole · 2025-12-10T14:25:38Z

Hey @tjaysukh,

I have removed the need for state.txt, but in order to do the same in libdfx, I would have had to duplicate a lot of code. I therefore exposed these functions through dfx-mgr, so this PR now depends even more heavily on libdfx changes.

Just fyi, the whitespace is a mess in daemon_helper.c - a mix of spaces, 4-width tabs and8-width tabs, all within the same functions and sometimes the same lines. It was very difficult to make changes and maintain these whitespaces. For this reason, I suggest looking through the changes with "ignore whitespace" on at first, and then enabling whitespace differences after understanding the functional changes.

It may be worth enforcing a clantidy and clangformat style for consistency.

p.s. I tacked on Talha's suggest const changes, but only for the functions I touched. I also added on newline to a load error print which has been annoying me for ages. Let me know if you'd prefer those/anything else here separately.

tjaysukh · 2025-12-11T12:23:45Z

@artiepoole,

Thanks again for the changes and your efforts. I’ll review and test everything, then get back to you

I have removed the need for state.txt, but in order to do the same in libdfx, I would have had to duplicate a lot of code. I therefore exposed these functions through dfx-mgr, so this PR now depends even more heavily on libdfx changes.

I need to discuss this with the stakeholders. As of now, in the lightweight use-case flow, dfx-mgr is not dependent on libdfx, but with this change, as you mentioned it will become heavily dependent on libdfx. I want to ensure everyone is aligned before we proceed.

artiepoole · 2025-12-11T14:25:49Z

Hi @tjaysukh,

As of now, in the lightweight use-case flow, dfx-mgr is not dependent on libdfx, but with this change, as you mentioned it will become heavily dependent on libdfx.

If necessary, it would not be difficult to recreate the functionality in dfx-mgr standalone. I was not aware of the need for this mode :) Just let me know what the ultimate decision is.

artiepoole · 2026-01-09T13:44:08Z

Hey @tjaysukh,

I'm just bumping to ask if has there been any update?
If so, please let me know how to proceed.

Best,
Artie

tjaysukh · 2026-01-12T11:59:59Z

@artiepoole,
I’ve confirmed with all stakeholders, and everyone is aligned on this approach. We can proceed with testing and incorporating this change once the libdfx updates are complete.

artiepoole · 2026-01-29T10:16:34Z

Hey @tjaysukh , I wanted to update you that the libdfx changes have been merged. Please see here for the confirmation.

Please keep me updated on the process

Best,
Artie

tjaysukh · 2026-01-30T11:40:56Z

@artiepoole,

Thanks for the update, I am currently testing the changes will keep you posted.

src/daemon_helper.c

tjaysukh · 2026-01-30T11:30:27Z

src/daemon_helper.h

-int user_unload_overlay(char *region);
+int user_load(int flag, const char *binfile, const char *overlay, const char *region);
+int user_unload_overlay(const char *region);
 int user_unload(int handle);


Please updated the user_unload() declaration with const as function updated.
int user_unload(const int handle)

I am willing to do this, but it's not typical for values passed by value - this value of handle is a local copy inside the function, and so the const here does nothing (as in, it is ignored in the header file) - at least that is what I understand

What I mean is that the function definition in src/daemon_helper.c was updated to int user_unload(const int handle). but the corresponding function declaration does not match this definition.

Ah, I understood that. I pushed back for the reasons explained below, but have just learned that it is preference, and so did as you asked 🫡 - sorry for any frustration.

my point of view

it is explained in the C/C++ standards that it does not matter, and clangtidy regards it as a linter warning/redundant code. In fact, my IDE suggests to remove the const from the header declaration. That's why it wasn't there and I didn't want to put it there.

The reference for the redundancy is point 3 of the Parameter-type-list section of CPP Function specs

…ting location Use `/sys/kernel/config/device-tree/overlays/` instead of remounting Signed-off-by: Artie Poole <stuart.poole@canonical.com>

Note: The implementation for controlling firmware is implemented in libdfx.h/.c to avoid code duplication. The kernel provides a mechanism to set where firmware loads will look to match filenames during probe events. This file is located at `/sys/module/firmware_class/parameters/path`. These changes remove the need for copying firmware files into /lib/firmware. The /lib/firmware location is still probed if the file is not found in the custom location. See https://docs.kernel.org/driver-api/firmware/fw_search_path.html for more information. Signed-off-by: Artie Poole <stuart.poole@canonical.com>

This also removes the need for a `state.txt` file, so we stop creating the `/run/dfx-mgrd` directory because it is no longer necessary Signed-off-by: Artie Poole <stuart.poole@canonical.com>

Signed-off-by: Artie Poole <stuart.poole@canonical.com>

tjaysukh · 2026-02-06T09:50:47Z

@artiepoole ,

I have completed testing the dfx-mgr application on my side, and everything looks good. Before we merge it to GitHub, it will go through the internal regression testing. Once that is completed, we will proceed with the merge.

artiepoole mentioned this pull request Nov 13, 2025

Ubuntu Core enablement changes Xilinx/libdfx#6

Closed

artiepoole force-pushed the xlnx_rel_v2025.2 branch 3 times, most recently from 83fd6da to 9095844 Compare November 17, 2025 12:28

artiepoole marked this pull request as ready for review November 17, 2025 12:30

artiepoole force-pushed the xlnx_rel_v2025.2 branch from 9095844 to 740ac2f Compare November 17, 2025 16:14

artiepoole marked this pull request as draft December 3, 2025 15:45

artiepoole marked this pull request as ready for review December 5, 2025 09:58

talhaHavadar suggested changes Dec 9, 2025

View reviewed changes

artiepoole force-pushed the xlnx_rel_v2025.2 branch 2 times, most recently from c3a460a to 0a3a47c Compare December 10, 2025 14:16

tjaysukh reviewed Jan 30, 2026

View reviewed changes

artiepoole added 3 commits January 30, 2026 12:21

daemon-helper.c: configfs no longer being remounted, use already exis…

0c462b1

…ting location Use `/sys/kernel/config/device-tree/overlays/` instead of remounting Signed-off-by: Artie Poole <stuart.poole@canonical.com>

daemon_helper.c: remove (most) system(..) calls

95b7d4a

This also removes the need for a `state.txt` file, so we stop creating the `/run/dfx-mgrd` directory because it is no longer necessary Signed-off-by: Artie Poole <stuart.poole@canonical.com>

artiepoole force-pushed the xlnx_rel_v2025.2 branch from 0a3a47c to 454f56b Compare January 30, 2026 12:27

artiepoole added 2 commits January 30, 2026 13:22

daemon_helper.c: use const args for user_load related functions

d1bcfa9

Signed-off-by: Artie Poole <stuart.poole@canonical.com>

dfx-mgr-client.c: add missing newline on load error print

1be2d55

Signed-off-by: Artie Poole <stuart.poole@canonical.com>

artiepoole force-pushed the xlnx_rel_v2025.2 branch from 454f56b to 1be2d55 Compare January 30, 2026 13:22

Conversation

artiepoole commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tjaysukh commented Nov 14, 2025

Uh oh!

artiepoole commented Nov 17, 2025

Uh oh!

tjaysukh commented Dec 3, 2025

Uh oh!

artiepoole commented Dec 3, 2025

Uh oh!

artiepoole commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

talhaHavadar commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

talhaHavadar commented Dec 9, 2025

Uh oh!

artiepoole commented Dec 10, 2025

Uh oh!

tjaysukh commented Dec 11, 2025

Uh oh!

artiepoole commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

artiepoole commented Jan 9, 2026

Uh oh!

tjaysukh commented Jan 12, 2026

Uh oh!

artiepoole commented Jan 29, 2026

Uh oh!

tjaysukh commented Jan 30, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artiepoole Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

my point of view

Uh oh!

tjaysukh commented Feb 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

artiepoole commented Nov 13, 2025 •

edited

Loading

artiepoole commented Dec 5, 2025 •

edited

Loading

artiepoole commented Dec 11, 2025 •

edited

Loading

artiepoole Jan 30, 2026 •

edited

Loading