Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NFS client PD and MicroPython VFS support #1

Merged
merged 23 commits into from
Dec 15, 2023
Merged

Add NFS client PD and MicroPython VFS support #1

merged 23 commits into from
Dec 15, 2023

Conversation

JE-Archer
Copy link
Contributor

This adds an NFS client PD to fs/nfs as well as adding an implementation of MicroPython's VFS layer to the Kitty system's MicroPython which allows you to interact with the NFS client. The end result being that you can run MicroPython's standard file operations to interact with a remote NFS server.

Neither the NFS client nor the MicroPython layer are complete or bug-free.

Building the Kitty system is now a bit more complicated. It requires a cross-compiling GCC for one (use the one that Microkit asks for). I also could not build libnfs on macOS due to an unsupported compiler flag that CMake was trying to use. You will need to define environment variables LIBGCC (directory where libgcc.a can be found), NFS_SERVER and NFS_DIRECTORY. The README has not been updated yet.

Copy link
Contributor

@wom-bat wom-bat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get @ivan's review as well. There are a number of minor changes I'd like before merging.

examples/kitty/src/micropython/sddf_fs.c Outdated Show resolved Hide resolved
examples/kitty/src/micropython/sddf_fs.c Outdated Show resolved Hide resolved

mp_obj_t mp_vfs_sddf_fs_file_open(const mp_obj_type_t *type, mp_obj_t file_in, mp_obj_t mode_in) {
mp_obj_vfs_sddf_fs_file_t *o = m_new_obj(mp_obj_vfs_sddf_fs_file_t);
// const char *mode_s = mp_obj_str_get_str(mode_in);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all this commented out stuff still here for a reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently I'm not supporting any notion of modes in open, which we might want to change, so I've left that there as reference.

examples/kitty/src/vmm/images/linux.dts Show resolved Hide resolved
fs/include/fs/protocol.h Outdated Show resolved Hide resolved
fs/nfs/tcp.c Show resolved Hide resolved
#include "util.h"
#include "tcp.h"

#define LINK_SPEED 1000000000 // Gigabit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be queried from system rather than hard coded. The NIC may have connected at 100Mb/s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took that part of the code from the echo server. Not sure how to query the system.

fs/nfs/tcp.c Outdated Show resolved Hide resolved

bool network_ready;

int tcp_ready(void) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static inline?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tcp_ready is called from other compilation units.

fs/nfs/tcp.c Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this belong in micropython?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there is stuff like await(mp_event_source_nfs); but everything else is generic right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not particularly generic as it is based on the synchronous model that we are using in MicroPython. I want to provide a generic client library but that will provide an asynchronous interface.

@Ivan-Velickovic
Copy link
Collaborator

Main things I want to understand are:

It requires a cross-compiling GCC for one (use the one that Microkit asks for)

Why?

I also could not build libnfs on macOS due to an unsupported compiler flag that CMake was trying to use

What flag?

You will need to define environment variables LIBGCC

Why?

We can discuss this more open-ended stuff in-person though.

@Ivan-Velickovic
Copy link
Collaborator

For the README we should also have some examples of some basic usage of NFS from Micropython - for our own reference as well as outside users of the example.

This commit modifies the NFS client to:

- provide an asynchronous API;
- use ringbuffers for communicating with clients;
- support running multiple operations concurrently;
- bounds-check client-provided buffers;
- allocate continuations from a pool instead of using malloc.

It also modifies the MicroPython FS support to the new NFS client interface.
fs/nfs/util.h Outdated Show resolved Hide resolved
This channel is so MicroPython can transfer the data it has
put in the framebuffer to be given to the Linux driver guest.
-->
<end pd="micropython" id="0" />
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The channel between Micropython and the VMM seems to be gone ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Can't test the graphics VM so didn't notice it went missing.

Unfortunately libnfs does not seem to work with Clang and also needs
libgcc for some reason. This patch puts the CI in a state where it
can build the current work-in-progress Kitty system until we can sort
it out and have it all build with Clang.
@Ivan-Velickovic Ivan-Velickovic merged commit 633280d into main Dec 15, 2023
1 check passed
@Ivan-Velickovic Ivan-Velickovic deleted the nfs branch December 15, 2023 09:52
Ivan-Velickovic added a commit that referenced this pull request Jun 15, 2024
Add NFS client PD and MicroPython VFS support
Ivan-Velickovic added a commit that referenced this pull request Aug 6, 2024
Add NFS client PD and MicroPython VFS support
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants