Re: [Qemu-devel] [PATCH v5 0/8] e1000: Various fixes and registers' implementation

[Leonid Bloch]

On Tue, Nov 10, 2015 at 3:19 PM, Leonid Bloch
<address@hidden> wrote:
> On Tue, Nov 10, 2015 at 3:01 PM, Jason Wang <address@hidden> wrote:
>>
>>
>> On 11/10/2015 07:39 PM, Leonid Bloch wrote:
>>> On Tue, Nov 10, 2015 at 8:21 AM, Jason Wang <address@hidden> 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? Why not to prepare the ground for compatibility, and only
> then introduce the new registers (as it is done now)?
>>
>>>> 2) looks like we could save some lines of codes in patch 3, see the
>>>> comment in that patch
>>>>
>>>> Since we're near to soft freeze (12th), want to ask whether or not you
>>>> want to send a v6 or I can fix 1 my self. (if 2 is correct, we can do
>>>> optimizations on top).

I have a v6 with #2 fixed ready. Awaiting your answer to my question
regarding #1 before sending the series.
>>> Will send a v6 with a fix to 2 today. Regarding 1 - awaiting your answer.
>>>
>>> Thanks,
>>> Leonid.
>>>>> Leonid Bloch (8):
>>>>>   e1000: Cosmetic and alignment fixes
>>>>>   e1000: Add support for migrating the entire MAC registers' array
>>>>>   e1000: Introduced an array to control the access to the MAC registers
>>>>>   e1000: Trivial implementation of various MAC registers
>>>>>   e1000: Fixing the received/transmitted packets' counters
>>>>>   e1000: Fixing the received/transmitted octets' counters
>>>>>   e1000: Fixing the packet address filtering procedure
>>>>>   e1000: Implementing various counters
>>>>>
>>>>>  hw/net/e1000.c      | 503 
>>>>> +++++++++++++++++++++++++++++++++++++++++-----------
>>>>>  hw/net/e1000_regs.h |   8 +-
>>>>>  include/hw/compat.h |   4 +
>>>>>  3 files changed, 406 insertions(+), 109 deletions(-)
>>>>>
>>