Re: [PATCH 08/21] hw/net/e1000: Remove unused E1000_FLAG_MAC flag

[Thomas Huth]

On 17/01/2025 17.41, Philippe Mathieu-Daudé wrote:
> On 17/1/25 09:58, Thomas Huth wrote:
> > On 16/01/2025 00.22, Philippe Mathieu-Daudé wrote:
> > > E1000_FLAG_MAC was only used by the hw_compat_2_4[] array,
> > > via the 'extra_mac_registers=off' property. We removed all
> > > machines using that array, lets remove all the code around
> > > E1000_FLAG_MAC.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > > ---
> > >   hw/net/e1000.c | 63 +++++++++-----------------------------------------
> > >   1 file changed, 11 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c
> > > index 3d0b2277039..14d2133cd80 100644
> > > --- a/hw/net/e1000.c
> > > +++ b/hw/net/e1000.c
> > > @@ -127,10 +127,8 @@ struct E1000State_st {
> > >       QEMUTimer *flush_queue_timer;
> > >   /* Compatibility flags for migration to/from qemu 1.3.0 and older */
> > > -#define E1000_FLAG_MAC_BIT 2
> > >   #define E1000_FLAG_TSO_BIT 3
> > >   #define E1000_FLAG_VET_BIT 4
> > > -#define E1000_FLAG_MAC (1 << E1000_FLAG_MAC_BIT)
> > >   #define E1000_FLAG_TSO (1 << E1000_FLAG_TSO_BIT)
> > >   #define E1000_FLAG_VET (1 << E1000_FLAG_VET_BIT)
> > > @@ -1218,46 +1216,17 @@ enum { MAC_ACCESS_PARTIAL = 1, 
> > > MAC_ACCESS_FLAG_NEEDED = 2 };
> > >    * n - flag needed
> > >    * p - partially implenented */
> > >   static const uint8_t mac_reg_access[0x8000] = {
> > > -    [IPAV]    = markflag(MAC),    [WUC]     = markflag(MAC),
> > > -    [IP6AT]   = markflag(MAC),    [IP4AT]   = markflag(MAC),
> > > -    [FFVT]    = markflag(MAC),    [WUPM]    = markflag(MAC),
> > > -    [ECOL]    = markflag(MAC),    [MCC]     = markflag(MAC),
> > > -    [DC]      = markflag(MAC),    [TNCRS]   = markflag(MAC),
> > > -    [RLEC]    = markflag(MAC),    [XONRXC]  = markflag(MAC),
> > > -    [XOFFTXC] = markflag(MAC),    [RFC]     = markflag(MAC),
> > > -    [TSCTFC]  = markflag(MAC),    [MGTPRC]  = markflag(MAC),
> > > -    [WUS]     = markflag(MAC),    [AIT]     = markflag(MAC),
> > > -    [FFLT]    = markflag(MAC),    [FFMT]    = markflag(MAC),
> > > -    [SCC]     = markflag(MAC),    [FCRUC]   = markflag(MAC),
> > > -    [LATECOL] = markflag(MAC),    [COLC]    = markflag(MAC),
> > > -    [SEQEC]   = markflag(MAC),    [CEXTERR] = markflag(MAC),
> > > -    [XONTXC]  = markflag(MAC),    [XOFFRXC] = markflag(MAC),
> > > -    [RJC]     = markflag(MAC),    [RNBC]    = markflag(MAC),
> > > -    [MGTPDC]  = markflag(MAC),    [MGTPTC]  = markflag(MAC),
> > > -    [RUC]     = markflag(MAC),    [ROC]     = markflag(MAC),
> > > -    [GORCL]   = markflag(MAC),    [GORCH]   = markflag(MAC),
> > > -    [GOTCL]   = markflag(MAC),    [GOTCH]   = markflag(MAC),
> > > -    [BPRC]    = markflag(MAC),    [MPRC]    = markflag(MAC),
> > > -    [TSCTC]   = markflag(MAC),    [PRC64]   = markflag(MAC),
> > > -    [PRC127]  = markflag(MAC),    [PRC255]  = markflag(MAC),
> > > -    [PRC511]  = markflag(MAC),    [PRC1023] = markflag(MAC),
> > > -    [PRC1522] = markflag(MAC),    [PTC64]   = markflag(MAC),
> > > -    [PTC127]  = markflag(MAC),    [PTC255]  = markflag(MAC),
> > > -    [PTC511]  = markflag(MAC),    [PTC1023] = markflag(MAC),
> > > -    [PTC1522] = markflag(MAC),    [MPTC]    = markflag(MAC),
> > > -    [BPTC]    = markflag(MAC),
> >
> > I think this is wrong. All those registers should still be marked with 
> > MAC_ACCESS_FLAG_NEEDED, shouldn't they?
>
> I followed Paolo's removal in commit fa4ec9ffda7
> ("e1000: remove old compatibility code"):
>
> -    [RDTR]    = markflag(MIT),    [TADV]    = markflag(MIT),
> -    [RADV]    = markflag(MIT),    [ITR]     = markflag(MIT),
>
> Is it the same problem?

Oops, sorry, I think I got confused by that MAC_ACCESS_FLAG_NEEDED logic in 
e1000_mmio_read() and e1000_mmio_write() ... after reading it more 
carefully, I think it is ok what you did here.

But maybe you could even remove the markflag macro and 
MAC_ACCESS_FLAG_NEEDED now completely?

 Thomas