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

[Jason Wang]

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?

[...]