Re: [Qemu-devel] [PATCH v5 3/8] e1000: Introduced an array to control the access to the MAC registers

[Leonid Bloch]

On Tue, Nov 10, 2015 at 7:37 AM, Jason Wang <address@hidden> wrote:
>
>
> On 11/09/2015 10:59 PM, Leonid Bloch wrote:
>> The array of uint8_t's which is introduced here, contains useful metadata
>> about the MAC registers: if a register should be always accessible, or if
>> it is accessible, but partly implemented, or if the register requires a
>> certain compatibility flag to be accessed. Currently, 5 hypothetical flags
>> are supported (3 exist for e1000 so far) but if in the future more than 5
>> flags will be needed, the datatype of this array can simply be swapped for
>> a larger one.
>>
>> This patch is intended to solve the following current problems:
>>
>> 1) On migration between different versions of QEMU, which differ by the
>> MAC registers implemented in them, some registers need not to be active if
>> a compatibility flag is set, in order to preserve the machine's state
>> perfectly for the older version. Checking this for each register
>> individually, would create a lot of clutter in the code.
>>
>> 2) Some registers are (or may be) only partly implemented (e.g.
>> placeholders that allow reading and writing, but lack other functions).
>> In such cases it is better to print a debug warning on read/write attempts.
>> As above, dealing with this functionality on a per-register level, would
>> require longer and more messy code.
>>
>> Signed-off-by: Leonid Bloch <address@hidden>
>> Signed-off-by: Dmitry Fleytman <address@hidden>
>> ---
>>  hw/net/e1000.c | 85 
>> +++++++++++++++++++++++++++++++++++++++++++++++++---------
>>  1 file changed, 73 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/net/e1000.c b/hw/net/e1000.c
>> index 0e00afa..2bc533f 100644
>> --- a/hw/net/e1000.c
>> +++ b/hw/net/e1000.c
>> @@ -142,6 +142,8 @@ typedef struct E1000State_st {
>>      uint32_t compat_flags;
>>  } E1000State;
>>
>> +#define chkflag(x)     (s->compat_flags & E1000_FLAG_##x)
>> +
>>  typedef struct E1000BaseClass {
>>      PCIDeviceClass parent_class;
>>      uint16_t phy_id2;
>> @@ -195,8 +197,7 @@ e1000_link_up(E1000State *s)
>>  static bool
>>  have_autoneg(E1000State *s)
>>  {
>> -    return (s->compat_flags & E1000_FLAG_AUTONEG) &&
>> -           (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN);
>> +    return chkflag(AUTONEG) && (s->phy_reg[PHY_CTRL] & MII_CR_AUTO_NEG_EN);
>>  }
>>
>>  static void
>> @@ -321,7 +322,7 @@ set_interrupt_cause(E1000State *s, int index, uint32_t 
>> val)
>>          if (s->mit_timer_on) {
>>              return;
>>          }
>> -        if (s->compat_flags & E1000_FLAG_MIT) {
>> +        if (chkflag(MIT)) {
>>              /* Compute the next mitigation delay according to pending
>>               * interrupts and the current values of RADV (provided
>>               * RDTR!=0), TADV and ITR.
>> @@ -1258,6 +1259,43 @@ static void (*macreg_writeops[])(E1000State *, int, 
>> uint32_t) = {
>>
>>  enum { NWRITEOPS = ARRAY_SIZE(macreg_writeops) };
>>
>> +enum { MAC_ACCESS_ALWAYS = 1, MAC_ACCESS_PARTIAL = 2,
>> +       MAC_ACCESS_FLAG_NEEDED = 4 };
>> +
>> +#define markflag(x)    ((E1000_FLAG_##x << 3) | MAC_ACCESS_FLAG_NEEDED)
>> +/* In the array below the meaning of the bits is: [f|f|f|f|f|n|p|a]
>> + * f - flag bits (up to 5 possible flags)
>> + * n - flag needed
>> + * p - partially implenented
>> + * a - access enabled always
>> + * n=p=a=0 - not implemented or unknown */
>
> Looks like n=p=0 implies a=0? If yes we can probably get rid of bit 'a'
> and save lots of lines below?

n=p=0 does not quite imply that a=0: a counter example would be a
register that is fully implemented, and does not require a special
flag to be accessed.
But that "a" bit is redundant indeed - the check if the register is
implemented is already performed by looking in
macreg_readops/macreg_writeops. I included it just so that the
information on which registers are implemented will be present in
mac_reg_chk, if the need for it will raise in the future.

But when thinking of it now, you are right - I will remove it, and
rename the new array to "mac_reg_access", to better represent its
purpose.
>
> [...]