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

[Thomas Huth <thuth@redhat.com>]

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