Re: [PATCH v3 03/12] hw/net/e1000e|igb: Remove xitr_guest_value logic

[Akihiko Odaki <akihiko.odaki@daynix.com>]

On 2025/05/02 12:16, Nicholas Piggin wrote:
> The guest value xITR logic is not required now that the write functions
> store necessary data to be read back, and internal users mask and shift
> fields they need as they go.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/net/e1000e_core.c | 31 +++++++++++++++----------------
>   hw/net/igb_core.c    | 16 +++++++++++++---
>   2 files changed, 28 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> index 96f74f1ea14..f8e6522f810 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -2563,18 +2563,6 @@ e1000e_mac_swsm_read(E1000ECore *core, int index)
>       return val;
>   }
>   
> -static uint32_t
> -e1000e_mac_itr_read(E1000ECore *core, int index)
> -{
> -    return core->itr_guest_value;
> -}
> -
> -static uint32_t
> -e1000e_mac_eitr_read(E1000ECore *core, int index)
> -{
> -    return core->eitr_guest_value[index - EITR];
> -}
> -
>   static uint32_t
>   e1000e_mac_icr_read(E1000ECore *core, int index)
>   {
> @@ -2792,7 +2780,6 @@ e1000e_set_itr(E1000ECore *core, int index, uint32_t val)
>   
>       trace_e1000e_irq_itr_set(val);
>   
> -    core->itr_guest_value = interval;
>       core->mac[index] = interval;
>   }
>   
> @@ -2804,7 +2791,6 @@ e1000e_set_eitr(E1000ECore *core, int index, uint32_t val)
>   
>       trace_e1000e_irq_eitr_set(eitr_num, val);
>   
> -    core->eitr_guest_value[eitr_num] = interval;
>       core->mac[index] = interval;
>   }
>   
> @@ -3029,6 +3015,7 @@ static const readops e1000e_macreg_readops[] = {
>       e1000e_getreg(GSCN_1),
>       e1000e_getreg(FCAL),
>       e1000e_getreg(FLSWCNT),
> +    e1000e_getreg(ITR),
>   
>       [TOTH]    = e1000e_mac_read_clr8,
>       [GOTCH]   = e1000e_mac_read_clr8,
> @@ -3062,7 +3049,6 @@ static const readops e1000e_macreg_readops[] = {
>       [MPRC]    = e1000e_mac_read_clr4,
>       [BPTC]    = e1000e_mac_read_clr4,
>       [TSCTC]   = e1000e_mac_read_clr4,
> -    [ITR]     = e1000e_mac_itr_read,
>       [CTRL]    = e1000e_get_ctrl,
>       [TARC1]   = e1000e_get_tarc,
>       [SWSM]    = e1000e_mac_swsm_read,
> @@ -3087,7 +3073,7 @@ static const readops e1000e_macreg_readops[] = {
>       [RETA ... RETA + 31]   = e1000e_mac_readreg,
>       [RSSRK ... RSSRK + 31] = e1000e_mac_readreg,
>       [MAVTV0 ... MAVTV3]    = e1000e_mac_readreg,
> -    [EITR...EITR + E1000E_MSIX_VEC_NUM - 1] = e1000e_mac_eitr_read
> +    [EITR...EITR + E1000E_MSIX_VEC_NUM - 1] = e1000e_mac_readreg,
>   };
>   enum { E1000E_NREADOPS = ARRAY_SIZE(e1000e_macreg_readops) };
>   
> @@ -3517,13 +3503,26 @@ void e1000e_core_pre_save(E1000ECore *core)
>               core->tx[i].skip_cp = true;
>           }
>       }
> +
> +    /* back compat, QEMU moves xITR in itr_guest_value state */
> +    core->itr_guest_value = core->mac[ITR];
> +    for (i = 0; i < E1000E_MSIX_VEC_NUM; i++) {
> +        core->eitr_guest_value[i] = core->mac[EITR + i];
> +    }
>   }
>   
>   int
>   e1000e_core_post_load(E1000ECore *core)
>   {
> +    int i;
>       NetClientState *nc = qemu_get_queue(core->owner_nic);
>   
> +    /* back compat */
> +    core->mac[ITR] = core->itr_guest_value;
> +    for (i = 0; i < E1000E_MSIX_VEC_NUM; i++) {
> +        core->mac[EITR + i] = core->eitr_guest_value[i];
> +    }
> +
>       /*
>        * nc.link_down can't be migrated, so infer link_down according
>        * to link status bit in core.mac[STATUS].
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c
> index 39e3ce1c8fe..271c54380e9 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -2880,7 +2880,7 @@ igb_mac_swsm_read(IGBCore *core, int index)
>   static uint32_t
>   igb_mac_eitr_read(IGBCore *core, int index)
>   {
> -    return core->eitr_guest_value[index - EITR0];
> +    return core->mac[index - EITR0];
>   }
>   
>   static uint32_t igb_mac_vfmailbox_read(IGBCore *core, int index)
> @@ -3046,8 +3046,7 @@ igb_set_eitr(IGBCore *core, int index, uint32_t val)
>   
>       trace_igb_irq_eitr_set(eitr_num, val);
>   
> -    core->eitr_guest_value[eitr_num] = val & ~E1000_EITR_CNT_IGNR;
> -    core->mac[index] = val & 0x7FFE;
> +    core->mac[index] = val;

This change will keep the CNT_INGR although the specification says it 
"is always read as zero".

>   }
>   
>   static void
> @@ -4527,13 +4526,24 @@ void igb_core_pre_save(IGBCore *core)
>               core->tx[i].skip_cp = true;
>           }
>       }
> +
> +    /* back compat, QEMU moves EITR in eitr_guest_value state */
> +    for (i = 0; i < IGB_INTR_NUM; i++) {
> +        core->eitr_guest_value[i] = core->mac[EITR0 + i];
> +    }
>   }
>   
>   int
>   igb_core_post_load(IGBCore *core)
>   {
> +    int i;
>       NetClientState *nc = qemu_get_queue(core->owner_nic);
>   
> +    /* back compat */
> +    for (i = 0; i < IGB_INTR_NUM; i++) {
> +        core->mac[EITR0 + i] = core->eitr_guest_value[i];
> +    }
> +
>       /*
>        * nc.link_down can't be migrated, so infer link_down according
>        * to link status bit in core.mac[STATUS].