From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:54250) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwT5x-0001ho-6v for qemu-devel@nongnu.org; Wed, 11 Nov 2015 05:59:00 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZwT40-0001pR-Aj for qemu-devel@nongnu.org; Wed, 11 Nov 2015 05:57:41 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38308) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwT3z-0001pD-W7 for qemu-devel@nongnu.org; Wed, 11 Nov 2015 05:55:40 -0500 References: <1447081170-11614-1-git-send-email-leonid.bloch@ravellosystems.com> <56418CE8.5080704@redhat.com> <5641EAC0.1020805@redhat.com> <5642B483.2090907@redhat.com> From: Jason Wang Message-ID: <56431EA3.3040500@redhat.com> Date: Wed, 11 Nov 2015 18:55:31 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 0/8] e1000: Various fixes and registers' implementation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Bloch Cc: Dmitry Fleytman , Leonid Bloch , qemu-devel@nongnu.org, Shmulik Ladkani On 11/11/2015 04:06 PM, Leonid Bloch wrote: > On Wed, Nov 11, 2015 at 5:22 AM, Jason Wang wrote: >> > >> > >> > On 11/10/2015 09:19 PM, Leonid Bloch wrote: >>> >> On Tue, Nov 10, 2015 at 3:01 PM, Jason Wang wrote: >>>> >>> >>>> >>> On 11/10/2015 07:39 PM, Leonid Bloch wrote: >>>>> >>>> On Tue, Nov 10, 2015 at 8:21 AM, Jason Wang wrote: >>>>>> >>>>> On 11/09/2015 10:59 PM, Leonid Bloch wrote: >>>>>>> >>>>>> This series fixes issues with packet/octet counting in e1000's Statistic >>>>>>> >>>>>> registers, fixes a bug in the packet address filtering procedure, and >>>>>>> >>>>>> implements many MAC registers that were absent before, some Statistic >>>>>>> >>>>>> counters among them. >>>>>>> >>>>>> >>>>>>> >>>>>> Besides this, the series introduces a parameter which, if set to "on" >>>>>>> >>>>>> (default), will cause the entire MAC registers' array to migrate during >>>>>>> >>>>>> live migration (please see patch #2 for details). The rational behind >>>>>>> >>>>>> this is the ability to implement additional MAC registers in the future, >>>>>>> >>>>>> without worrying about migration compatibility between future versions. >>>>>>> >>>>>> For compatibility with previous versions, the above mentioned parameter >>>>>>> >>>>>> can be set to "off". >>>>>>> >>>>>> >>>>>>> >>>>>> Also, a new array is introduced to control the access to the various MAC >>>>>>> >>>>>> registers. This takes care of situations when a MAC register requires a >>>>>>> >>>>>> certain parameter to be accessed, or is partially implemented, and >>>>>>> >>>>>> requires a debug warning to be printed on access attempts. >>>>>>> >>>>>> >>>>>>> >>>>>> Additionally, several cosmetic changes are made. >>>>>>> >>>>>> >>>>>>> >>>>>> Differences v1-2: >>>>>>> >>>>>> -------------------- >>>>>>> >>>>>> * Wording of several commit messages corrected. >>>>>>> >>>>>> * For trivially implemented Diagnostic registers, a debug message is >>>>>>> >>>>>> added on read/write attempts, alerting of incomplete implementation. >>>>>>> >>>>>> * Following testing on a physical device, only the lower 16 bits can now >>>>>>> >>>>>> be read from AIT, and only the lower 4 - from FFMT*. >>>>>>> >>>>>> * The grow_8reg_if_not_full function is rewritten. >>>>>>> >>>>>> * inc_tx_bcast_or_mcast_count and increase_size_stats are now called >>>>>>> >>>>>> from within e1000_send_packet, to avoid code duplication. >>>>>>> >>>>>> >>>>>>> >>>>>> Differences v2-3: >>>>>>> >>>>>> -------------------- >>>>>>> >>>>>> * Minor rewordings of some commit messages (0002, 0003). >>>>>>> >>>>>> * Live migration capability is added to the newly implemented registers. >>>>>>> >>>>>> >>>>>>> >>>>>> Differences v3-4: >>>>>>> >>>>>> -------------------- >>>>>>> >>>>>> * Introduction of the "full_mac_registers" parameter (see above). >>>>>>> >>>>>> * Reversion of the live migration handling introduced in v3. >>>>>>> >>>>>> * Small alignment changes in patch #1 to correspond with the following >>>>>>> >>>>>> patches. >>>>>>> >>>>>> >>>>>>> >>>>>> Differences v4-v5: >>>>>>> >>>>>> -------------------- >>>>>>> >>>>>> * Introduction of an array to control the access to the MAC registers. >>>>>>> >>>>>> * Removal of the specific functions that warned of partial >>>>>>> >>>>>> implementation on read/write from patch 4. >>>>>>> >>>>>> * Adequate changes to patches 4 and 8: mainly adding the registers >>>>>>> >>>>>> introduced there to the new array. >>>>>>> >>>>>> >>>>>>> >>>>>> The majority of these changes result from Jason Wang's review - thank >>>>>>> >>>>>> you, Jason! >>>>>> >>>>> Thanks a lot for the patches. Almost done with two minor concerns: >>>>>> >>>>> >>>>>> >>>>> 1) to unbreak bisection we'd better enable the extra_mac_registers (and >>>>>> >>>>> compatibility stuffs) in patch 8 or patch 9 >>>>> >>>> Do you mean by that changing patch 2, so that the compatibility would >>>>> >>>> be "on" by default, and setting it to "off" by default only in patch >>>>> >>>> 8, or an additional patch 9? >>>> >>> I mean do not introduce the property "extra_mac_registers" until patch 8 >>>> >>> and 9. In this case all function will be enabled completely at that time >>>> >>> instead of partially patch by patch in this series. >>> >> But won't there be compatibility issues between the patches if done >>> >> like that? >> > >> > Looks not, all new mac registers and mac array migration will be >> > functional or used only in last patch. We don't have any compatibility >> > issue since E1000_FLAG_MAC can only be set for the last patch since the >> > meaning of it should be "migrate the whole mac array and enable the >> > various mac registers". And if we just use the order like this series, >> > we may have compatibility issue during bisection. E.g migrate between w/ >> > patch 8 and w/o patch 8. > But the trivial MAC registers are introduced in patch 4 already (in > order to minimize code changes in the subsequent patches). Besides, if > migration of the full MAC registers' array will be enabled, even > before all the new registers are introduced, what will be the the > problem with migration? One should be able to migrate from w/ patch 8, > to anywhere after patch 2 with no problem, and to before that - with > compatibility flag set. Yes, migration can complete. But the value of the counters will suddenly drop to zero after migration from patch 8 to patch 7. This is suboptimal and can lead unexpected results.