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

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Friday, 17 March 2023 06:46
> To: Sriram Yagnaraman <sriram.yagnaraman@est.tech>
> 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>
> Subject: Re: [PATCH for 8.0 v2] igb: Save the entire Tx context descriptor
> 
> On 2023/03/17 5:27, Sriram Yagnaraman wrote:
> >
> >> -----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
> 
> If there is a better representation of the entire context descriptor we may 
> use it
> as an internal use, but I think it is good enough for the purpose, too.

If I understand correctly this change will help to realize "Table 7-31: Valid 
context vs Required Offload"? 
If so, the internal representation can already be prepared to hold the 
following: 
- vlan
- l4len
- iplen
- maclen
- mss
- l4type
- ip4

If we have an internal representation, the fields can hold values from the 
context, or from VMDq registers, or from the data descriptor depending on what 
the driver has configured.
Anyhow, I don't want to derail this patch, you already have ACKs, please go 
ahead if you don't want to do this now.

> 
> For patch [1], I think it is better to gather the logic to derive the VID 
> into one
> place instead of cluttering several places with the relevant code. Concretely,
> igb_tx_insert_vlan() can decide whether to read the VID from the context
> descriptor or from VMIR, or not to read (in case VLAN insertion is disabled).
> 
> Regards,
> Akihiko Odaki
> 
> >
> >>               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
> >>
> >