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

[Jason Wang]

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 

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

[Leonid Bloch]

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.
>
>>  Why not to prepare the ground for compatibility, and only
>> then introduce the new registers (as it is done now)?
>
> We do prepare it from the first patch but just not enable it until we're
> sure that everything is ready.
>

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
 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? 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 |   

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

[Leonid Bloch]

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?
> 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).

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(-)
>>
>

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

[Leonid Bloch]

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? 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).
>> 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(-)

>

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

[Jason Wang]

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.

>> 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).
> 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(-)
>>>

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

[Jason Wang]

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.

>  Why not to prepare the ground for compatibility, and only
> then introduce the new registers (as it is done now)?

We do prepare it from the first patch but just not enable it until we're
sure that everything is ready.

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

[Leonid Bloch]

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!

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(-)

-- 
2.4.3

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

[Jason Wang]

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
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).

>
> 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(-)
>