1. Patchew
  2. QEMU
  3. [PATCH 00/21] hw/i386/pc: Remove deprecated 2.4 and 2.5 PC machines
  4. View patch

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

Philippe Mathieu-Daudé posted 21 patches 1 year ago
There is a newer version of this series
[PATCH 08/21] hw/net/e1000: Remove unused E1000_FLAG_MAC flag
Posted by Philippe Mathieu-Daudé 1 year ago 
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),
-
-    [TDFH]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [TDFT]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [TDFHS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [TDFTS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [TDFPC] = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [RDFH]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [RDFT]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [RDFHS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [RDFTS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [RDFPC] = markflag(MAC) | MAC_ACCESS_PARTIAL,
-    [PBM]   = markflag(MAC) | MAC_ACCESS_PARTIAL,
+    [TDFH]  = MAC_ACCESS_PARTIAL,
+    [TDFT]  = MAC_ACCESS_PARTIAL,
+    [TDFHS] = MAC_ACCESS_PARTIAL,
+    [TDFTS] = MAC_ACCESS_PARTIAL,
+    [TDFPC] = MAC_ACCESS_PARTIAL,
+    [RDFH]  = MAC_ACCESS_PARTIAL,
+    [RDFT]  = MAC_ACCESS_PARTIAL,
+    [RDFHS] = MAC_ACCESS_PARTIAL,
+    [RDFTS] = MAC_ACCESS_PARTIAL,
+    [RDFPC] = MAC_ACCESS_PARTIAL,
+    [PBM]   = MAC_ACCESS_PARTIAL,
 };
 
 static void
@@ -1419,13 +1388,6 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id)
     return 0;
 }
 
-static bool e1000_full_mac_needed(void *opaque)
-{
-    E1000State *s = opaque;
-
-    return chkflag(MAC);
-}
-
 static bool e1000_tso_state_needed(void *opaque)
 {
     E1000State *s = opaque;
@@ -1451,7 +1413,6 @@ static const VMStateDescription vmstate_e1000_full_mac_state = {
     .name = "e1000/full_mac_state",
     .version_id = 1,
     .minimum_version_id = 1,
-    .needed = e1000_full_mac_needed,
     .fields = (const VMStateField[]) {
         VMSTATE_UINT32_ARRAY(mac_reg, E1000State, 0x8000),
         VMSTATE_END_OF_LIST()
@@ -1679,8 +1640,6 @@ static void pci_e1000_realize(PCIDevice *pci_dev, Error **errp)
 
 static const Property e1000_properties[] = {
     DEFINE_NIC_PROPERTIES(E1000State, conf),
-    DEFINE_PROP_BIT("extra_mac_registers", E1000State,
-                    compat_flags, E1000_FLAG_MAC_BIT, true),
     DEFINE_PROP_BIT("migrate_tso_props", E1000State,
                     compat_flags, E1000_FLAG_TSO_BIT, true),
     DEFINE_PROP_BIT("init-vet", E1000State,
-- 
2.47.1
Re: [PATCH 08/21] hw/net/e1000: Remove unused E1000_FLAG_MAC flag
Posted by Thomas Huth 1 year ago 
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?

  Thomas


> -    [TDFH]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFT]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFHS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFTS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFPC] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFH]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFT]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFHS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFTS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFPC] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [PBM]   = markflag(MAC) | MAC_ACCESS_PARTIAL,
> +    [TDFH]  = MAC_ACCESS_PARTIAL,
> +    [TDFT]  = MAC_ACCESS_PARTIAL,
> +    [TDFHS] = MAC_ACCESS_PARTIAL,
> +    [TDFTS] = MAC_ACCESS_PARTIAL,
> +    [TDFPC] = MAC_ACCESS_PARTIAL,
> +    [RDFH]  = MAC_ACCESS_PARTIAL,
> +    [RDFT]  = MAC_ACCESS_PARTIAL,
> +    [RDFHS] = MAC_ACCESS_PARTIAL,
> +    [RDFTS] = MAC_ACCESS_PARTIAL,
> +    [RDFPC] = MAC_ACCESS_PARTIAL,
> +    [PBM]   = MAC_ACCESS_PARTIAL,
>   };
>   
>   static void
> @@ -1419,13 +1388,6 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id)
>       return 0;
>   }
>   
> -static bool e1000_full_mac_needed(void *opaque)
> -{
> -    E1000State *s = opaque;
> -
> -    return chkflag(MAC);
> -}
> -
>   static bool e1000_tso_state_needed(void *opaque)
>   {
>       E1000State *s = opaque;
> @@ -1451,7 +1413,6 @@ static const VMStateDescription vmstate_e1000_full_mac_state = {
>       .name = "e1000/full_mac_state",
>       .version_id = 1,
>       .minimum_version_id = 1,
> -    .needed = e1000_full_mac_needed,
>       .fields = (const VMStateField[]) {
>           VMSTATE_UINT32_ARRAY(mac_reg, E1000State, 0x8000),
>           VMSTATE_END_OF_LIST()
> @@ -1679,8 +1640,6 @@ static void pci_e1000_realize(PCIDevice *pci_dev, Error **errp)
>   
>   static const Property e1000_properties[] = {
>       DEFINE_NIC_PROPERTIES(E1000State, conf),
> -    DEFINE_PROP_BIT("extra_mac_registers", E1000State,
> -                    compat_flags, E1000_FLAG_MAC_BIT, true),
>       DEFINE_PROP_BIT("migrate_tso_props", E1000State,
>                       compat_flags, E1000_FLAG_TSO_BIT, true),
>       DEFINE_PROP_BIT("init-vet", E1000State,
Re: [PATCH 08/21] hw/net/e1000: Remove unused E1000_FLAG_MAC flag
Posted by Philippe Mathieu-Daudé 1 year ago 
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?

> 
>   Thomas
Re: [PATCH 08/21] hw/net/e1000: Remove unused E1000_FLAG_MAC flag
Posted by Thomas Huth 1 year ago 
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
Re: [PATCH 08/21] hw/net/e1000: Remove unused E1000_FLAG_MAC flag
Posted by Daniel P. Berrangé 1 year ago 
On Thu, Jan 16, 2025 at 12:22:34AM +0100, 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(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

> 
> 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),
> -
> -    [TDFH]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFT]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFHS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFTS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [TDFPC] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFH]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFT]  = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFHS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFTS] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [RDFPC] = markflag(MAC) | MAC_ACCESS_PARTIAL,
> -    [PBM]   = markflag(MAC) | MAC_ACCESS_PARTIAL,
> +    [TDFH]  = MAC_ACCESS_PARTIAL,
> +    [TDFT]  = MAC_ACCESS_PARTIAL,
> +    [TDFHS] = MAC_ACCESS_PARTIAL,
> +    [TDFTS] = MAC_ACCESS_PARTIAL,
> +    [TDFPC] = MAC_ACCESS_PARTIAL,
> +    [RDFH]  = MAC_ACCESS_PARTIAL,
> +    [RDFT]  = MAC_ACCESS_PARTIAL,
> +    [RDFHS] = MAC_ACCESS_PARTIAL,
> +    [RDFTS] = MAC_ACCESS_PARTIAL,
> +    [RDFPC] = MAC_ACCESS_PARTIAL,
> +    [PBM]   = MAC_ACCESS_PARTIAL,

It seems like this is removing all use of the 'markflag' macro ?

If I'm correct, then that macro can be removed. That in turnm means
the 'MAC_ACCESS_FLAG_NEEDED' enum entry can also be removed, as well
as places in code that check this flag.

>  };
>  
>  static void
> @@ -1419,13 +1388,6 @@ static int e1000_tx_tso_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> -static bool e1000_full_mac_needed(void *opaque)
> -{
> -    E1000State *s = opaque;
> -
> -    return chkflag(MAC);
> -}
> -
>  static bool e1000_tso_state_needed(void *opaque)
>  {
>      E1000State *s = opaque;
> @@ -1451,7 +1413,6 @@ static const VMStateDescription vmstate_e1000_full_mac_state = {
>      .name = "e1000/full_mac_state",
>      .version_id = 1,
>      .minimum_version_id = 1,
> -    .needed = e1000_full_mac_needed,
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32_ARRAY(mac_reg, E1000State, 0x8000),
>          VMSTATE_END_OF_LIST()
> @@ -1679,8 +1640,6 @@ static void pci_e1000_realize(PCIDevice *pci_dev, Error **errp)
>  
>  static const Property e1000_properties[] = {
>      DEFINE_NIC_PROPERTIES(E1000State, conf),
> -    DEFINE_PROP_BIT("extra_mac_registers", E1000State,
> -                    compat_flags, E1000_FLAG_MAC_BIT, true),
>      DEFINE_PROP_BIT("migrate_tso_props", E1000State,
>                      compat_flags, E1000_FLAG_TSO_BIT, true),
>      DEFINE_PROP_BIT("init-vet", E1000State,
> -- 
> 2.47.1
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.