On 11/02/2015 03:49 PM, Dmitry Fleytman wrote: > >> On 2 Nov 2015, at 05:35 AM, Jason Wang <jasow...@redhat.com >> <mailto:jasow...@redhat.com>> wrote: >> >> >> >> 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 <jasow...@redhat.com >>>> <mailto:jasow...@redhat.com> >>>> <mailto:jasow...@redhat.com>> 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). > > > Hi Jason, > > This is exactly how we started. > > Issues that made us split the code base were following: > > 1. The majority of registers are different. Even same registers in > many cases have different bits and require different processing logic. > This introduced too much if’s into the code;
Then we can probably have different writeops and readops. This way, we can at least save the codes of common registers. > 2. The data path is totally different not just because of virtio > headers but also because these devices use different descriptor > layouts and require different logic in order to parse and fill those. > There are legacy descriptors that look almost the same but of course > we must support all descriptor types described by spec; Yes, but looks like the only extend rx/tx descriptors is 8257x specific. And 8257x fully supports both legacy and context descriptor of 8254x. This give us another chance to reuse the code. > 3. Interrupt handling logic is different because of MSI support; Right, but this is not hard to address, probably a new helper. > 4. Mutli-queue and RSS make the code even less similar to the old device; Yes, this could be in 8275x specific file. > 5. Changes required to isolate shared code base required changes in > migration tables and fishy tricks to preserve compatibility with > current implementation; Since 8257x is a totally new device, it can has its own vmstate if it's simpler to be implemented and we don't even need to care migration compatibility. > 6. e1000 code suffered from massive changes which are very hard to > verify because spec is big and there are no drivers that use all > device features. Then we can try to change e1000 as mini as possible. E.g just factor out the common logic to helpers to reuse it in e1000e. > > Overall, code for handling differences in device behaviours was bigger > and more complicated then the device logic itself. The difference is > not subtle when it comes to the full featured device implementation. > As for FreeBSD’s driver, I’m not deeply familiar with its code but I > suspect it works with device in legacy mode which is pretty similar to > an old device indeed. Since we must support all modes our situation is > different. Yes, it does not use extended descriptor format. > > Again, I’m totally into shared code and would like to have some common > code base anyway. Currently it looks like the best way to achieve this > is to finish with all device features and then see what parts of logic > may be shared between the old and the new devices. It’s better to have > slight code duplication and smaller shared code base than to have > bloated and tricky shared code that will introduce its own problems to > both devices. The code duplication is not slight in this rfc :). So the code has the possibility to be unified. But I'm ok to evaluate this after all features were developed. Thanks > > Best Regards, > Dmitry > >> >>> >>>> - 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. >
