On Fri, Jul 9, 2010 at 12:48 PM, Thomas Miletich <thomas.milet...@gmail.com> wrote: > 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.
Good catch. > - 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? There is actually a reason for this but it isn't obvious, I will add a comment. The virtio-net.h header file comes from Linux (like the other virtio header files), whereas the driver .c is written for Etherboot. I think the intent was to keep the header files in sync from Linux and not change them or add gPXE-specific driver structures. > - 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. Yes, I actually started reformatting the virtio code but then aborted since I needed to make changes to virtio. A separate patch would be nice although I may not have time to do it right away. There is another change to core virtio that I'd like to do if I get time: vrings are allocated without alignment support from the memory allocator - instead an extra page is allocated and the pointer is manually aligned. To avoid wasting memory the proper memory allocator functions should be used. > The rest looks good to me, thanks for your work :) Thanks for reviewing :). Stefan > 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
