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

Implement virtio-net device #34

Merged
merged 6 commits into from
Jul 21, 2024

Conversation

jimmylu890303
Copy link
Contributor

Implementation of Virtio-net Device

  • Defined struct virtio_net_dev to represent the virtio-net device.
  • Implemented virtio_net_init to initialize the TAP device for communication.
  • Introduced virtio_net_init_pci to register the virtio-net device on PCI, configure virtqueues.
  • Implemented functions to handle available events for RX and TX virtqueues.
  • Added functions to notify the guest OS about used descriptors after packet processing.

Makefile Modification

  • Added rules to compile virtio-net.c into virtio-net.o, ensuring correct compilation and linkage of virtio-net functionality within the project.

Kernel Networking Options Enabled

  • Enabled necessary kernel configuration options (CONFIG_NET, CONFIG_INET, CONFIG_VIRTIO, CONFIG_VIRTIO_NET, CONFIG_NETDEVICES) to support networking functionality.
  • Enabled specific networking commands required for testing the virtio-net device.

Structural Updates

  • Added struct virtio_net_dev entry within struct vm_t to facilitate the integration and management of the virtio-net device.

Additional Changes

  • Called virtio_net_init and virtio_net_init_pci in vm_arch_init_platform_device and main respectively, ensuring initialization of the virtio-net device before the virtual machine starts.

Makefile Outdated Show resolved Hide resolved
configs/busybox.config Outdated Show resolved Hide resolved
configs/linux-x86.config Outdated Show resolved Hide resolved
configs/linux-x86.config Outdated Show resolved Hide resolved
configs/linux-x86.config Outdated Show resolved Hide resolved
src/arch/x86/vm.c Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
src/virtio-net.c Show resolved Hide resolved
src/virtio-net.h Outdated Show resolved Hide resolved
src/virtio-pci.h Show resolved Hide resolved
@jserv jserv requested review from RinHizakura and yanjiew1 July 4, 2024 07:08
src/main.c Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from 2448d1f to 8ff17bb Compare July 6, 2024 12:11
@jimmylu890303
Copy link
Contributor Author

jimmylu890303 commented Jul 6, 2024

I have pushed a newer version. Please take a look.

For testing the function, you can use the command below:

Host OS :

sudo ip link delete br0
sudo brctl addbr br0
sudo ip addr add 10.0.0.1/24 dev br0
sudo ip route add default via 10.0.0.1 dev br0
sudo ip link set br0 up
sudo ip link set tap0 master br0
sudo ip link set tap0 up

Guest OS :

ip addr add 10.0.0.2/24 dev eth0
ip link set eth0 up
ip route add default via 10.0.0.1

src/arch/x86/vm.c Outdated Show resolved Hide resolved
src/arch/x86/vm.c Outdated Show resolved Hide resolved
@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch 2 times, most recently from 24b160f to 6ef8a65 Compare July 9, 2024 09:13
Copy link
Collaborator

@RinHizakura RinHizakura left a comment

Choose a reason for hiding this comment

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

I'm good with the changes now. Thanks for all the kindly reply.

Let' see if @jserv has any extra comment.

@jimmylu890303
Copy link
Contributor Author

I'm good with the changes now. Thanks for all the kindly reply.

Let' see if @jserv has any extra comment.

Thanks for your review !

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Rebase the latest master branch because the commit "Bump linux version to 6.1.91" was accidentally checked in as a non-functional change. See #35

Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Indent all files using clang-format. That is, run find src -name '*.[ch]' | xargs clang-format-12 -i. Do replace clang-format-12 with clang-format version 12 or later.

@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from 6ef8a65 to e0a0284 Compare July 20, 2024 04:43
@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from e0a0284 to 7543e27 Compare July 20, 2024 05:25
@jimmylu890303
Copy link
Contributor Author

jimmylu890303 commented Jul 20, 2024

I have pushed the latest version with the following updates:

  • I rebased the latest master branch to remove the non-functional commit "Bump linux version to 6.1.91" as discussed in issue Fail to run kvm-host with Linux in version 6.1.91 #35.
  • I have reformatted all relevant files using clang-format. Specifically, I ran the command find src -name '*.[ch]' | xargs clang-format-12 -i with clang-format version 12.

@jserv
Copy link
Contributor

jserv commented Jul 21, 2024

  1. I have reformatted all relevant files using clang-format. Specifically, I ran the command find src -name '*.[ch]' | xargs clang-format-12 -i with clang-format version 12.

Warning: No newline at end of file src/virtio-net.c
See https://medium.com/@alexey.inkin/how-to-force-newline-at-end-of-files-and-why-you-should-do-it-fdf76d1d090e

