[gPXE-devel] [PATCH] [virtio] Replace virtio-net with native gPXE driver

[Stefan Hajnoczi]

On Fri, Jul 9, 2010 at 12:48 PM, Thomas Miletich
<thomas.miletich at 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 <stefanha at gmail.com> wrote:
>> On Sat, Jul 3, 2010 at 10:07 PM, Stefan Hajnoczi <stefanha at gmail.com> wrote:
>>> On Sat, Jul 3, 2010 at 9:52 PM, Stefan Hajnoczi <stefanha at 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 <stefanha at 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 at etherboot.org
>> http://etherboot.org/mailman/listinfo/gpxe-devel
>>
>