Skip to content

Implement SIMPLE_NETWORK_PROTOCOL #606

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

Merged
merged 8 commits into from
Dec 16, 2022
Merged

Conversation

veluca93
Copy link
Contributor

@veluca93 veluca93 commented Dec 9, 2022

Fixes #431.

The code is written on top of @d-sonuga's PR, with the main change being in the tests, where an UDP packet is sent to the UDP echo server and the answer is read back.

Copy link
Member

@nicholasbishop nicholasbishop left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Added some suggestions and questions :)

if simple_network.receive(&mut buffer, None, None, None, None)
== Err(Status::NOT_READY.into())
{
bt.stall(1_000_000);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we could use the wait_for_packet here. I recall from the last PR attempting this that we couldn't get that event to fire... any idea what the problem could be there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This event didn't seem to fire on QEMU at all, despite the packet being received. I thought I was doing something wrong, but if with the same code I try to wait for a wait_for_key event, that event resolves fine...

At this point, I think either the QEMU/OVMF implementation and the EFI spec disagree, QEMU doesn't implement this event for whatever reason (I found this thread from 10 years ago that seems to imply that the event not being implemented is fairly common, but I am not sure how I would verify), or there was something else wrong with my code that I don't undersand. ~30m of digging got me nowhere last time I tried.

This is also why the event is not exposed, since I haven't managed to get it to work :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I dug around in the EDK2 and OVMF source a bit but don't have much to add. It does look like the WaitForPacket Event and all related machinery have been implemented for at least the virtio-net-pci driver in QEMU. However, even while using this driver, the event never seems to be signaled.

I'll try to plug gdb into qemu tomorrow and see what might be going on.

Copy link
Member

Choose a reason for hiding this comment

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

This is also why the event is not exposed, since I haven't managed to get it to work :)

I would recommend exposing it anyway; it may well work on some non-QEMU/EDK2 environments.

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 exposed it with a comment that cautions the user to check before using it :)

@veluca93 veluca93 force-pushed the snp branch 2 times, most recently from 0221361 to 4a92872 Compare December 16, 2022 08:23
@nicholasbishop
Copy link
Member

nicholasbishop commented Dec 16, 2022

(Sorry for my slow speed at clicking the "allow checks to run" button -- ideally I could just tell github to trust you or this PR, but it doesn't seem to be an option. Once a PR has been accepted then future PRs from you will be "trusted" for CI runs I think.)

@veluca93
Copy link
Contributor Author

(Sorry for my slow speed at clicking the "allow checks to run" button -- ideally I could just tell github to trust you or this PR, but it doesn't seem to be an option. Once a PR has been accepted then future PRs from you will be "trusted" for CI runs I think.)

No worries, I've been on the other side of this and I understand very well :) Hopefully this last version passes all the checks...

Copy link
Member

@nicholasbishop nicholasbishop left a comment

Choose a reason for hiding this comment

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

lgtm, thanks again for the PR

@nicholasbishop nicholasbishop merged commit 1ee3f38 into rust-osdev:main Dec 16, 2022
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.

Add support for the Simple Network Protocol
4 participants