Makefile Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
src/virtio-net.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Use git rebase -i to rework the git commits.
The following commits can be squashed and refined:

  • Define VIRTIO_NET_IRQ 14
  • Define VIRTIO_PCI_DEVICE_ID_NET macro
  • Define macro in virtio-net.h
  • Define struct virtio_net_dev and functions in virtio-net.h

Git commits are snapshots around logical units of change. Over time, commits should tell a story of the history of your repository and how it came to be the way that it currently is.
See https://github.blog/2022-06-30-write-better-commits-build-better-projects/

You don't have to mention the file names in the subject of git commit messages. Instead, summarize the functional changes.

- Define `VIRTIO_NET_IRQ` macro for the `notify_used` virtq
callback function to send IRQ
- Define `VIRTIO_PCI_DEVICE_ID_NET` (0x1041) according to
the Virtio 1.1 specification
- Define `VIRTIO_NET_PCI_CLASS` macro according to the PCI
specification with class code (0x020000) for Ethernet Controller
- Define struct `virtio_net_dev` and operation functions for
the virtio-net device
@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from 7543e27 to 7549229 Compare July 21, 2024 13:28
@sysprog21 sysprog21 deleted a comment from jimmylu890303 Jul 21, 2024
src/virtio-net.c Outdated Show resolved Hide resolved
@@ -93,6 +93,8 @@ int main(int argc, char *argv[])
return throw_err("Failed to load initrd");
if (diskimg_file && vm_load_diskimg(&vm, diskimg_file) < 0)
return throw_err("Failed to load disk image");
if (vm_enable_net(&vm) < 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

If virtio-net is not enabled, it still makes sense to initialize and launch this virtual machine via KVM. Thus, instead of returning, we can proceed.

Copy link
Contributor Author

@jimmylu890303 jimmylu890303 Jul 21, 2024

Choose a reason for hiding this comment

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

I changed throw_err to fprintf. If vm_enable_net failed to enable virtio-net device, it will now handle the situation by logging an error message.

if (vm_enable_net(&vm) < 0)
        fprintf(stderr, "Failed to Enable Virtio-Net Device\n");

Copy link
Contributor

Choose a reason for hiding this comment

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

Refine the message as following:

Failed to enable virtio-net device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok ! Already done

- Defined struct `virtio_net_dev` to represent the virtio-net device.
- Implemented `virtio_net_init` to initialize the TAP device and
prepare it for communication.
- Added `virtio_net_init_pci` for registering the virtio-net device to PCI,
setting up virtqueues.
- Implemented functions to enable and handle available events for RX and TX virtqueues.
- Added functions to notify the guest OS about used descriptors after packet processing.
This commit removes the initialization of vq->info.notify_off
value in the virtio-net implementation. The reason for this
change is that during the execution of RX and TX enable
callback functions, the `vm_ioeventfd_register` function
registers the ioeventfd at the same address repeatly.
Therefore, the notify_off value is now initialized directly
within the virtqueue enable callback functions.
This commit adds a struct `virtio_net_dev` entry
within the struct `vm_t` structure to facilitate
the implementation of the virtio-net device.
This addition is necessary to integrate and manage
the virtio-net device within the broader context
of the virtual machine.
@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from 7549229 to 2ec4a21 Compare July 21, 2024 13:55
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

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

Unify the commit "Implement vm_enable_net API" and "Compile virtio-net.c into virtio-net.o during build process" into a single one since they are tended to the same purpose.

@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from 2ec4a21 to a878518 Compare July 21, 2024 15:33
The vm_enable_net function to facilitate the integration
of additional I/O devices in the future. The function initializes
the virtio-net device by setting up the tap device using virtio_net_init
and subsequently registers the virtio-net device with the PCI bus.
This function is invoked during the VM initialization session.
Enabled kernel configuration options required for networking
functionality,including `CONFIG_NET`,`CONFIG_INET`,`CONFIG_VIRTIO`,
`CONFIG_VIRTIO_NET`,`CONFIG_NETDEVICES` etc.
Additionally, enabled specific networking commacands necessary for
testing the virtio-net device.
@jimmylu890303 jimmylu890303 force-pushed the Implement-virtio-net branch from a878518 to f727c6e Compare July 21, 2024 18:00
@jserv jserv merged commit e2e954c into sysprog21:master Jul 21, 2024
3 checks passed
@jserv
Copy link
Contributor

jserv commented Jul 21, 2024

Thank @jimmylu890303 for contributing!

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