From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38352) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zw1cU-0007xb-4t for qemu-devel@nongnu.org; Tue, 10 Nov 2015 00:37:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zw1cQ-0001aG-Sd for qemu-devel@nongnu.org; Tue, 10 Nov 2015 00:37:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:57803) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zw1cQ-0001Zx-Lp for qemu-devel@nongnu.org; Tue, 10 Nov 2015 00:37:22 -0500 References: <1447081170-11614-1-git-send-email-leonid.bloch@ravellosystems.com> <1447081170-11614-4-git-send-email-leonid.bloch@ravellosystems.com> From: Jason Wang Message-ID: <5641828C.2040104@redhat.com> Date: Tue, 10 Nov 2015 13:37:16 +0800 MIME-Version: 1.0 In-Reply-To: <1447081170-11614-4-git-send-email-leonid.bloch@ravellosystems.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v5 3/8] e1000: Introduced an array to control the access to the MAC registers List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Bloch , qemu-devel@nongnu.org Cc: Dmitry Fleytman , Leonid Bloch , Shmulik Ladkani 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 > Signed-off-by: Dmitry Fleytman > --- > 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? [...]