On Tue, Nov 10, 2015 at 7:37 AM, Jason Wang <jasow...@redhat.com> 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 <leonid.bl...@ravellosystems.com> >> Signed-off-by: Dmitry Fleytman <dmitry.fleyt...@ravellosystems.com> >> --- >> 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. > > [...]