Re: [Qemu-devel] [RFC PATCH 0/5] Introduce Intel 82574 GbE Controller Emulation (e1000e)

[Jason Wang]

On 10/31/2015 01:52 PM, Dmitry Fleytman wrote:
> Hello Jason,
>
> Thanks for reviewing. See my answers inline.
>
>
>> On 30 Oct 2015, at 07:28 AM, Jason Wang <address@hidden
>> <mailto:address@hidden>> wrote:
>>
>>
>>
>> On 10/28/2015 01:44 PM, Jason Wang wrote:
>>>
>>> On 10/26/2015 01:00 AM, Leonid Bloch wrote:
>>>> Hello qemu-devel,
>>>>
>>>> This patch series is an RFC for the new networking device emulation
>>>> we're developing for QEMU.
>>>>
>>>> This new device emulates the Intel 82574 GbE Controller and works
>>>> with unmodified Intel e1000e drivers from the Linux/Windows kernels.
>>>>
>>>> The status of the current series is "Functional Device Ready, work
>>>> on Extended Features in Progress".
>>>>
>>>> More precisely, these patches represent a functional device, which
>>>> is recognized by the standard Intel drivers, and is able to transfer
>>>> TX/RX packets with CSO/TSO offloads, according to the spec.
>>>>
>>>> Extended features not supported yet (work in progress):
>>>>  1. TX/RX Interrupt moderation mechanisms
>>>>  2. RSS
>>>>  3. Full-featured multi-queue (use of multiqueued network backend)
>>>>
>>>> Also, there will be some code refactoring and performance
>>>> optimization efforts.
>>>>
>>>> This series was tested on Linux (Fedora 22) and Windows (2012R2)
>>>> guests, using Iperf, with TX/RX and TCP/UDP streams, and various
>>>> packet sizes.
>>>>
>>>> More thorough testing, including data streams with different MTU
>>>> sizes, and Microsoft Certification (HLK) tests, are pending missing
>>>> features' development.
>>>>
>>>> See commit messages (esp. "net: Introduce e1000e device emulation")
>>>> for more information about the development approaches and the
>>>> architecture options chosen for this device.
>>>>
>>>> This series is based upon v2.3.0 tag of the upstream QEMU repository,
>>>> and it will be rebased to latest before the final submission.
>>>>
>>>> Please share your thoughts - any feedback is highly welcomed :)
>>>>
>>>> Best Regards,
>>>> Dmitry Fleytman.
>>> Thanks for the series. Will go through this in next few days.
>>
>> Have a quick glance at the series, got the following questions:
>>
>> - Though e1000e differs from e1000 in many places, I still see lots of
>> code duplications. We need consider to reuse e1000.c (or at least part
>> of). I believe we don't want to fix a bug twice in two places in the
>> future and I expect hundreds of lines could be saved through this way.
>
> That’s a good question :)
>
> This is how we started, we had a common “core” code base meant to
> implement all common logic (this split is still present in the patches
> - there are e1000e_core.c and e1000e.c files).
> Unfortunately at some point it turned out that there are more
> differences that commons. We noticed that the code becomes filled with
> many minor differences handling.
> This also made the code base more complicated and harder to follow.
>
> So at some point of time it was decided to split the code base and
> revert all changes done to the e1000 device (except a few
> fixes/improvements Leonid submitted a few days ago).
>
> Although there was common code between devices, total SLOC of e1000
> and e1000e devices became smaller after the split.
>
> Amount of code that may be shared between devices will be even smaller
> after we complete the implementation which still misses a few features
> (see cover letter) that will change many things.
>
> Still after the device implementation is done, we plan to review code
> similarities again to see if there are possibilities for code sharing.

I see, but if we can try to re-use or unify the codes from beginning, it
would be a little bit easier. Looks like the differences were mainly:

1) MSI-X support
2) offloading support through virtio-net header
3) trace points
4) other new functions through e1000e specific registers

So we could first unify the code through implementing the support of 2
and 3 for e1000. For MSI-X and other e1000e specific new functions, it
could be done through:

1) model specific callbacks, e.g realize, transmission and reception
2) A new register flags e.g PHY_RW_E1000E which means the register is
for e1000e only. Or even model specific wirteops and readops
3) For other subtle differences, it could be done in the code by
checking the model explicitly.

What's your opinion? (A good example of code sharing is freebsd's e1000
driver which covers both 8254x and 8257x).

>
>> - For e1000e it self, since it was a new device, so no need to care
>> about compatibility stuffs (e.g auto negotiation and mit). We can just
>> enable them forever.
>
> Yes, we have this in plans.
>
>> - And for the generic packet abstraction layer, what's the advantages of
>> this? If it has lot, maybe we can use it in other nic model (e.g
>> virtio-net)?
>
> These abstractions were initially developed by me as a part of vmxnet3
> device to be generic and re-usable. Their main advantage is support
> for virtio headers for virtio-enabled backends and emulation of
> network offloads in software for backends that do not support virtio.
>
> Of course they may be re-used by virtio, however I’m not sure if it
> will be really useful because virtio has feature negotiation
> facilities and do not require SW emulation for network task offloads.
>
> For other devices they are useful because each and every device that
> requires SW offloads implementation need to do exactly the same things
> and it doesn’t make sense to have a few implementations for this.
>
> Best Regards,
> Dmitry

Ok, thanks for the explanation.

>
>>
>> Thanks
>>
>>>
>>> Since 2.5 is in soft freeze, this looks a 2.6 material.
>