Hello Stefan Here are a few comments: - virtnet_enqueue_iob: The address of the local variable 'header' is passed to vring_add_buf(). As you said on IRC, the hypervisor could access the data after 'header' went out of scope and 'header' could already be overwritten by something else.
- We have virtio-net.h. Some struct definitions are in the header file, while some others are still in the .c file. Why not move all the definitions, including the 2 enums, to the header? - All the virtio code, except virtio-net.c, is using spaces for indentation. While we're at it we could have a second patch and run 'indent -kr -i8' (or something similar) on those files. The rest looks good to me, thanks for your work :) Thomas On Fri, Jul 9, 2010 at 7:25 AM, Stefan Hajnoczi <stefa...@gmail.com> wrote: > On Sat, Jul 3, 2010 at 10:07 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: >> On Sat, Jul 3, 2010 at 9:52 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: >>> This patch adds a native gPXE virtio-net driver and removes the legacy >>> Etherboot virtio-net driver. The main reasons for doing this are: >>> >>> 1. Multiple virtio-net NICs are now supported by gPXE. The legacy >>> driver kept global state and caused issues in virtual machines with >>> more than one virtio-net device. >>> >>> 2. Faster downloads. The native gPXE driver downloads 100 MB over HTTP >>> in 12s, the legacy Etherboot driver in 37s. This simple benchmark >>> uses KVM with tap networking and the Python SimpleHTTPServer both >>> running on the same host. >>> >>> Changes to core virtio code reduce vring descriptors to 256 (QEMU uses >>> 128 for virtio-blk and 256 for virtio-net) and change the opaque token >>> from u16 to void*. Lowering the descriptor count reduces memory >>> consumption. The void* opaque token change makes driver code simpler. >>> >>> Signed-off-by: Stefan Hajnoczi <stefa...@gmail.com> >> >> Testing would be appreciated so here is a ROM-o-matic to build images: >> >> http://etherboot.org/share/stefanha/virtio-net/contrib/rom-o-matic/ >> >> If you want to build from source: >> >> $ git clone -b virtio-net >> git://git.etherboot.org/scm/people/stefanha/gpxe.git >> $ cd gpxe/src >> $ make > > Thanks for testing the new driver: > > http://support.etherboot.org/index.php?do=details&task_id=96 > > I'd appreciate code review if anyone has time. > > Stefan > _______________________________________________ > gPXE-devel mailing list > gPXE-devel@etherboot.org > http://etherboot.org/mailman/listinfo/gpxe-devel > _______________________________________________ gPXE-devel mailing list gPXE-devel@etherboot.org http://etherboot.org/mailman/listinfo/gpxe-devel
