RE: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor

[Sriram Yagnaraman]

> -----Original Message-----
> From: qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org
> <qemu-devel-bounces+sriram.yagnaraman=est.tech@nongnu.org> On Behalf
> Of Akihiko Odaki
> Sent: Thursday, 16 March 2023 16:57
> Cc: qemu-devel@nongnu.org; Jason Wang <jasowang@redhat.com>; Dmitry
> Fleytman <dmitry.fleytman@gmail.com>; quintela@redhat.com; Philippe
> Mathieu-Daudé <philmd@linaro.org>; Akihiko Odaki
> <akihiko.odaki@daynix.com>
> Subject: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
> 
> The current implementation of igb uses only part of a advanced Tx context
> descriptor because it misses some features and sniffs the trait of the packet
> instead of respecting the packet type specified in the descriptor. However, we
> will certainly need the entire Tx context descriptor when we update igb to
> respect these ignored fields. Save the entire Tx context descriptor to prepare
> for such a change.
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> V1 -> V2: Bump igb-tx version
> 
>  hw/net/igb.c      | 10 ++++++----
>  hw/net/igb_core.c | 17 ++++++++++-------  hw/net/igb_core.h |  3 +--
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/igb.c b/hw/net/igb.c index c6d753df87..f9ec82fc28 100644
> --- a/hw/net/igb.c
> +++ b/hw/net/igb.c
> @@ -504,11 +504,13 @@ static int igb_post_load(void *opaque, int
> version_id)
> 
>  static const VMStateDescription igb_vmstate_tx = {
>      .name = "igb-tx",
> -    .version_id = 1,
> -    .minimum_version_id = 1,
> +    .version_id = 2,
> +    .minimum_version_id = 2,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT16(vlan, struct igb_tx),
> -        VMSTATE_UINT16(mss, struct igb_tx),
> +        VMSTATE_UINT32(ctx.vlan_macip_lens, struct igb_tx),
> +        VMSTATE_UINT32(ctx.seqnum_seed, struct igb_tx),
> +        VMSTATE_UINT32(ctx.type_tucmd_mlhl, struct igb_tx),
> +        VMSTATE_UINT32(ctx.mss_l4len_idx, struct igb_tx),
>          VMSTATE_BOOL(tse, struct igb_tx),
>          VMSTATE_BOOL(ixsm, struct igb_tx),
>          VMSTATE_BOOL(txsm, struct igb_tx), diff --git a/hw/net/igb_core.c
> b/hw/net/igb_core.c index a7c7bfdc75..304f5d849f 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -390,7 +390,8 @@ static bool
>  igb_setup_tx_offloads(IGBCore *core, struct igb_tx *tx)  {
>      if (tx->tse) {
> -        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, tx->mss)) {
> +        uint32_t mss = tx->ctx.mss_l4len_idx >> 16;
> +        if (!net_tx_pkt_build_vheader(tx->tx_pkt, true, true, mss)) {
>              return false;
>          }
> 
> @@ -550,8 +551,10 @@ igb_process_tx_desc(IGBCore *core,
>                     E1000_ADVTXD_DTYP_CTXT) {
>              /* advanced transmit context descriptor */
>              tx_ctx_desc = (struct e1000_adv_tx_context_desc *)tx_desc;
> -            tx->vlan = le32_to_cpu(tx_ctx_desc->vlan_macip_lens) >> 16;
> -            tx->mss = le32_to_cpu(tx_ctx_desc->mss_l4len_idx) >> 16;
> +            tx->ctx.vlan_macip_lens = 
> le32_to_cpu(tx_ctx_desc->vlan_macip_lens);
> +            tx->ctx.seqnum_seed = le32_to_cpu(tx_ctx_desc->seqnum_seed);
> +            tx->ctx.type_tucmd_mlhl = le32_to_cpu(tx_ctx_desc-
> >type_tucmd_mlhl);
> +            tx->ctx.mss_l4len_idx =
> + le32_to_cpu(tx_ctx_desc->mss_l4len_idx);

Wouldn't it be better to parse the context into all the required fields like 
vlan, mss, etc., already when handling the context descriptor, instead of 
parsing it for every data descriptor later?
Also, in my yet to be merged patch [1] which handles VLAN insertion for VMDq I 
use the vlan field in multiple places, so it would be better to have the vlan 
value readily available. 
[1]: https://lists.gnu.org/archive/html/qemu-devel/2023-02/msg00393.html

>              return;
>          } else {
>              /* unknown descriptor type */ @@ -575,8 +578,9 @@
> igb_process_tx_desc(IGBCore *core,
>      if (cmd_type_len & E1000_TXD_CMD_EOP) {
>          if (!tx->skip_cp && net_tx_pkt_parse(tx->tx_pkt)) {
>              if (cmd_type_len & E1000_TXD_CMD_VLE) {
> -                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, tx->vlan,
> -                    core->mac[VET] & 0xffff);
> +                uint16_t vlan = tx->ctx.vlan_macip_lens >> 16;
> +                uint16_t vet = core->mac[VET] & 0xffff;
> +                net_tx_pkt_setup_vlan_header_ex(tx->tx_pkt, vlan, vet);
>              }
>              if (igb_tx_pkt_send(core, tx, queue_index)) {
>                  igb_on_tx_done_update_stats(core, tx->tx_pkt); @@ -4024,8
> +4028,7 @@ static void igb_reset(IGBCore *core, bool sw)
>      for (i = 0; i < ARRAY_SIZE(core->tx); i++) {
>          tx = &core->tx[i];
>          net_tx_pkt_reset(tx->tx_pkt);
> -        tx->vlan = 0;
> -        tx->mss = 0;
> +        memset(&tx->ctx, 0, sizeof(tx->ctx));
>          tx->tse = false;
>          tx->ixsm = false;
>          tx->txsm = false;
> diff --git a/hw/net/igb_core.h b/hw/net/igb_core.h index
> 814c1e264b..3483edc655 100644
> --- a/hw/net/igb_core.h
> +++ b/hw/net/igb_core.h
> @@ -72,8 +72,7 @@ struct IGBCore {
>      QEMUTimer *autoneg_timer;
> 
>      struct igb_tx {
> -        uint16_t vlan;  /* VLAN Tag */
> -        uint16_t mss;   /* Maximum Segment Size */
> +        struct e1000_adv_tx_context_desc ctx;
>          bool tse;       /* TCP/UDP Segmentation Enable */
>          bool ixsm;      /* Insert IP Checksum */
>          bool txsm;      /* Insert TCP/UDP Checksum */
> --
> 2.39.2
>